Skip to content
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

Making sure that sa_family is always initialized #2239

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jan561
Copy link
Contributor

@Jan561 Jan561 commented Dec 1, 2023

Alternative to #2235

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

@Jan561
Copy link
Contributor Author

Jan561 commented Dec 1, 2023

Actually this fixes nothing as recvmsg doesn't even call SockaddrLike::from_raw. Instead, it just assumes correct initialization and doesn't check anything

Ok(unsafe { read_mhdr(mhdr, r, msg_controllen, address.assume_init()) })

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to SockaddrLikePriv::as_mut_ptr is unnecessary. You can do this manipulation in pack_mhdr_to_receive on raw pointers directly.
Also, could you please add a test that reproduces the original bug?

@Jan561
Copy link
Contributor Author

Jan561 commented Dec 2, 2023

The change to SockaddrLikePriv::as_mut_ptr is unnecessary. You can do this manipulation in pack_mhdr_to_receive on raw pointers directly.

Actually that's not possible, the address: *mut S argument is not NULL for S = (), it is just dangling.

That's how this function is called, with address being initialized with MaybeUninit::<S>::uninit():

pack_mhdr_to_receive(iov.as_mut().as_mut_ptr(), iov.len(), msg_control, msg_controllen, address.as_mut_ptr())

Also, could you please add a test that reproduces the original bug?

Adding a test for this isn't that trivial, since currently the address field is "random" if not initialized by the syscall.

But the set of possible family values is quite small on BSDs (sa_family_t = u8), so I think with a test that calls recvmsg 500 or 1000 times on a connected socket we can be almost certain that one iteration will have the family field preset to e.g. INET or INET6.

But before we can add this test, we need to fix recvmsg

@Jan561
Copy link
Contributor Author

Jan561 commented Dec 2, 2023

I'd also consider the recvmsg situation a blocking issue for this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants