Set the length of a socket address when calling `recvmsg` on Linux #2041

JarredAllen · 2023-05-20T00:12:09Z

Background

I think I've found a bug with recvmsg on Linux where it doesn't set the length of a received socket address. I found this while working with unix datagram sockets with path struct sockaddr_un addresses, but I think this applies to any variable-length socket address type.

At present, the sun_len field of UnixAddr on Linux shows up as 0 whenever I call recvmsg. I think it's reading uninitialized memory (afaict the recvmsg function never initializes it before we call MaybeUninit::assume_init), though it's possible that it's being zero-initialized somewhere that I missed. Either way, this isn't the correct behavior, which should set it to the length of the struct sockaddr_un contents (or whatever other type of socket).

Changes

I changed the recvmsg function to construct the returned socket address from the struct sockaddr and length returned by the libc recvmsg call (using the from_raw function the trait provides). Since the trait is sealed so downstream crates can't implement it, I believe this shouldn't be a breaking change.

Validation

I've tested that my code (which failed due to this bug) now works. I also added a new test case which tests that we construct UnixAddrs correctly in the recvmsg function, which passes locally for me and fails if I cherry-pick it onto the current master.

I've also checked that cargo test and cargo clippy both still pass on aarch64-apple-darwin and on aarch64-unknown-linux-musl targets (the two targets I develop for/have access to). Hopefully your CI will confirm that everything else still works.

asomers

It's unfortunate that this change results in an extra data copy, in S::from_raw. Instead, what about adding a SockaddrLike::set_len method? recvmsg would call it after libc::recv_msg. For most socket types, it would be a noop, but for UnixAddr on Linux it would set the sun_len field.

asomers · 2023-05-20T14:51:06Z

test/sys/test_socket.rs

+        print!("Couldn't send ({e:?}), so skipping test");
+        return;


Suggested change

print!("Couldn't send ({e:?}), so skipping test");

return;

skip!("Couldn't send ({e:?}), so skipping test");

Done in 72a2f56

asomers · 2023-05-20T14:56:41Z

src/sys/socket/mod.rs

@@ -2048,20 +2047,23 @@ pub fn recvmsg<'a, 'outer, 'inner, S>(fd: RawFd, iov: &'outer mut [IoSliceMut<'i
    where S: SockaddrLike + 'a,
    'inner: 'outer
 {
-    let mut address = mem::MaybeUninit::uninit();
+    let mut address: libc::sockaddr_storage = unsafe { mem::MaybeUninit::zeroed().assume_init() };


Why use sockaddr_storage instead of S?

It didn't pass tests when I used S, I think because () is shorter than the actual socket address type so it ran out of room (though I didn't check for the cause, so I might be wrong about that). Though the point is moot as I put it back to how it was before for implementing your suggestion about the different approach.

JarredAllen · 2023-05-22T18:54:40Z

Instead, what about adding a SockaddrLike::set_len method? recvmsg would call it after libc::recv_msg.

I implemented that change, though I wasn't sure if you wanted it to be publicly-visible or not. I can hide it from the docs and mark it an internal implementation detail, if you prefer.

Also, I made it return an error if you try to set the length on any socket address type whose length is static (anything other that UnixAddr or SockaddrStorage containing a UnixAddr internally), but I could make it validate the length or some other behavior instead, if you'd prefer that.

asomers

I like this version a lot better. Just a few inline comments, and:

Please do add #[doc(hidden)] to set_length, as you suggested, and
Please add a CHANGELOG entry.

asomers · 2023-07-15T22:24:07Z

src/sys/socket/addr.rs

+
+    /// Set the length of this socket address
+    ///
+    /// This method may only be called on socket addresses whose lenghts are dynamic, and it


Suggested change

/// This method may only be called on socket addresses whose lenghts are dynamic, and it

/// This method may only be called on socket addresses whose lengths are dynamic, and it

asomers · 2023-07-15T22:29:45Z

src/sys/socket/addr.rs

+                cfg_if! {
+                    if #[cfg(any(target_os = "android",
+                                 target_os = "fuchsia",
+                                 target_os = "illumos",
+                                 target_os = "linux",
+                                 target_os = "redox",
+                        ))] {
+                        addr.sun_len = new_length as u8;
+                    } else {
+                        addr.sun.sun_len = new_length as u8;
+                    }
+                }
+                Ok(())


