bug: `dup2_syscall` validates `oldfd` after the equality fast path

Open Beginner friendly
#1,374 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
88/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
c, rust

Research direction

Start in src/rawposix/src/fs_calls.rs at dup2_syscall around line 3154, then inspect process_tests/deterministic/conc_004_dup_close_fork_refcounts.c and skip_test_cases.txt. Run the referenced CONC-004 test to reproduce the invalid equal-descriptor case. Done means invalid oldfd values return EBADF without panicking, valid equal descriptors still succeed, and the test is removed from the skip list.

Written by the indexing model from the issue text.

Description

bug

Component: src/rawposix/src/fs_calls.rs:3154 (dup2_syscall)
Severity: POSIX conformance
Found by: CONC-004 (#1304), conc_004_dup_close_fork_refcounts.c:708

Symptom

dup2(badfd, badfd) reports success and returns an fd number that was never open.
The caller then treats a closed descriptor as valid.

Root cause
// src/rawposix/src/fs_calls.rs:3154
if old_vfd_arg > MAXFD as u64 || new_vfd_arg > MAXFD as u64 {
    return syscall_error(Errno::EBADF, "dup2", "Bad File Descriptor");
} else if old_vfd_arg == new_vfd_arg {
    // Does nothing
    return new_vfd_arg as i32;      // <-- oldfd never validated
}

POSIX: "If oldfd is not a valid file descriptor, then the call fails, and newfd is
not closed."
dup2() returns newfd unchanged only when oldfd is valid and
equal to it. The range check catches out-of-range numbers but not in-range-but-closed
ones.

There is a second defect in the same function: the get_specific_virtual_fd(...)
call on the success path ends in .unwrap(), panicking the host on failure.

Proposed fix

Move validation before the equality check, and replace the .unwrap() with EBADF:

     if old_vfd_arg > MAXFD as u64 || new_vfd_arg > MAXFD as u64 {
         return syscall_error(Errno::EBADF, "dup2", "Bad File Descriptor");
-    } else if old_vfd_arg == new_vfd_arg {
-        // Does nothing
+    }
+
+    // `oldfd` must be validated BEFORE the `oldfd == newfd` fast path.
+    let old_vfd = match fdtables::translate_virtual_fd(cageid, old_vfd_arg) {
+        Ok(entry) => entry,
+        Err(_e) => return syscall_error(Errno::EBADF, "dup2", "Bad File Descriptor"),
+    };
+
+    if old_vfd_arg == new_vfd_arg {
+        // oldfd is valid and equals newfd: no-op, newfd is NOT closed.
         return new_vfd_arg as i32;
     }

then the existing body, with .unwrap() replaced by a match returning EBADF.

Guest-visible change: dup2(badfd, badfd) now fails with EBADF; previously it
"succeeded".

Un-skip on merge

Remove process_tests/deterministic/conc_004_dup_close_fork_refcounts.c from
skip_test_cases.txt.

Dominant language
C
Stars
23
Forks
21
Avg merge
20h 6m
Merged PRs (30d)
5

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from Lind-Project/lind-wasm

All issues in Lind-Project/lind-wasm

Similar issues

More C issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.