From 60d9c1fcafdc8854794cd4759cae12334ba5ce68 Mon Sep 17 00:00:00 2001 From: Ryan Breen Date: Mon, 15 Dec 2025 06:34:50 -0500 Subject: [PATCH] Fix pipe ref counts and UDP socket unbind during dup/clone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs fixed: 1. Pipe reference counts not updated during dup/clone operations: - dup(), dup2(), and FdTable::clone() were cloning the Arc> but not incrementing the readers/writers counts inside PipeBuffer - This caused premature EOF (writers=0) or broken pipe (readers=0) errors when a duplicated fd was closed - Added add_reader() and add_writer() methods to PipeBuffer - Updated dup(), dup2(), and clone() to properly maintain ref counts 2. UDP socket unbinds prematurely when fd is closed: - sys_close explicitly called unbind_udp() when closing a socket fd - With Arc> design, multiple fds can share a socket - Closing one fd would unbind the socket while other fds still referenced it - Removed explicit unbind since UdpSocket::Drop already handles it when the last Arc reference is released 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- kernel/src/ipc/fd.rs | 46 ++++++++++++++++++++++++++++++++++---- kernel/src/ipc/pipe.rs | 10 +++++++++ kernel/src/syscall/pipe.rs | 14 +++++------- 3 files changed, 57 insertions(+), 13 deletions(-) diff --git a/kernel/src/ipc/fd.rs b/kernel/src/ipc/fd.rs index ef3d3f7..ce44393 100644 --- a/kernel/src/ipc/fd.rs +++ b/kernel/src/ipc/fd.rs @@ -102,9 +102,20 @@ impl Default for FdTable { impl Clone for FdTable { fn clone(&self) -> Self { - FdTable { - fds: alloc::boxed::Box::new((*self.fds).clone()), + let cloned_fds = alloc::boxed::Box::new((*self.fds).clone()); + + // Increment pipe reference counts for all cloned pipe fds + for fd_opt in cloned_fds.iter() { + if let Some(fd_entry) = fd_opt { + match &fd_entry.kind { + FdKind::PipeRead(buffer) => buffer.lock().add_reader(), + FdKind::PipeWrite(buffer) => buffer.lock().add_writer(), + _ => {} + } + } } + + FdTable { fds: cloned_fds } } } @@ -179,8 +190,21 @@ impl FdTable { let fd_entry = self.fds[old_fd as usize].clone().ok_or(9)?; - // Close new_fd if it's open (silently ignore errors) - let _ = self.close(new_fd); + // If new_fd is open, close it and decrement pipe ref counts + if let Some(old_entry) = self.fds[new_fd as usize].take() { + match old_entry.kind { + FdKind::PipeRead(buffer) => buffer.lock().close_read(), + FdKind::PipeWrite(buffer) => buffer.lock().close_write(), + _ => {} + } + } + + // Increment pipe reference counts for the duplicated fd + match &fd_entry.kind { + FdKind::PipeRead(buffer) => buffer.lock().add_reader(), + FdKind::PipeWrite(buffer) => buffer.lock().add_writer(), + _ => {} + } self.fds[new_fd as usize] = Some(fd_entry); Ok(new_fd) @@ -196,6 +220,13 @@ impl FdTable { let fd_entry = self.fds[old_fd as usize].clone().ok_or(9)?; + // Increment pipe reference counts for the duplicated fd + match &fd_entry.kind { + FdKind::PipeRead(buffer) => buffer.lock().add_reader(), + FdKind::PipeWrite(buffer) => buffer.lock().add_writer(), + _ => {} + } + // Find lowest available slot for i in 0..MAX_FDS { if self.fds[i].is_none() { @@ -203,6 +234,13 @@ impl FdTable { return Ok(i as i32); } } + + // No slot found - need to decrement the counts we just added + match &fd_entry.kind { + FdKind::PipeRead(buffer) => buffer.lock().close_read(), + FdKind::PipeWrite(buffer) => buffer.lock().close_write(), + _ => {} + } Err(24) // EMFILE } } diff --git a/kernel/src/ipc/pipe.rs b/kernel/src/ipc/pipe.rs index 98feb19..0388bbe 100644 --- a/kernel/src/ipc/pipe.rs +++ b/kernel/src/ipc/pipe.rs @@ -130,6 +130,16 @@ impl PipeBuffer { } } + /// Add a reader (used when duplicating pipe read fds) + pub fn add_reader(&mut self) { + self.readers += 1; + } + + /// Add a writer (used when duplicating pipe write fds) + pub fn add_writer(&mut self) { + self.writers += 1; + } + /// Get the number of bytes available to read #[allow(dead_code)] pub fn available(&self) -> usize { diff --git a/kernel/src/syscall/pipe.rs b/kernel/src/syscall/pipe.rs index de4a3c5..4a9c975 100644 --- a/kernel/src/syscall/pipe.rs +++ b/kernel/src/syscall/pipe.rs @@ -148,15 +148,11 @@ pub fn sys_close(fd: i32) -> SyscallResult { FdKind::StdIo(_) => { log::debug!("sys_close: Closed stdio fd={}", fd); } - FdKind::UdpSocket(socket_ref) => { - // Unbind the socket if it was bound - let socket = socket_ref.lock(); - if let Some(port) = socket.local_port { - crate::socket::SOCKET_REGISTRY.unbind_udp(port); - log::debug!("sys_close: Closed UDP socket fd={}, unbound port {}", fd, port); - } else { - log::debug!("sys_close: Closed unbound UDP socket fd={}", fd); - } + FdKind::UdpSocket(_) => { + // Socket unbinding is handled by UdpSocket::Drop when the last + // Arc reference is released, allowing shared sockets (via dup/fork) + // to remain bound until all references are closed. + log::debug!("sys_close: Closed UDP socket fd={}", fd); } } SyscallResult::Ok(0)