Suggested change

cfg_if! {

if #[cfg(any(target_os = "android",

target_os = "fuchsia",

target_os = "illumos",

target_os = "linux",

target_os = "redox",

))] {

addr.sun_len = new_length as u8;

} else {

addr.sun.sun_len = new_length as u8;

}

}

Ok(())

addr.set_length(new_length)

asomers · 2023-07-15T22:30:39Z

src/sys/socket/addr.rs

+    ///
+    /// `new_length` must be a valid length for this type of address. Specifically, reads of that
+    /// length from `self` must be valid.
+    unsafe fn set_length(&mut self, new_length: usize) -> std::result::Result<(), SocketAddressLengthNotDynamic>;


If you add a default implementation here, then you can avoid a lot of boilerplate elsewhere.

Suggested change

unsafe fn set_length(&mut self, new_length: usize) -> std::result::Result<(), SocketAddressLengthNotDynamic>;

unsafe fn set_length(&mut self, new_length: usize) -> std::result::Result<(), SocketAddressLengthNotDynamic> {

Err(SocketAddressLengthNotDynamic)

}

asomers · 2023-07-15T22:32:54Z

src/sys/socket/mod.rs

@@ -2048,7 +2053,7 @@ pub fn recvmsg<'a, 'outer, 'inner, S>(fd: RawFd, iov: &'outer mut [IoSliceMut<'i
    where S: SockaddrLike + 'a,
    'inner: 'outer
 {
-    let mut address = mem::MaybeUninit::uninit();
+    let mut address = mem::MaybeUninit::zeroed();


Why do you think it's necessary to zero the address here?

I originally thought it might be necessary when I first started working on this (since the length was being left uninitialized initially), but looking back at it now I think everything is initialized now so I put it back to uninit()

JarredAllen · 2023-07-17T18:38:49Z

I like this version a lot better. Just a few inline comments, and:
* Please do add `#[doc(hidden)]` to `set_length`, as you suggested, and

* Please add a CHANGELOG entry.

I've done those changes, please let me know if there's anything else you want to see.

src/sys/socket/addr.rs

Co-authored-by: Alan Somers <asomers@gmail.com>

asomers · 2023-07-17T21:54:23Z

The linter isn't happy with that change.

JarredAllen · 2023-07-17T21:58:44Z

The linter isn't happy with that change.

Fixed! I had to make an allowance because the variable was only used on some platforms and was left unused on others.

asomers

bors r+

bors · 2023-07-17T22:35:51Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

2041: Set the length of a socket address when calling `recvmsg` on Linux r=asomers a=JarredAllen # Background I think I've found a bug with `recvmsg` on Linux where it doesn't set the length of a received socket address. I found this while working with unix datagram sockets with path `struct sockaddr_un` addresses, but I think this applies to any variable-length socket address type. At present, the `sun_len` field of `UnixAddr` on Linux shows up as 0 whenever I call `recvmsg`. I think it's reading uninitialized memory (afaict the `recvmsg` function never initializes it before we call `MaybeUninit::assume_init`), though it's possible that it's being zero-initialized somewhere that I missed. Either way, this isn't the correct behavior, which should set it to the length of the `struct sockaddr_un` contents (or whatever other type of socket). # Changes I changed the `recvmsg` function to construct the returned socket address from the `struct sockaddr` and length returned by the libc `recvmsg` call (using the `from_raw` function the trait provides). Since the trait is sealed so downstream crates can't implement it, I believe this shouldn't be a breaking change. # Validation I've tested that my code (which failed due to this bug) now works. I also added a new test case which tests that we construct `UnixAddr`s correctly in the `recvmsg` function, which passes locally for me and fails if I cherry-pick it onto the current `master`. I've also checked that `cargo test` and `cargo clippy` both still pass on `aarch64-apple-darwin` and on `aarch64-unknown-linux-musl` targets (the two targets I develop for/have access to). Hopefully your CI will confirm that everything else still works. Co-authored-by: Jarred Allen <jarred@moveparallel.com> Co-authored-by: Jarred Allen <jarredallen73@gmail.com>

JarredAllen added 2 commits May 19, 2023 16:40

Set the length of a sockaddr received on Linux

905c23e

Add test coverage for bug with unset socket address length

96fa5a8

asomers requested changes May 20, 2023

View reviewed changes

JarredAllen added 2 commits May 22, 2023 11:45

PR suggestion: Set the length later instead of using from_raw

c80828e

PR suggestions: use skip macro

72a2f56

JarredAllen added 2 commits May 22, 2023 12:43

Fixup accidentally-introduced changes

e5bd9ba

Remove redundant unsafe block

ff0fb3d

JarredAllen requested a review from asomers May 22, 2023 19:48

Merge branch 'master' into set-sockaddr-length-linux

3174d85

asomers requested changes Jul 15, 2023

View reviewed changes

JarredAllen added 2 commits July 17, 2023 11:33

More PR feedback

012e788

Merge branch 'master' into set-sockaddr-length-linux

23125cb

JarredAllen requested a review from asomers July 17, 2023 18:38

asomers requested changes Jul 17, 2023

View reviewed changes

src/sys/socket/addr.rs Outdated Show resolved Hide resolved

Correctly let the kernel set the length on BSDs

2378f12

Co-authored-by: Alan Somers <asomers@gmail.com>

JarredAllen requested a review from asomers July 17, 2023 21:45

Fix lint

2d60044

asomers approved these changes Jul 17, 2023

View reviewed changes

bors bot merged commit c375a10 into nix-rust:master Jul 17, 2023
40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set the length of a socket address when calling `recvmsg` on Linux #2041

Set the length of a socket address when calling `recvmsg` on Linux #2041

JarredAllen commented May 20, 2023

asomers left a comment

asomers May 20, 2023

JarredAllen May 22, 2023

asomers May 20, 2023

JarredAllen May 22, 2023

JarredAllen commented May 22, 2023 •

edited

Loading

asomers left a comment

asomers Jul 15, 2023

asomers Jul 15, 2023

asomers Jul 15, 2023

asomers Jul 15, 2023

JarredAllen Jul 17, 2023

JarredAllen commented Jul 17, 2023

asomers commented Jul 17, 2023

JarredAllen commented Jul 17, 2023

asomers left a comment

bors bot commented Jul 17, 2023

	print!("Couldn't send ({e:?}), so skipping test");
	return;
	skip!("Couldn't send ({e:?}), so skipping test");

	/// This method may only be called on socket addresses whose lenghts are dynamic, and it
	/// This method may only be called on socket addresses whose lengths are dynamic, and it

-    unsafe fn set_length(&mut self, new_length: usize) -> std::result::Result<(), SocketAddressLengthNotDynamic>;
+    unsafe fn set_length(&mut self, new_length: usize) -> std::result::Result<(), SocketAddressLengthNotDynamic> {
+        Err(SocketAddressLengthNotDynamic)
+    }

Set the length of a socket address when calling recvmsg on Linux #2041

Set the length of a socket address when calling recvmsg on Linux #2041

Conversation

JarredAllen commented May 20, 2023

Background

Changes

Validation

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JarredAllen commented May 22, 2023 • edited Loading

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JarredAllen commented Jul 17, 2023

asomers commented Jul 17, 2023

JarredAllen commented Jul 17, 2023

asomers left a comment

Choose a reason for hiding this comment

bors bot commented Jul 17, 2023

Set the length of a socket address when calling `recvmsg` on Linux #2041

Set the length of a socket address when calling `recvmsg` on Linux #2041

JarredAllen commented May 22, 2023 •

edited

Loading