Fix panic on dropping RecvAncillaryBuffer after failed recvmsg#676
Fix panic on dropping RecvAncillaryBuffer after failed recvmsg#676sunfishcode merged 2 commits intobytecodealliance:mainfrom
RecvAncillaryBuffer after failed recvmsg#676Conversation
notgull
left a comment
There was a problem hiding this comment.
notgull code after spending five minutes in production:
| if res.is_ok() { | ||
| unsafe { | ||
| control.set_control_len(msghdr.msg_controllen.try_into().unwrap_or(usize::MAX)); | ||
| } |
There was a problem hiding this comment.
I'm not sure if the contents of the control buffer are defined on error. I think it would be safest to set the buffer's length to zero on error.
There was a problem hiding this comment.
set_control_len just sets the length (and read) field of the RecvAncillaryBuffer, which isn't part of the buffer that libc will be touching so it won't be changed by the recvmsg call if this set_control_len line isn't run.
And for a newly allocated RecvAncillaryBuffer, it will already be 0. So there's no need to set it, in that case.
If the same RecvAncillaryBuffer is re-used in multiple calls, I'm not sure how that works currently. Should check that doesn't leak file descriptors or anything.
There was a problem hiding this comment.
We should probably be clearing out all of the inner data before we do anything with it.
There was a problem hiding this comment.
Added a commit that clears the RecvAncillaryBuffer at the start of the recv operation.
| if res.is_ok() { | ||
| unsafe { | ||
| control.set_control_len(msghdr.msg_controllen.try_into().unwrap_or(usize::MAX)); | ||
| } | ||
| } |
Calling drain on `RecvAncillaryBuffer`, including in its `Drop` implementation was failing, with an "attempted to subtract with overflow" error in `cvt_msg`. If `recvmsg` returns `-1`, `msg_controllen` will not be updated by the call. So it had a non-zero value as passed into the function, despite there not being any control messages to parse.
This should prevent leaks of file descriptors if the `RecvAncillaryBuffer` is re-used for multiple reads.
|
@notgull Do you have any further comments here? |
|
@notgull Is this ready to merge? |
|
Thanks! |
Calling drain on
RecvAncillaryBuffer, including in itsDropimplementation was failing, with an "attempted to subtract with overflow" error incvt_msg.If
recvmsgreturns-1,msg_controllenwill not be updated by the call. So it had a non-zero value as passed into the function, despite there not being any control messages to parse.