From 987e58640da078eac3c7a0cb28022427bf348e2e Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Thu, 5 Dec 2024 10:47:05 -0700 Subject: [PATCH 1/3] Exclude NUL characters from OSStrings returned by getsockopt On FreeBSD, getsockopt appends a single NUL to the returned string. On Linux, it pads the entire buffer with NULs. But both of those behaviors may be version-dependent. Adjust GetOsString to handle any number of NUL characters. --- changelog/2557.fixed.md | 1 + src/sys/socket/sockopt.rs | 12 +++++++++--- test/sys/test_sockopt.rs | 19 +++++++++++++------ 3 files changed, 23 insertions(+), 9 deletions(-) create mode 100644 changelog/2557.fixed.md diff --git a/changelog/2557.fixed.md b/changelog/2557.fixed.md new file mode 100644 index 0000000000..fe6af72eea --- /dev/null +++ b/changelog/2557.fixed.md @@ -0,0 +1 @@ +Properly exclude NUL characters from `OSString`s returned by `getsockopt`. diff --git a/src/sys/socket/sockopt.rs b/src/sys/socket/sockopt.rs index 9515541b57..14019d45fa 100644 --- a/src/sys/socket/sockopt.rs +++ b/src/sys/socket/sockopt.rs @@ -7,8 +7,8 @@ use crate::{errno::Errno, Result}; use cfg_if::cfg_if; use libc::{self, c_int, c_void, socklen_t}; #[cfg(apple_targets)] -use std::ffi::{CStr, CString}; -use std::ffi::{OsStr, OsString}; +use std::ffi::CString; +use std::ffi::{CStr, OsStr, OsString}; use std::mem::{self, MaybeUninit}; use std::os::unix::ffi::OsStrExt; #[cfg(linux_android)] @@ -1745,7 +1745,13 @@ impl> Get for GetOsString { unsafe fn assume_init(self) -> OsString { let len = self.len as usize; let mut v = unsafe { self.val.assume_init() }; - OsStr::from_bytes(&v.as_mut()[0..len]).to_owned() + if let Ok(cs) = CStr::from_bytes_until_nul(&v.as_mut()[0..len]) { + OsStr::from_bytes(cs.to_bytes()) + } else { + // The OS returned a non-NUL-terminated string + OsStr::from_bytes(&v.as_mut()[0..len]) + } + .to_owned() } } diff --git a/test/sys/test_sockopt.rs b/test/sys/test_sockopt.rs index 32261ccf38..34bb0994fb 100644 --- a/test/sys/test_sockopt.rs +++ b/test/sys/test_sockopt.rs @@ -249,14 +249,13 @@ fn test_so_type_unknown() { } // The CI doesn't supported getsockopt and setsockopt on emulated processors. -// It's believed that a QEMU issue, the tests run ok on a fully emulated system. -// Current CI just run the binary with QEMU but the Kernel remains the same as the host. +// It's believed to be a QEMU issue; the tests run ok on a fully emulated +// system. Current CI just runs the binary with QEMU but the kernel remains the +// same as the host. // So the syscall doesn't work properly unless the kernel is also emulated. #[test] -#[cfg(all( - any(target_arch = "x86", target_arch = "x86_64"), - any(target_os = "freebsd", target_os = "linux") -))] +#[cfg(any(target_os = "freebsd", target_os = "linux"))] +#[cfg_attr(qemu, ignore)] fn test_tcp_congestion() { use std::ffi::OsString; @@ -269,6 +268,14 @@ fn test_tcp_congestion() { .unwrap(); let val = getsockopt(&fd, sockopt::TcpCongestion).unwrap(); + let bytes = val.as_os_str().as_encoded_bytes(); + for b in bytes.iter() { + assert_ne!( + *b, 0, + "OsString should contain no embedded NULs: {:?}", + val + ); + } setsockopt(&fd, sockopt::TcpCongestion, &val).unwrap(); setsockopt( From 764ff5d0d0600ba913d4bd5a4018caa979192004 Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Thu, 5 Dec 2024 10:54:19 -0700 Subject: [PATCH 2/3] fixup: fix MSRV breakage --- test/sys/test_sockopt.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/sys/test_sockopt.rs b/test/sys/test_sockopt.rs index 34bb0994fb..864604bd19 100644 --- a/test/sys/test_sockopt.rs +++ b/test/sys/test_sockopt.rs @@ -258,6 +258,7 @@ fn test_so_type_unknown() { #[cfg_attr(qemu, ignore)] fn test_tcp_congestion() { use std::ffi::OsString; + use std::os::unix::ffi::OsStrExt; let fd = socket( AddressFamily::Inet, @@ -268,7 +269,7 @@ fn test_tcp_congestion() { .unwrap(); let val = getsockopt(&fd, sockopt::TcpCongestion).unwrap(); - let bytes = val.as_os_str().as_encoded_bytes(); + let bytes = val.as_os_str().as_bytes(); for b in bytes.iter() { assert_ne!( *b, 0, From 075230d9f9901246af4853c4bfbc407461d7c768 Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Sat, 7 Dec 2024 07:18:30 -0700 Subject: [PATCH 3/3] Fixup: expand the comment --- src/sys/socket/sockopt.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sys/socket/sockopt.rs b/src/sys/socket/sockopt.rs index 14019d45fa..fec7fc5af5 100644 --- a/src/sys/socket/sockopt.rs +++ b/src/sys/socket/sockopt.rs @@ -1746,9 +1746,11 @@ impl> Get for GetOsString { let len = self.len as usize; let mut v = unsafe { self.val.assume_init() }; if let Ok(cs) = CStr::from_bytes_until_nul(&v.as_mut()[0..len]) { + // It's legal for the kernel to return any number of NULs at the + // end of the string. C applications don't care, after all. OsStr::from_bytes(cs.to_bytes()) } else { - // The OS returned a non-NUL-terminated string + // Even zero NULs is possible. OsStr::from_bytes(&v.as_mut()[0..len]) } .to_owned()