Fix `struct msghdr` types on non-Linux platforms. #648

kinetiknz · 2017-07-05T23:40:51Z

Type struct msghdr and struct cmsghdr types defined in sys/socket/ffi.rs match the Linux headers, but differ from the standard header (and other OSes): http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_socket.h.html

This PR fixes a memory corruption/unsafety issue on non-Linux machines when using recvmsg with certain parameters.

While fixing this, I wondered what the reason was for nix not using libc's definition of these structures.

Closes #748.

Susurrus · 2017-07-06T02:55:45Z

I'd like to take the policy that we never fix FFI stuff directly within nix, instead only fixing in libc and switching our types to use those. All the types defined in socket::ffi actually already exist in libc. Can you just remove that module and use the libc types directly instead?

Susurrus · 2017-07-06T02:56:53Z

Whoops, should have read your entire post. The goal is for nix to exclusively use libc datatypes for all platforms, the only reason not to would be if they aren't fixing their end.

kinetiknz · 2017-07-10T05:54:50Z

Can you just remove that module and use the libc types directly instead?

Will do. Updated PR coming soon.

Susurrus · 2017-07-12T22:35:19Z

CHANGELOG.md

@@ -66,6 +66,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  ([#623](https://github.com/nix-rust/nix/pull/623))
 - Multiple constants related to the termios API have now been properly defined for
  all supported platforms. ([#527](https://github.com/nix-rust/nix/pull/527))
+- Fixed field types of `sys::socket::ffi::{msghdr, cmsghdr}` on
+  non-Linux platforms.  ([#](https://...))


You can fill this in at this point. It won't change between now and when it's committed.

Susurrus · 2017-07-12T22:36:34Z

src/sys/socket/ffi.rs

@@ -33,9 +48,9 @@ pub struct msghdr<'a> {
    pub msg_name: *const c_void,


Both msghdr and cmsghdr here should be removed in favor of the declarations in libc.

kinetiknz · 2017-08-09T00:47:41Z

Apologies for the delay, I've been busy with other stuff. I'll try to get the PR updated this week with the removal of socket/ffi.rs and various fixes for this area of the code. Most of the work is already complete, it just needs tidying up.

Susurrus · 2017-09-20T14:27:18Z

@kinetiknz I actually did some work in this area (see #748). Would you be willing to push this through? This is the last remaining FFI declarations in nix.

kinetiknz · 2017-09-25T07:40:53Z

Sure thing, apologies for letting this sit so long!

Susurrus

Just a few minor comments, but I don't understand this code very well. I'd like to do a deeper dive on this, but before I do could you add some comments to help me out? You seem to have a good sense of this API, so it'd be really helpful getting more of that knowledge written down.

Additionally, do we have tests for this? If not, can some be written or is this a hard API for that?

Susurrus · 2017-10-14T16:02:32Z

src/sys/socket/mod.rs

@@ -198,8 +203,10 @@ use self::ffi::{cmsghdr, msghdr, type_of_cmsg_data, type_of_msg_iovlen, type_of_
 /// To make room for multiple messages, nest the type parameter with
 /// tuples, e.g.
 /// `let cmsg: CmsgSpace<([RawFd; 3], CmsgSpace<[RawFd; 2]>)> = CmsgSpace::new();`


I think we should make this a doccomment so it can be compile & run tested.

Susurrus · 2017-10-14T16:03:32Z

src/sys/socket/mod.rs

@@ -380,7 +388,7 @@ pub enum ControlMessage<'a> {
 pub struct UnknownCmsg<'a>(&'a cmsghdr, &'a [u8]);

 fn cmsg_align(len: usize) -> usize {


I'd suggest we add an #[inline] here as it's a small convenience function.

Could you also add a comment here summarizing this function? I'm having trouble following this code and having a comment for this helper function would be nice.

Susurrus · 2017-11-11T22:04:12Z

@kinetiknz Just pinging you again on this as it's the last nugget before we have all FFI types upstreamed.

kinetiknz · 2017-11-13T09:22:45Z

@kinetiknz Just pinging you again on this as it's the last nugget before we have all FFI types upstreamed.

Sorry! I'll push my changes tomorrow.

Susurrus

This all looks pretty correct to me, unfortunately I'm not too familiar with all of this. @asomers Could you take a look over this as well?

Susurrus · 2017-11-15T02:57:06Z

src/sys/socket/mod.rs

@@ -187,8 +188,12 @@ unsafe fn copy_bytes<'a, 'b, T: ?Sized>(src: &T, dst: &'a mut &'b mut [u8]) {
    mem::swap(dst, &mut remainder);
 }

+// OSX always aligns struct cmsghdr as if it were a 32-bit OS
+#[cfg(target_os = "macos")]


Does this apply to ios as well?

Looks like it, thanks.

Susurrus · 2017-11-15T02:57:41Z

src/sys/socket/mod.rs

@@ -187,8 +188,12 @@ unsafe fn copy_bytes<'a, 'b, T: ?Sized>(src: &T, dst: &'a mut &'b mut [u8]) {
    mem::swap(dst, &mut remainder);
 }

+// OSX always aligns struct cmsghdr as if it were a 32-bit OS
+#[cfg(target_os = "macos")]
+type align_of_cmsg_data = libc::c_uint;


Should this be u32 instead of c_uint? Even if they are the same I'd think u32 is more clear.

Susurrus · 2017-11-15T03:00:17Z

src/sys/socket/mod.rs

@@ -197,9 +202,16 @@ use self::ffi::{cmsghdr, msghdr, type_of_cmsg_data, type_of_msg_iovlen, type_of_
 ///
 /// To make room for multiple messages, nest the type parameter with
 /// tuples, e.g.


Remove the ", e.g." and add ":"

Susurrus · 2017-12-20T03:30:47Z

src/sys/socket/mod.rs

-                    cmsg_type: libc::SCM_RIGHTS,
-                    cmsg_data: [],
+                let cmsg = {
+                  let mut cmsg: cmsghdr = mem::uninitialized();


This would be more clear if rewritten to use struct update syntax:

let mut cmsg = cmsghdr { cmsg_len: self.len() as _, cmsg_level: libc::SOL_SOCKET, cmsg_type: libc::SCM_RIGHTS, ..unsafe { mem::uninitialized() } };

I agree that's nicer, but it looks like the musl target prevents this due to private fields. 😞

It's only a problem for msghdr, so I'll keep the cmsghdr changes and revert the rest.

Susurrus · 2017-11-15T03:20:29Z

Also, could you rebase and squash all the commits into one since this is really a single big change?

kinetiknz · 2017-11-23T03:05:20Z

Also, could you rebase and squash all the commits into one since this is really a single big change?

Will do, but I figured it'd be easier to review as a series of small commits.

Susurrus · 2017-12-04T04:41:58Z

@kinetiknz I think there are just a few small things to finish to wrap this up in a nice pretty bow. You have some time soon to finish this off?

kinetiknz · 2017-12-06T23:49:11Z

Sure, I should have time early next week.

Susurrus · 2017-12-13T16:17:23Z

@kinetiknz Will you have time to do some work on this this week? This is the last piece of nix that still relies on its own C types rather than libcs.

kinetiknz · 2017-12-20T03:06:01Z

@kinetiknz Will you have time to do some work on this this week? This is the last piece of nix that still relies on its own C types rather than libcs.

Sorry about the delay, rebased and squashed with the last set of comments addressed. Also adjusted the definition of align_of_cmsg_data for DragonFly BSD based on 709cbdf.

Susurrus

Yeah, I think we should basically go and merge this. I'd just ask for a simple alphetizing of a #cfg and the struct initializing could be more clear using struct update syntax. Hopefully that's not too much to ask for this PR, cause after that let's merge!

Susurrus · 2017-12-20T03:30:47Z

src/sys/socket/mod.rs

-                    cmsg_type: libc::SCM_RIGHTS,
-                    cmsg_data: [],
+                let cmsg = {
+                  let mut cmsg: cmsghdr = mem::uninitialized();


This would be more clear if rewritten to use struct update syntax:

let mut cmsg = cmsghdr { cmsg_len: self.len() as _, cmsg_level: libc::SOL_SOCKET, cmsg_type: libc::SCM_RIGHTS, ..unsafe { mem::uninitialized() } };

Susurrus · 2017-12-20T03:31:08Z

src/sys/socket/mod.rs

-                    cmsg_level: libc::SOL_SOCKET,
-                    cmsg_type: libc::SCM_TIMESTAMP,
-                    cmsg_data: [],
+                let cmsg = {


Same as above.

Susurrus · 2017-12-20T03:33:06Z

src/sys/socket/mod.rs

@@ -295,19 +296,30 @@ unsafe fn copy_bytes<'a, 'b, T: ?Sized>(src: &T, dst: &'a mut &'b mut [u8]) {
    mem::swap(dst, &mut remainder);
 }

+// Darwin and DragonFly BSD always aligns struct cmsghdr as if it were a 32-bit OS
+#[cfg(any(target_os = "ios", target_os = "macos", target_os = "dragonfly"))]


Can you alphabetize this list?

kinetiknz · 2017-12-20T03:49:19Z

No problem, let me know if there's anything else!

kinetiknz · 2017-12-20T05:04:59Z

Repushed with musl and iOS target fixes.

Susurrus

One last tiny nitpick, then I think we're definite RTM!

Susurrus · 2017-12-20T03:59:15Z

src/sys/socket/mod.rs

-                    cmsg_level: libc::SOL_SOCKET,
-                    cmsg_type: libc::SCM_RIGHTS,
-                    cmsg_data: [],
+                  cmsg_len: self.len() as _,


I think this indentation is messed up.

Not sure what happened there, but fixed version pushed.

Susurrus · 2017-12-20T08:17:07Z

Just realized this is more than fixing the FFI stuff, it actually fixes things on non-Linux platforms, so it's a bug fix. Can you add a CHANGELOG entry under the Fixed section for this?

…atch.

kinetiknz · 2017-12-20T08:38:00Z

Just realized this is more than fixing the FFI stuff, it actually fixes things on non-Linux platforms, so it's a bug fix. Can you add a CHANGELOG entry under the Fixed section for this?

Added something, suggestions welcome if that's not clear enough.

Susurrus · 2017-12-20T14:00:22Z

bors r+

648: Fix `struct msghdr` types on non-Linux platforms. r=Susurrus a=kinetiknz Type `struct msghdr` and `struct cmsghdr` types defined in `sys/socket/ffi.rs` match the Linux headers, but differ from the standard header (and other OSes): http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_socket.h.html This PR fixes a memory corruption/unsafety issue on non-Linux machines when using `recvmsg` with certain parameters. While fixing this, I wondered what the reason was for nix not using libc's definition of these structures. Closes #748.

bors · 2017-12-20T15:02:17Z

Build succeeded

Susurrus · 2017-12-20T15:04:44Z

Awesome, thanks for following through on this, @kinetiknz. And when/if you have time any additional tests and refinements are always appreciated!

kinetiknz force-pushed the msghdr_size branch 6 times, most recently from a39e409 to ed75b24 Compare July 10, 2017 04:29

kinetiknz force-pushed the msghdr_size branch from ed75b24 to bc98b99 Compare July 11, 2017 05:09

Susurrus suggested changes Jul 12, 2017

View reviewed changes

Susurrus mentioned this pull request Sep 20, 2017

Migrate all FFI declarations to libc #748

Closed

8 tasks

kinetiknz force-pushed the msghdr_size branch from bc98b99 to b35564f Compare September 27, 2017 10:29

Susurrus suggested changes Oct 14, 2017

View reviewed changes

kinetiknz force-pushed the msghdr_size branch from d9cb637 to b464f9b Compare November 15, 2017 00:45

Susurrus suggested changes Nov 15, 2017

View reviewed changes

kinetiknz force-pushed the msghdr_size branch 3 times, most recently from a950024 to a7ac04f Compare December 20, 2017 03:04

nix-rust deleted a comment from kinetiknz Dec 20, 2017

Susurrus suggested changes Dec 20, 2017

View reviewed changes

kinetiknz force-pushed the msghdr_size branch from a7ac04f to 9994365 Compare December 20, 2017 03:47

kinetiknz force-pushed the msghdr_size branch from 9994365 to 2c15dc9 Compare December 20, 2017 05:04

nix-rust deleted a comment from kinetiknz Dec 20, 2017

Susurrus suggested changes Dec 20, 2017

View reviewed changes

Susurrus mentioned this pull request Dec 20, 2017

Clippy cleanup #818

Merged

kinetiknz force-pushed the msghdr_size branch from 2c15dc9 to 87a8562 Compare December 20, 2017 08:09

Susurrus approved these changes Dec 20, 2017

View reviewed changes

Migrate sys/socket to libc FFI types and fix msghdr definition mism…

19ef148

…atch.

kinetiknz force-pushed the msghdr_size branch from 87a8562 to 19ef148 Compare December 20, 2017 08:36

bors bot merged commit 19ef148 into nix-rust:master Dec 20, 2017

asomers mentioned this pull request Jan 7, 2019

Allow cargo test to complete on OpenBSD 6.4/AMD64 #1001

Closed

		@@ -33,9 +48,9 @@ pub struct msghdr<'a> {
		pub msg_name: *const c_void,

		@@ -380,7 +388,7 @@ pub enum ControlMessage<'a> {
		pub struct UnknownCmsg<'a>(&'a cmsghdr, &'a [u8]);

		fn cmsg_align(len: usize) -> usize {

Fix struct msghdr types on non-Linux platforms. #648

Fix struct msghdr types on non-Linux platforms. #648

Conversation

kinetiknz commented Jul 5, 2017 • edited by Susurrus Loading

Susurrus commented Jul 6, 2017

Susurrus commented Jul 6, 2017

kinetiknz commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinetiknz commented Aug 9, 2017

Susurrus commented Sep 20, 2017

kinetiknz commented Sep 25, 2017

Susurrus 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

Susurrus commented Nov 11, 2017

kinetiknz commented Nov 13, 2017

Susurrus 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

kinetiknz Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Nov 15, 2017

kinetiknz commented Nov 23, 2017

Susurrus commented Dec 4, 2017

kinetiknz commented Dec 6, 2017

Susurrus commented Dec 13, 2017

kinetiknz commented Dec 20, 2017

Susurrus 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

kinetiknz commented Dec 20, 2017

kinetiknz commented Dec 20, 2017

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Dec 20, 2017

kinetiknz commented Dec 20, 2017

Susurrus commented Dec 20, 2017

bors bot commented Dec 20, 2017

Build succeeded

Susurrus commented Dec 20, 2017

Fix `struct msghdr` types on non-Linux platforms. #648

Fix `struct msghdr` types on non-Linux platforms. #648

kinetiknz commented Jul 5, 2017 •

edited by Susurrus

Loading

kinetiknz Dec 20, 2017 •

edited

Loading