From df8e2deddcdcc5621ea194e20cd3a5858d1e0aae Mon Sep 17 00:00:00 2001
From: 4lDO2 <4lDO2@protonmail.com>
Date: Thu, 7 Jul 2022 12:20:44 +0200
Subject: [PATCH] Fix file descriptor leak in fork().
---
src/platform/redox/clone.rs | 55 +++++++++++++++++++++++++------------
src/platform/redox/exec.rs | 3 --
2 files changed, 38 insertions(+), 20 deletions(-)
diff --git a/src/platform/redox/clone.rs b/src/platform/redox/clone.rs
index 8877ef91..398541bf 100644
--- a/src/platform/redox/clone.rs
+++ b/src/platform/redox/clone.rs
@@ -119,9 +119,11 @@ pub fn fork_impl() -> Result<usize> {
}
fn fork_inner(initial_rsp: *mut usize) -> Result<usize> {
- let new_pid = {
+ let (cur_filetable_fd, new_pid_fd, new_pid);
+
+ {
let cur_pid_fd = FdGuard::new(syscall::open("thisproc:current/open_via_dup", O_CLOEXEC)?);
- let (new_pid_fd, new_pid) = new_context()?;
+ (new_pid_fd, new_pid) = new_context()?;
// Do not allocate new signal stack, but copy existing address (all memory will be re-mapped
// CoW later).
@@ -138,6 +140,18 @@ fn fork_inner(initial_rsp: *mut usize) -> Result<usize> {
copy_str(*cur_pid_fd, *new_pid_fd, "name")?;
copy_str(*cur_pid_fd, *new_pid_fd, "cwd")?;
+ // Copy existing files into new file table, but do not reuse the same file table (i.e. new
+ // parent FDs will not show up for the child).
+ {
+ cur_filetable_fd = FdGuard::new(syscall::dup(*cur_pid_fd, b"filetable")?);
+
+ // This must be done before the address space is copied.
+ unsafe {
+ initial_rsp.write(*cur_filetable_fd);
+ initial_rsp.add(1).write(*new_pid_fd);
+ }
+ }
+
// CoW-duplicate address space.
{
let cur_addr_space_fd = FdGuard::new(syscall::dup(*cur_pid_fd, b"addrspace")?);
@@ -190,22 +204,18 @@ fn fork_inner(initial_rsp: *mut usize) -> Result<usize> {
let buf = create_set_addr_space_buf(*new_addr_space_fd, fork_ret as usize, initial_rsp as usize);
let _ = syscall::write(*new_addr_space_sel_fd, &buf)?;
}
-
- // Copy existing files into new file table, but do not reuse the same file table (i.e. new
- // parent FDs will not show up for the child).
- {
- let cur_filetable_fd = FdGuard::new(syscall::dup(*cur_pid_fd, b"filetable")?);
- // TODO: Use cross_scheme_links or something similar to avoid copying the file table in the
- // kernel.
- let new_filetable_fd = FdGuard::new(syscall::dup(*cur_filetable_fd, b"copy")?);
- let new_filetable_sel_fd = FdGuard::new(syscall::dup(*new_pid_fd, b"current-filetable")?);
-
- let _ = syscall::write(*new_filetable_sel_fd, &usize::to_ne_bytes(*new_filetable_fd));
- }
copy_float_env_regs(*cur_pid_fd, *new_pid_fd)?;
-
- new_pid
- };
+ }
+ // Copy the file table. We do this last to ensure that all previously used file descriptors are
+ // closed. The only exception -- the filetable selection fd and the current filetable fd --
+ // will be closed by the child process.
+ {
+ // TODO: Use cross_scheme_links or something similar to avoid copying the file table in the
+ // kernel.
+ let new_filetable_fd = FdGuard::new(syscall::dup(*cur_filetable_fd, b"copy")?);
+ let new_filetable_sel_fd = FdGuard::new(syscall::dup(*new_pid_fd, b"current-filetable")?);
+ let _ = syscall::write(*new_filetable_sel_fd, &usize::to_ne_bytes(*new_filetable_fd));
+ }
// Unblock context.
syscall::kill(new_pid, SIGCONT)?;
@@ -216,6 +226,11 @@ fn fork_inner(initial_rsp: *mut usize) -> Result<usize> {
unsafe extern "sysv64" fn __relibc_internal_fork_impl(initial_rsp: *mut usize) -> usize {
Error::mux(fork_inner(initial_rsp))
}
+#[no_mangle]
+unsafe extern "sysv64" fn __relibc_internal_fork_hook(cur_filetable_fd: usize, new_pid_fd: usize) {
+ let _ = syscall::close(cur_filetable_fd);
+ let _ = syscall::close(new_pid_fd);
+}
#[no_mangle]
core::arch::global_asm!("
@@ -233,13 +248,19 @@ fork_wrapper:
push r14
push r15
+ sub rsp, 16
+
mov rdi, rsp
call __relibc_internal_fork_impl
jmp 2f
fork_ret:
+ mov rdi, [rsp]
+ mov rsi, [rsp + 8]
+ call __relibc_internal_fork_hook
xor rax, rax
2:
+ add rsp, 16
pop r15
pop r14
pop r13
diff --git a/src/platform/redox/exec.rs b/src/platform/redox/exec.rs
index 77c03b7c..bdda06da 100644
--- a/src/platform/redox/exec.rs
+++ b/src/platform/redox/exec.rs
@@ -88,9 +88,6 @@ fn fexec_impl_inner(file: File, path: &[u8], args: &[&[u8]], envs: &[&[u8]], arg
// some misalignments, and then execute the SYS_EXEC syscall to replace the program memory
// entirely.
- // TODO: Introduce RAII guards to all owned allocations so that no leaks occur in case of
- // errors.
-
let mut header_bytes = [0_u8; core::mem::size_of::<Header>()];
read_all(fd, Some(0), &mut header_bytes)?;
let header = Header::from_bytes(&header_bytes);
--
GitLab