Merge #1244

1244: Clippy cleanup r=asomers a=asomers

Reported-by: Clippy

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

CHANGELOG.md

@@ -33,6 +33,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
   (#[1233](https://github.com/nix-rust/nix/pull/1233))
 - Added `EventFilter` bitflags for `EV_DISPATCH` and `EV_RECEIPT` on OpenBSD.
   (#[1252](https://github.com/nix-rust/nix/pull/1252))
+- `CpuSet` now implements `Default`.
+- `CpuSet` and `UnixCredentials` now implement `Default`.
+  (#[1244](https://github.com/nix-rust/nix/pull/1244))
 
 ### Changed
 - Changed `fallocate` return type from `c_int` to `()` (#[1201](https://github.com/nix-rust/nix/pull/1201))
@@ -42,6 +45,15 @@ This project adheres to [Semantic Versioning](http://semver.org/).
   (#[1245](https://github.com/nix-rust/nix/pull/1245))
 - `execv`, `execve`, `execvp` and `execveat` in `::nix::unistd` and `reboot` in
   `::nix::sys::reboot` now return `Result<Infallible>` instead of `Result<Void>` (#[1239](https://github.com/nix-rust/nix/pull/1239))
+- `sys::socket::sockaddr_storage_to_addr` is no longer `unsafe`.  So is
+  `offset_of!`.
+- `sys::socket::sockaddr_storage_to_addr`, `offset_of!`, and `Errno::clear` are
+  no longer `unsafe`.
+- `SockAddr::as_ffi_pair`,`sys::socket::sockaddr_storage_to_addr`, `offset_of!`,
+  and `Errno::clear` are no longer `unsafe`.
+  (#[1244](https://github.com/nix-rust/nix/pull/1244))
+- Several `Inotify` methods now take `self` by value instead of by reference
+  (#[1244](https://github.com/nix-rust/nix/pull/1244))
 
 ### Fixed
 
@@ -59,6 +71,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
   correctness's sake across all architectures and compilers, though now bugs
   have been reported so far.
   (#[1243](https://github.com/nix-rust/nix/pull/1243))
+- Fixed unaligned pointer read in `Inotify::read_events`.
+  (#[1244](https://github.com/nix-rust/nix/pull/1244))
 
 ### Removed
 

src/errno.rs

@@ -46,8 +46,11 @@ cfg_if! {
 }
 
 /// Sets the platform-specific errno to no-error
-unsafe fn clear() {
-    *errno_location() = 0;
+fn clear() {
+    // Safe because errno is a thread-local variable
+    unsafe {
+        *errno_location() = 0;
+    }
 }
 
 /// Returns the platform-specific value of errno
@@ -70,7 +73,7 @@ impl Errno {
         from_i32(err)
     }
 
-    pub unsafe fn clear() {
+    pub fn clear() {
         clear()
     }
 

src/fcntl.rs

@@ -161,6 +161,8 @@ libc_bitflags!(
     }
 );
 
+// The conversion is not identical on all operating systems.
+#[allow(clippy::identity_conversion)]
 pub fn open<P: ?Sized + NixPath>(path: &P, oflag: OFlag, mode: Mode) -> Result<RawFd> {
     let fd = path.with_nix_path(|cstr| {
         unsafe { libc::open(cstr.as_ptr(), oflag.bits(), mode.bits() as c_uint) }
@@ -169,6 +171,8 @@ pub fn open<P: ?Sized + NixPath>(path: &P, oflag: OFlag, mode: Mode) -> Result<R
     Errno::result(fd)
 }
 
+// The conversion is not identical on all operating systems.
+#[allow(clippy::identity_conversion)]
 #[cfg(not(target_os = "redox"))]
 pub fn openat<P: ?Sized + NixPath>(
     dirfd: RawFd,

src/macros.rs

@@ -208,7 +208,11 @@ macro_rules! libc_enum {
 /// offset of `field` within struct `ty`
 #[cfg(not(target_os = "redox"))]
 macro_rules! offset_of {
-    ($ty:ty, $field:ident) => {
-        &(*(ptr::null() as *const $ty)).$field as *const _ as usize
-    }
+    ($ty:ty, $field:ident) => {{
+        // Safe because we don't actually read from the dereferenced pointer
+        #[allow(unused_unsafe)] // for when the macro is used in an unsafe block
+        unsafe {
+            &(*(ptr::null() as *const $ty)).$field as *const _ as usize
+        }
+    }}
 }

src/sched.rs

@@ -80,7 +80,8 @@ mod sched_linux_like {
             if field >= CpuSet::count() {
                 Err(Error::Sys(Errno::EINVAL))
             } else {
-                Ok(unsafe { libc::CPU_SET(field, &mut self.cpu_set) })
+                unsafe { libc::CPU_SET(field, &mut self.cpu_set); }
+                Ok(())
             }
         }
 
@@ -90,7 +91,8 @@ mod sched_linux_like {
             if field >= CpuSet::count() {
                 Err(Error::Sys(Errno::EINVAL))
             } else {
-                Ok(unsafe { libc::CPU_CLR(field, &mut self.cpu_set) })
+                unsafe { libc::CPU_CLR(field, &mut self.cpu_set);}
+                Ok(())
             }
         }
 
@@ -100,6 +102,12 @@ mod sched_linux_like {
         }
     }
 
+    impl Default for CpuSet {
+        fn default() -> Self {
+            Self::new()
+        }
+    }
+
     /// `sched_setaffinity` set a thread's CPU affinity mask
     /// ([`sched_setaffinity(2)`](http://man7.org/linux/man-pages/man2/sched_setaffinity.2.html))
     ///
@@ -181,8 +189,8 @@ mod sched_linux_like {
 
         let res = unsafe {
             let combined = flags.bits() | signal.unwrap_or(0);
-            let ptr = stack.as_mut_ptr().offset(stack.len() as isize);
-            let ptr_aligned = ptr.offset((ptr as usize % 16) as isize * -1);
+            let ptr = stack.as_mut_ptr().add(stack.len());
+            let ptr_aligned = ptr.sub(ptr as usize % 16);
             libc::clone(
                 mem::transmute(
                     callback as extern "C" fn(*mut Box<dyn FnMut() -> isize>) -> i32,

src/sys/inotify.rs

@@ -29,8 +29,9 @@ use libc::{
 };
 use std::ffi::{OsString,OsStr,CStr};
 use std::os::unix::ffi::OsStrExt;
-use std::mem::size_of;
+use std::mem::{MaybeUninit, size_of};
 use std::os::unix::io::{RawFd,AsRawFd,FromRawFd};
+use std::ptr;
 use crate::unistd::read;
 use crate::Result;
 use crate::NixPath;
@@ -130,7 +131,7 @@ impl Inotify {
     /// Returns a watch descriptor. This is not a File Descriptor! 
     ///
     /// For more information see, [inotify_add_watch(2)](http://man7.org/linux/man-pages/man2/inotify_add_watch.2.html).
-    pub fn add_watch<P: ?Sized + NixPath>(&self,
+    pub fn add_watch<P: ?Sized + NixPath>(self,
                                           path: &P,
                                           mask: AddWatchFlags) 
                                             -> Result<WatchDescriptor>
@@ -151,14 +152,14 @@ impl Inotify {
     ///
     /// For more information see, [inotify_rm_watch(2)](http://man7.org/linux/man-pages/man2/inotify_rm_watch.2.html).
     #[cfg(target_os = "linux")]
-    pub fn rm_watch(&self, wd: WatchDescriptor) -> Result<()> {
+    pub fn rm_watch(self, wd: WatchDescriptor) -> Result<()> {
         let res = unsafe { libc::inotify_rm_watch(self.fd, wd.wd) };
 
         Errno::result(res).map(drop)
     }
 
     #[cfg(target_os = "android")]
-    pub fn rm_watch(&self, wd: WatchDescriptor) -> Result<()> {
+    pub fn rm_watch(self, wd: WatchDescriptor) -> Result<()> {
         let res = unsafe { libc::inotify_rm_watch(self.fd, wd.wd as u32) };
 
         Errno::result(res).map(drop)
@@ -170,21 +171,24 @@ impl Inotify {
     /// 
     /// Returns as many events as available. If the call was non blocking and no
     /// events could be read then the EAGAIN error is returned.
-    pub fn read_events(&self) -> Result<Vec<InotifyEvent>> {
+    pub fn read_events(self) -> Result<Vec<InotifyEvent>> {
         let header_size = size_of::<libc::inotify_event>();
-        let mut buffer = [0u8; 4096];
+        const BUFSIZ: usize = 4096;
+        let mut buffer = [0u8; BUFSIZ];
         let mut events = Vec::new();
         let mut offset = 0;
 
         let nread = read(self.fd, &mut buffer)?;
 
         while (nread - offset) >= header_size {
             let event = unsafe {
-                &*(
-                    buffer
-                        .as_ptr()
-                        .offset(offset as isize) as *const libc::inotify_event
-                )
+                let mut event = MaybeUninit::<libc::inotify_event>::uninit();
+                ptr::copy_nonoverlapping(
+                    buffer.as_ptr().add(offset),
+                    event.as_mut_ptr() as *mut u8,
+                    (BUFSIZ - offset).min(header_size)
+                );
+                event.assume_init()
             };
 
             let name = match event.len {
@@ -193,7 +197,7 @@ impl Inotify {
                     let ptr = unsafe { 
                         buffer
                             .as_ptr()
-                            .offset(offset as isize + header_size as isize)
+                            .add(offset + header_size)
                             as *const c_char
                     };
                     let cstr = unsafe { CStr::from_ptr(ptr) };

src/sys/mman.rs

@@ -260,20 +260,37 @@ libc_bitflags!{
     }
 }
 
-/// Locks all memory pages that contain part of the address range with `length` bytes starting at
-/// `addr`. Locked pages never move to the swap area.
+/// Locks all memory pages that contain part of the address range with `length`
+/// bytes starting at `addr`.
+///
+/// Locked pages never move to the swap area.
+///
+/// # Safety
+///
+/// `addr` must meet all the requirements described in the `mlock(2)` man page.
 pub unsafe fn mlock(addr: *const c_void, length: size_t) -> Result<()> {
     Errno::result(libc::mlock(addr, length)).map(drop)
 }
 
-/// Unlocks all memory pages that contain part of the address range with `length` bytes starting at
-/// `addr`.
+/// Unlocks all memory pages that contain part of the address range with
+/// `length` bytes starting at `addr`.
+///
+/// # Safety
+///
+/// `addr` must meet all the requirements described in the `munlock(2)` man
+/// page.
 pub unsafe fn munlock(addr: *const c_void, length: size_t) -> Result<()> {
     Errno::result(libc::munlock(addr, length)).map(drop)
 }
 
-/// Locks all memory pages mapped into this process' address space. Locked pages never move to the
-/// swap area.
+/// Locks all memory pages mapped into this process' address space.
+///
+/// Locked pages never move to the swap area.
+///
+/// # Safety
+///
+/// `addr` must meet all the requirements described in the `mlockall(2)` man
+/// page.
 pub fn mlockall(flags: MlockAllFlags) -> Result<()> {
     unsafe { Errno::result(libc::mlockall(flags.bits())) }.map(drop)
 }
@@ -283,8 +300,11 @@ pub fn munlockall() -> Result<()> {
     unsafe { Errno::result(libc::munlockall()) }.map(drop)
 }
 
-/// Calls to mmap are inherently unsafe, so they must be made in an unsafe block. Typically
-/// a higher-level abstraction will hide the unsafe interactions with the mmap'd region.
+/// allocate memory, or map files or devices into memory
+///
+/// # Safety
+///
+/// See the `mmap(2)` man page for detailed requirements.
 pub unsafe fn mmap(addr: *mut c_void, length: size_t, prot: ProtFlags, flags: MapFlags, fd: RawFd, offset: off_t) -> Result<*mut c_void> {
     let ret = libc::mmap(addr, length, prot.bits(), flags.bits(), fd, offset);
 
@@ -295,10 +315,22 @@ pub unsafe fn mmap(addr: *mut c_void, length: size_t, prot: ProtFlags, flags: Ma
     }
 }
 
+/// remove a mapping
+///
+/// # Safety
+///
+/// `addr` must meet all the requirements described in the `munmap(2)` man
+/// page.
 pub unsafe fn munmap(addr: *mut c_void, len: size_t) -> Result<()> {
     Errno::result(libc::munmap(addr, len)).map(drop)
 }
 
+/// give advice about use of memory
+///
+/// # Safety
+///
+/// See the `madvise(2)` man page.  Take special care when using
+/// `MmapAdvise::MADV_FREE`.
 pub unsafe fn madvise(addr: *mut c_void, length: size_t, advise: MmapAdvise) -> Result<()> {
     Errno::result(libc::madvise(addr, length, advise as i32)).map(drop)
 }
@@ -332,6 +364,12 @@ pub unsafe fn mprotect(addr: *mut c_void, length: size_t, prot: ProtFlags) -> Re
     Errno::result(libc::mprotect(addr, length, prot.bits())).map(drop)
 }
 
+/// synchronize a mapped region
+///
+/// # Safety
+///
+/// `addr` must meet all the requirements described in the `msync(2)` man
+/// page.
 pub unsafe fn msync(addr: *mut c_void, length: size_t, flags: MsFlags) -> Result<()> {
     Errno::result(libc::msync(addr, length, flags.bits())).map(drop)
 }

src/sys/socket/addr.rs

@@ -768,39 +768,60 @@ impl SockAddr {
     /// with the size of the actual data type. sockaddr is commonly used as a proxy for
     /// a superclass as C doesn't support inheritance, so many functions that take
     /// a sockaddr * need to take the size of the underlying type as well and then internally cast it back.
-    pub unsafe fn as_ffi_pair(&self) -> (&libc::sockaddr, libc::socklen_t) {
+    pub fn as_ffi_pair(&self) -> (&libc::sockaddr, libc::socklen_t) {
         match *self {
             SockAddr::Inet(InetAddr::V4(ref addr)) => (
-                &*(addr as *const libc::sockaddr_in as *const libc::sockaddr),
+                // This cast is always allowed in C
+                unsafe {
+                    &*(addr as *const libc::sockaddr_in as *const libc::sockaddr)
+                },
                 mem::size_of_val(addr) as libc::socklen_t
             ),
             SockAddr::Inet(InetAddr::V6(ref addr)) => (
-                &*(addr as *const libc::sockaddr_in6 as *const libc::sockaddr),
+                // This cast is always allowed in C
+                unsafe {
+                    &*(addr as *const libc::sockaddr_in6 as *const libc::sockaddr)
+                },
                 mem::size_of_val(addr) as libc::socklen_t
             ),
             SockAddr::Unix(UnixAddr(ref addr, len)) => (
-                &*(addr as *const libc::sockaddr_un as *const libc::sockaddr),
+                // This cast is always allowed in C
+                unsafe {
+                    &*(addr as *const libc::sockaddr_un as *const libc::sockaddr)
+                },
                 (len + offset_of!(libc::sockaddr_un, sun_path)) as libc::socklen_t
             ),
             #[cfg(any(target_os = "android", target_os = "linux"))]
             SockAddr::Netlink(NetlinkAddr(ref sa)) => (
-                &*(sa as *const libc::sockaddr_nl as *const libc::sockaddr),
+                // This cast is always allowed in C
+                unsafe {
+                    &*(sa as *const libc::sockaddr_nl as *const libc::sockaddr)
+                },
                 mem::size_of_val(sa) as libc::socklen_t
             ),
             #[cfg(any(target_os = "android", target_os = "linux"))]
             SockAddr::Alg(AlgAddr(ref sa)) => (
-                &*(sa as *const libc::sockaddr_alg as *const libc::sockaddr),
+                // This cast is always allowed in C
+                unsafe {
+                    &*(sa as *const libc::sockaddr_alg as *const libc::sockaddr)
+                },
                 mem::size_of_val(sa) as libc::socklen_t
             ),
             #[cfg(any(target_os = "ios", target_os = "macos"))]
             SockAddr::SysControl(SysControlAddr(ref sa)) => (
-                &*(sa as *const libc::sockaddr_ctl as *const libc::sockaddr),
+                // This cast is always allowed in C
+                unsafe {
+                    &*(sa as *const libc::sockaddr_ctl as *const libc::sockaddr)
+                },
                 mem::size_of_val(sa) as libc::socklen_t
 
             ),
             #[cfg(any(target_os = "android", target_os = "linux"))]
             SockAddr::Link(LinkAddr(ref addr)) => (
-                &*(addr as *const libc::sockaddr_ll as *const libc::sockaddr),
+                // This cast is always allowed in C
+                unsafe {
+                    &*(addr as *const libc::sockaddr_ll as *const libc::sockaddr)
+                },
                 mem::size_of_val(addr) as libc::socklen_t
             ),
             #[cfg(any(target_os = "dragonfly",
@@ -810,12 +831,18 @@ impl SockAddr {
                       target_os = "netbsd",
                       target_os = "openbsd"))]
             SockAddr::Link(LinkAddr(ref addr)) => (
-                &*(addr as *const libc::sockaddr_dl as *const libc::sockaddr),
+                // This cast is always allowed in C
+                unsafe {
+                    &*(addr as *const libc::sockaddr_dl as *const libc::sockaddr)
+                },
                 mem::size_of_val(addr) as libc::socklen_t
             ),
             #[cfg(target_os = "linux")]
             SockAddr::Vsock(VsockAddr(ref sa)) => (
-                &*(sa as *const libc::sockaddr_vm as *const libc::sockaddr),
+                // This cast is always allowed in C
+                unsafe {
+                    &*(sa as *const libc::sockaddr_vm as *const libc::sockaddr)
+                },
                 mem::size_of_val(sa) as libc::socklen_t
             ),
         }