From cc6d88bfd3b7b65168b61608f24aaf190ce6f2ee Mon Sep 17 00:00:00 2001 From: Jason White Date: Wed, 13 Sep 2023 15:01:16 -0700 Subject: [PATCH] ci: Fix clippy warnings (#23) Summary: Fixes all current clippy warnings[1] so CI is green again. Fixing the warning `clippy::needless_pass_by_ref_mut` became a little involved. The internal version of clippy isn't recent enough to have this warning and so just doing `#[allow(clippy::needless_pass_by_ref_mut)]` leads to another error. The resulting change fixes the clippy warning and gets rid of some of the shenanigans I was doing to avoid allocating path buffers within the child process. [1]: https://github.com/facebookexperimental/reverie/actions/runs/6164196219/job/16729396694 Pull Request resolved: https://github.com/facebookexperimental/reverie/pull/23 Reviewed By: VladimirMakaev Differential Revision: D49209202 Pulled By: jasonwhite fbshipit-source-id: b03ff432783910bef11fc239d146659dc2c0db30 --- reverie-examples/chunky_print.rs | 2 +- reverie-process/src/fd.rs | 76 +++++++++++++++++++--------- reverie-process/src/lib.rs | 5 +- reverie-process/src/mount.rs | 4 +- reverie-process/src/seccomp/notif.rs | 2 +- 5 files changed, 59 insertions(+), 30 deletions(-) diff --git a/reverie-examples/chunky_print.rs b/reverie-examples/chunky_print.rs index e3bbe3f..d57732b 100644 --- a/reverie-examples/chunky_print.rs +++ b/reverie-examples/chunky_print.rs @@ -74,7 +74,7 @@ impl GlobalTool for ChunkyPrintGlobal { let mut mg = self.0.lock().unwrap(); match m { Msg::Print(w, s) => { - let v = mg.printbuf.entry(from).or_insert_with(Vec::new); + let v = mg.printbuf.entry(from).or_default(); v.push((w, s)); } Msg::Tick => { diff --git a/reverie-process/src/fd.rs b/reverie-process/src/fd.rs index 2905779..b8ea8c6 100644 --- a/reverie-process/src/fd.rs +++ b/reverie-process/src/fd.rs @@ -10,7 +10,6 @@ use core::pin::Pin; use core::task::Context; use core::task::Poll; use std::ffi::CStr; -use std::ffi::CString; use std::io; use std::io::Read; use std::io::Write; @@ -378,20 +377,37 @@ pub fn is_dir(path: *const libc::c_char) -> bool { } } -fn cstring_as_slice(s: &mut CString) -> &mut [libc::c_char] { - let bytes = s.as_bytes_with_nul(); - unsafe { - // This is safe because we are already provided a mutable `CString` and - // we don't alias the two mutable references. - core::slice::from_raw_parts_mut(bytes.as_ptr() as *mut libc::c_char, bytes.len()) +/// Copies the bytes of a `CStr` to a buffer. Helpful to avoid allocations when +/// performing path operations in a child process that hasn't called `execve` +/// yet. +fn copy_cstr_to_slice<'a>( + s: &CStr, + buf: &'a mut [libc::c_char], +) -> Result<&'a mut [libc::c_char], Errno> { + let bytes = s.to_bytes_with_nul(); + + if bytes.len() > buf.len() { + return Err(Errno::ENAMETOOLONG); } + + unsafe { + core::ptr::copy_nonoverlapping( + bytes.as_ptr() as *const libc::c_char, + buf.as_mut_ptr(), + bytes.len(), + ) + }; + + Ok(&mut buf[0..bytes.len()]) } /// Creates every path component in `path` without allocating. This is done by -/// replacing each `/` with a NUL terminator as needed (and then changing the -/// `\0` back to `/` afterwards). -pub fn create_dir_all(path: &mut CString, mode: libc::mode_t) -> Result<(), Errno> { - create_dir_all_(cstring_as_slice(path), mode) +/// copying the path to a static buffer and replacing each `/` with a NUL +/// terminator as needed (and then changing the `\0` back to `/` afterwards). +pub fn create_dir_all(path: &CStr, mode: libc::mode_t) -> Result<(), Errno> { + let mut buf = ['\0' as libc::c_char; libc::PATH_MAX as usize]; + let path = copy_cstr_to_slice(path, &mut buf)?; + create_dir_all_(path, mode) } /// Helper function. The last character in the path is always `\0`. @@ -431,11 +447,13 @@ fn create_dir_all_(path: &mut [libc::c_char], mode: libc::mode_t) -> Result<(), /// Creates an empty file at `path` without allocating. pub fn touch_path( - path: &mut CString, + path: &CStr, file_mode: libc::mode_t, dir_mode: libc::mode_t, ) -> Result<(), Errno> { - touch_path_(cstring_as_slice(path), file_mode, dir_mode) + let mut buf = ['\0' as libc::c_char; libc::PATH_MAX as usize]; + let path = copy_cstr_to_slice(path, &mut buf)?; + touch_path_(path, file_mode, dir_mode) } /// Helper function. The last character in the path is always `\0`. @@ -493,12 +511,30 @@ where #[cfg(test)] mod tests { + use std::ffi::CString; use std::os::unix::ffi::OsStrExt; use const_cstr::const_cstr; use super::*; + #[test] + fn test_copy_cstr_to_slice() { + const BYTES: &[u8] = b"/foo/bar\0"; + let s = CStr::from_bytes_with_nul(BYTES).unwrap(); + + // Buffer exactly the right size. + let mut buf = [0; BYTES.len()]; + assert_eq!( + copy_cstr_to_slice(s, &mut buf).unwrap().len(), + s.to_bytes_with_nul().len() + ); + + // Buffer too small to hold last NUL byte. + let mut buf = [0; BYTES.len() - 1]; + assert_eq!(copy_cstr_to_slice(s, &mut buf), Err(Errno::ENAMETOOLONG)); + } + #[test] fn test_is_dir() { assert!(is_dir(const_cstr!("/").as_ptr())); @@ -524,7 +560,7 @@ mod tests { #[test] fn test_create_dir_all() { let tempdir = tempfile::TempDir::new().unwrap(); - let mut path = CString::new( + let path = CString::new( tempdir .path() .join("some/path/to/a/dir") @@ -532,11 +568,8 @@ mod tests { .as_bytes(), ) .unwrap(); - let path2 = path.clone(); - - create_dir_all(&mut path, 0o777).unwrap(); - assert_eq!(path, path2); + create_dir_all(&path, 0o777).unwrap(); assert!(is_dir(path.as_ptr())); } @@ -544,7 +577,7 @@ mod tests { #[test] fn test_touch_path() { let tempdir = tempfile::TempDir::new().unwrap(); - let mut path = CString::new( + let path = CString::new( tempdir .path() .join("some/path/to/a/file") @@ -552,11 +585,8 @@ mod tests { .as_bytes(), ) .unwrap(); - let path2 = path.clone(); - - touch_path(&mut path, 0o666, 0o777).unwrap(); - assert_eq!(path, path2); + touch_path(&path, 0o666, 0o777).unwrap(); assert!(FileType::new(path.as_ptr()).unwrap().is_file()); } diff --git a/reverie-process/src/lib.rs b/reverie-process/src/lib.rs index 7d10535..758eb56 100644 --- a/reverie-process/src/lib.rs +++ b/reverie-process/src/lib.rs @@ -227,9 +227,7 @@ mod tests { .trim_end() .split('\n') .map(|line| { - let mut items = line.splitn(2, ':'); - let first = items.next().unwrap(); - let second = items.next().unwrap(); + let (first, second) = line.split_once(':').unwrap(); (first, second.trim()) }) .collect() @@ -655,6 +653,7 @@ mod tests { .arg("/proc/self/status") .seccomp(filter) .seccomp_notify() + .stdout(Stdio::null()) .spawn() .unwrap(); diff --git a/reverie-process/src/mount.rs b/reverie-process/src/mount.rs index 4b533c5..3591ecc 100644 --- a/reverie-process/src/mount.rs +++ b/reverie-process/src/mount.rs @@ -292,9 +292,9 @@ impl Mount { // a different tmpfs. if let Some(src) = &self.source { if FileType::new(src.as_ptr())?.is_dir() { - create_dir_all(&mut self.target, 0o777)?; + create_dir_all(&self.target, 0o777)?; } else { - touch_path(&mut self.target, 0o666, 0o777)?; + touch_path(&self.target, 0o666, 0o777)?; } } } diff --git a/reverie-process/src/seccomp/notif.rs b/reverie-process/src/seccomp/notif.rs index c6ae01c..570ecc2 100644 --- a/reverie-process/src/seccomp/notif.rs +++ b/reverie-process/src/seccomp/notif.rs @@ -283,7 +283,7 @@ impl futures::stream::Stream for SeccompNotif { /// currently pending, the operation blocks until an event occurs. /// /// NOTE: This is only available since Linux 5.0. -fn seccomp_notif_recv(fd: &mut Fd) -> io::Result { +fn seccomp_notif_recv(fd: &Fd) -> io::Result { // According to the docs, this struct must be zeroed out first. let mut response = core::mem::MaybeUninit::::zeroed();