Merge #1575

1575: Fix unsoundness in FdSet methods r=asomers a=taylordotfish

Ensure file descriptors are nonnegative and less than `FD_SETSIZE`. (Fixes #1572.)

Co-authored-by: taylor.fish <[email protected]>

CHANGELOG.md

@@ -3,6 +3,19 @@
 All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](https://semver.org/).
 
+## [Unreleased] - ReleaseDate
+
+### Added
+### Changed
+### Fixed
+
+- Fixed soundness issues in `FdSet::insert`, `FdSet::remove`, and
+  `FdSet::contains` involving file descriptors outside of the range
+  `0..FD_SETSIZE`.
+  (#[1575](https://github.com/nix-rust/nix/pull/1575))
+
+### Removed
+
 ## [0.23.0] - 2021-09-28
 ### Added
 

src/sys/select.rs

@@ -1,4 +1,5 @@
 //! Portably monitor a group of file descriptors for readiness.
+use std::convert::TryFrom;
 use std::iter::FusedIterator;
 use std::mem;
 use std::ops::Range;
@@ -17,6 +18,13 @@ pub use libc::FD_SETSIZE;
 #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
 pub struct FdSet(libc::fd_set);
 
+fn assert_fd_valid(fd: RawFd) {
+    assert!(
+        usize::try_from(fd).map_or(false, |fd| fd < FD_SETSIZE),
+        "fd must be in the range 0..FD_SETSIZE",
+    );
+}
+
 impl FdSet {
     /// Create an empty `FdSet`
     pub fn new() -> FdSet {
@@ -29,16 +37,19 @@ impl FdSet {
 
     /// Add a file descriptor to an `FdSet`
     pub fn insert(&mut self, fd: RawFd) {
+        assert_fd_valid(fd);
         unsafe { libc::FD_SET(fd, &mut self.0) };
     }
 
     /// Remove a file descriptor from an `FdSet`
     pub fn remove(&mut self, fd: RawFd) {
+        assert_fd_valid(fd);
         unsafe { libc::FD_CLR(fd, &mut self.0) };
     }
 
     /// Test an `FdSet` for the presence of a certain file descriptor.
     pub fn contains(&self, fd: RawFd) -> bool {
+        assert_fd_valid(fd);
         unsafe { libc::FD_ISSET(fd, &self.0) }
     }
 

test/sys/test_select.rs

@@ -52,3 +52,31 @@ pub fn test_pselect_nfds2() {
     assert!(fd_set.contains(r1));
     assert!(!fd_set.contains(r2));
 }
+
+macro_rules! generate_fdset_bad_fd_tests {
+    ($fd:expr, $($method:ident),* $(,)?) => {
+        $(
+            #[test]
+            #[should_panic]
+            fn $method() {
+                FdSet::new().$method($fd);
+            }
+        )*
+    }
+}
+
+mod test_fdset_negative_fd {
+    use super::*;
+    generate_fdset_bad_fd_tests!(-1, insert, remove, contains);
+}
+
+mod test_fdset_too_large_fd {
+    use super::*;
+    use std::convert::TryInto;
+    generate_fdset_bad_fd_tests!(
+        FD_SETSIZE.try_into().unwrap(),
+        insert,
+        remove,
+        contains,
+    );
+}