diff --git a/litebox_shim_linux/src/lib.rs b/litebox_shim_linux/src/lib.rs index eb20756954..6a9d9977d8 100644 --- a/litebox_shim_linux/src/lib.rs +++ b/litebox_shim_linux/src/lib.rs @@ -263,7 +263,7 @@ impl LinuxShim { } = task; let files = syscalls::file::FilesState::new(fs); - files.set_max_fd(syscalls::process::RLIMIT_NOFILE_CUR - 1); + files.set_max_fd(syscalls::process::RLIMIT_NOFILE_CUR); let files = Arc::new(files); let credentials = Arc::new(syscalls::process::Credentials { uid, diff --git a/litebox_shim_linux/src/syscalls/file.rs b/litebox_shim_linux/src/syscalls/file.rs index af9a5e85a3..9599373955 100644 --- a/litebox_shim_linux/src/syscalls/file.rs +++ b/litebox_shim_linux/src/syscalls/file.rs @@ -91,6 +91,7 @@ pub(crate) struct FilesState { pub(crate) fs: alloc::sync::Arc>, pub(crate) raw_descriptor_store: litebox::sync::RwLock, + /// Exclusive upper bound for raw file descriptor values. max_fd: AtomicUsize, } @@ -124,14 +125,44 @@ impl FilesState { rds: &mut litebox::fd::RawDescriptorStorage, typed_fd: TypedFd, ) -> Result> { - // XXX(jb): should we try to somehow enforce that it is set at the smallest - // available/unassigned FD number? - let raw_fd = rds.fd_into_raw_integer(typed_fd); let max_fd = self.max_fd.load(Ordering::Relaxed); - if raw_fd > max_fd { - let orig = rds.fd_consume_raw_integer::(raw_fd).unwrap(); - return Err(alloc::sync::Arc::into_inner(orig).unwrap()); + self.insert_raw_fd_at_or_above_locked(rds, typed_fd, 0, max_fd) + } + + fn insert_raw_fd_at_or_above( + &self, + typed_fd: TypedFd, + min_fd: usize, + max_fd: usize, + ) -> Result> { + let mut rds = self.raw_descriptor_store.write(); + self.insert_raw_fd_at_or_above_locked(&mut rds, typed_fd, min_fd, max_fd) + } + + fn insert_raw_fd_at_or_above_locked( + &self, + rds: &mut litebox::fd::RawDescriptorStorage, + typed_fd: TypedFd, + min_fd: usize, + max_fd: usize, + ) -> Result> { + if min_fd >= max_fd { + return Err(typed_fd); } + + // XXX: Can clean+speed this up by exposing a new method at RawDescriptorStorage + let mut raw_fd = min_fd; + for occupied_raw_fd in rds.iter_alive().skip_while(|&fd| fd < min_fd) { + if occupied_raw_fd != raw_fd { + break; + } + raw_fd += 1; + } + if raw_fd >= max_fd { + return Err(typed_fd); + } + let success = rds.fd_into_specific_raw_integer(typed_fd, raw_fd); + assert!(success); Ok(raw_fd) } } @@ -779,6 +810,15 @@ impl Task { self.do_close_and_replace::>(raw_fd, None) } + fn remove_and_drop_descriptor(&self, fd: &TypedFd) { + let entry = { + let mut dt = self.global.litebox.descriptor_table_mut(); + dt.remove(fd) + }; + // do not hold any locks while dropping the entry + drop(entry); + } + /// Close the file at `raw_fd` and optionally place a new file in the same slot. /// /// This function ensure `close` and `insert` are done atomically. @@ -856,30 +896,15 @@ impl Task { ConsumedFd::Network(fd) => self.global.close_socket(&self.wait_cx(), fd), ConsumedFd::Pipes(fd) => self.global.close_linux_pipe(&fd), ConsumedFd::Eventfd(fd) => { - let entry = { - let mut dt = self.global.litebox.descriptor_table_mut(); - dt.remove(&fd) - }; - // do not hold any locks while dropping the entry - drop(entry); + self.remove_and_drop_descriptor(&fd); Ok(()) } ConsumedFd::Epoll(fd) => { - let entry = { - let mut dt = self.global.litebox.descriptor_table_mut(); - dt.remove(&fd) - }; - // do not hold any locks while dropping the entry - drop(entry); + self.remove_and_drop_descriptor(&fd); Ok(()) } ConsumedFd::Unix(fd) => { - let entry = { - let mut dt = self.global.litebox.descriptor_table_mut(); - dt.remove(&fd) - }; - // do not hold any locks while dropping the entry - drop(entry); + self.remove_and_drop_descriptor(&fd); Ok(()) } } @@ -2434,17 +2459,17 @@ impl Task { fd: &TypedFd, close_on_exec: bool, target: DupFdRequest, + close_typed_fd: impl FnOnce(TypedFd), ) -> Result { - let max_fd = task - .process() - .limits - .get_rlimit_cur(litebox_common_linux::RlimitResource::NOFILE); + let max_fd = files.max_fd.load(Ordering::Relaxed); match target { - DupFdRequest::Exact(target) if target >= max_fd => { + DupFdRequest::Exact(target) | DupFdRequest::LowestAtOrAbove(target) + if target >= max_fd => + { return Err(DupFdError::TargetFdExceedsLimit); } - DupFdRequest::LowestAtOrAbove(min_fd) if min_fd >= max_fd => { - return Err(DupFdError::TargetFdExceedsLimit); + DupFdRequest::LowestAvailable if max_fd == 0 => { + return Err(DupFdError::TooManyFiles); } _ => {} } @@ -2463,27 +2488,24 @@ impl Task { target } DupFdRequest::LowestAvailable => { - let rds = &mut *files.raw_descriptor_store.write(); - rds.fd_into_raw_integer(fd) + match files.insert_raw_fd_at_or_above(fd, 0, max_fd) { + Ok(fd) => fd, + Err(fd) => { + close_typed_fd(fd); + return Err(DupFdError::TooManyFiles); + } + } } DupFdRequest::LowestAtOrAbove(min_fd) => { - let rds = &mut *files.raw_descriptor_store.write(); - let mut raw_fd = min_fd; - for occupied_raw_fd in rds.iter_alive().skip_while(|&fd| fd < min_fd) { - if occupied_raw_fd != raw_fd { - break; + match files.insert_raw_fd_at_or_above(fd, min_fd, max_fd) { + Ok(fd) => fd, + Err(fd) => { + close_typed_fd(fd); + return Err(DupFdError::TooManyFiles); } - raw_fd += 1; } - let success = rds.fd_into_specific_raw_integer(fd, raw_fd); - assert!(success); - raw_fd } }; - if new_fd >= max_fd { - let _ = task.do_close(new_fd); - return Err(DupFdError::TooManyFiles); - } Ok(new_fd) } @@ -2492,12 +2514,38 @@ impl Task { files .run_on_raw_fd( file, - |fd| dup(self, &files, fd, close_on_exec, target), - |fd| dup(self, &files, fd, close_on_exec, target), - |fd| dup(self, &files, fd, close_on_exec, target), - |fd| dup(self, &files, fd, close_on_exec, target), - |fd| dup(self, &files, fd, close_on_exec, target), - |fd| dup(self, &files, fd, close_on_exec, target), + |fd| { + dup(self, &files, fd, close_on_exec, target, |fd| { + let _ = files.fs.close(&fd); + }) + }, + |fd| { + dup(self, &files, fd, close_on_exec, target, |fd| { + let _ = self + .global + .close_socket(&self.wait_cx(), alloc::sync::Arc::new(fd)); + }) + }, + |fd| { + dup(self, &files, fd, close_on_exec, target, |fd| { + let _ = self.global.close_linux_pipe(&fd); + }) + }, + |fd| { + dup(self, &files, fd, close_on_exec, target, |fd| { + self.remove_and_drop_descriptor(&fd); + }) + }, + |fd| { + dup(self, &files, fd, close_on_exec, target, |fd| { + self.remove_and_drop_descriptor(&fd); + }) + }, + |fd| { + dup(self, &files, fd, close_on_exec, target, |fd| { + self.remove_and_drop_descriptor(&fd); + }) + }, ) .map_err(|_| DupFdError::BadFd)? } diff --git a/litebox_shim_linux/src/syscalls/process.rs b/litebox_shim_linux/src/syscalls/process.rs index 9d473eedd9..573109007c 100644 --- a/litebox_shim_linux/src/syscalls/process.rs +++ b/litebox_shim_linux/src/syscalls/process.rs @@ -820,8 +820,7 @@ impl Task { } match resource { litebox_common_linux::RlimitResource::NOFILE => { - let new_max_fd = new_limit.rlim_cur.saturating_sub(1); - self.files.borrow().set_max_fd(new_max_fd); + self.files.borrow().set_max_fd(new_limit.rlim_cur); } _ => unimplemented!("Unsupported resource for set_rlimit: {:?}", resource), } diff --git a/litebox_shim_linux/src/syscalls/tests.rs b/litebox_shim_linux/src/syscalls/tests.rs index 2691742812..9740c66a9f 100644 --- a/litebox_shim_linux/src/syscalls/tests.rs +++ b/litebox_shim_linux/src/syscalls/tests.rs @@ -187,7 +187,7 @@ fn test_fcntl() { #[test] fn test_pipe2_race_with_concurrent_close() { let task = init_platform(None); - task.files.borrow().set_max_fd(3); + task.files.borrow().set_max_fd(4); let stop = alloc::sync::Arc::new(core::sync::atomic::AtomicBool::new(false)); let stop_closer = stop.clone();