-
Notifications
You must be signed in to change notification settings - Fork 672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes recvm(m)sg blindly assuming correct initialization of received address #2249
base: master
Are you sure you want to change the base?
Conversation
Always fucking apple |
dd5bf36
to
12bf2c7
Compare
let addr_len = mhdr.msg_namelen; | ||
|
||
let address = unsafe { | ||
S::from_raw(address.as_ptr().cast(), Some(addr_len)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you aware that this creates a data copy? The original code was carefully designed to avoid such a thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it creates a copy, but so does recvfrom
(currently, on the master branch):
Line 2218 in cb56e89
Ok((ret, T::from_raw(addr.assume_init().as_ptr(), Some(len)))) |
I feel like in a future PR we should make the address
field of RecvMsg
private and instead add a getter that lazily performs the checks and the copy, like this:
pub fn address(&self) -> Option<S> {
S::from_raw(self.address.as_ptr(), len)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it matter that some other function also performs a data copy? Are you pointing that out to suggest that we improve it? I'm requesting that you not add an additional data copy to recvmsg
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay yeah, that makes sense, I think I misunderstood you before. I'll change the signature of read_mhdr
back to expecting a pointer, and perform the conversion there.
let addr_len = mhdr.msg_namelen; | ||
|
||
let address = unsafe { | ||
S::from_raw(address.as_ptr().cast(), Some(addr_len)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it matter that some other function also performs a data copy? Are you pointing that out to suggest that we improve it? I'm requesting that you not add an additional data copy to recvmsg
.
12bf2c7
to
b18d167
Compare
b18d167
to
73a8941
Compare
recvm(m)sg
now callingSockaddrLike::from_raw
instead ofassume_init
on the received addressChecklist:
CONTRIBUTING.md