Skip to content

Resolve file descriptors once per syscall - #1302

Open
Weiteng Chen (CvvT) wants to merge 3 commits into
mainfrom
fix/1261-stable-chunked-fd
Open

Resolve file descriptors once per syscall#1302
Weiteng Chen (CvvT) wants to merge 3 commits into
mainfrom
fix/1261-stable-chunked-fd

Conversation

@CvvT

Copy link
Copy Markdown
Contributor

Resolve raw file descriptors once and retain the resulting AnyTypedFd throughout each syscall. This prevents chunked operations from switching to a different open-file description when another task concurrently closes and reuses the raw descriptor.

This updates file, memory-mapping, and socket syscalls, and consolidates typed-fd dispatch and narrowing.

Fixes #1261.

Chunked syscalls re-resolved the caller's raw fd for every chunk, so a
concurrent close plus open could make a later chunk operate on a
different open file description that had reused the raw slot.

Resolve the raw fd once into an `AnyTypedFd` that retains the subsystem's
`Arc<TypedFd<_>>`, and thread that through the whole syscall. A racing
close now makes later chunks fail with `EBADF` instead of silently
following the reused slot. This covers `read`/`write`, the `iovec`
variants, the large-buffer `read` chunking, `sendfile`, `mmap`, all
socket syscalls, `fstat`/`faccessat`, and `dup`.

The fd layer is consolidated to support that:
- `AnyTypedFd::dispatch` replaces `FilesState::run_on_typed_fd`, with
  `as_fs`/`fs_only` and the `on_any_fd!` macro covering the cases that
  previously needed blocks of repeated no-op closures.
- fd narrowing happens once, in `Task::typed_fd`/`FilesState::typed_fd`;
  `FsPath` stores `i32` so callers stop round-tripping through `u32`.
- `sys_recvmmsg` and `sys_sendfile` take a single `files` borrow.
- `AnyTypedFd` carries `Debug`/`subsystem_name` for diagnostics.

Adds regression tests covering fd reuse under `readv`, `dup`, and socket
lookups.
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

🤖 SemverChecks 🤖 No breaking API changes detected

Note: this does not mean API is unchanged, or even that there are no breaking changes; simply, none of the detections triggered.

@CvvT
Weiteng Chen (CvvT) marked this pull request as ready for review September 5, 2026 00:54
@wdcui

Copy link
Copy Markdown
Member

Static review findings (5-min budget, dual-model review: GPT-5.6 Sol + Opus 5)

Reviewed the actual diff vs origin/main (litebox_shim_linux/src/lib.rs, syscalls/file.rs, syscalls/mm.rs, syscalls/net.rs).

🟡 Medium — mm.rs ELF-patch path re-resolves the raw fd instead of reusing the pre-resolved handle

Location: litebox_shim_linux/src/syscalls/mm.rs:136 and :147 (feeding into init_elf_patch_state at :548, :581)

do_mmap_file resolves the descriptor once (let typed_fd = self.typed_fd(fd)… at line 121) and correctly threads &typed_fd into try_cow_mmap_file/do_mmap_file_memcpy. However, the ELF-rewriting path is still handed the raw i32 fd:

  • maybe_patch_exec_segment(result, len, fd, …) (:136) → init_elf_patch_state(fd, …) (:768)
  • init_elf_patch_state(fd, …) (:147, direct call)

init_elf_patch_state then re-resolves the raw fd twice more via self.sys_read(fd, &mut ehdr_buf, Some(0)) (:548) and self.sys_read(fd, &mut phdrs_buf, Some(e_phoff)) (:581), each doing a fresh raw_descriptor_store lookup.

Since the fd table is shared across CLONE_FILES threads, a sibling thread can close/dup2 over fd between the initial resolution (:121) and these later reads. Two concrete failure modes within a single mmap call:

  1. sys_read returns EBADFinit_elf_patch_state silently skips patch-state init → maybe_patch_exec_segment treats it as "no patch needed" and reports the mapping successful but unpatched. For a pre-patched binary that requires the trampoline rewrite, this leads to a SIGSEGV on the first rewritten syscall instead of the intended fail path.
  2. The slot was replaced by another file → ELF header/phdrs are read from a different file than the one actually mapped, and elf_patch_cache (keyed by the raw i32) gets populated with segment offsets from the wrong file.

This doesn't appear to be a regression introduced by this PR, but the PR's stated invariant ("resolve each raw fd once per syscall") isn't fully established for mmap's ELF-patch path even though a resolved AnyTypedFd is already in scope — it reads as an incomplete migration.

Suggested fix: Change maybe_patch_exec_segment and init_elf_patch_state to take &AnyTypedFd<Platform> (keep the raw i32 only as the elf_patch_cache key) and replace the two self.sys_read(fd, …) calls with self.do_read(typed_fd, …), matching what's already done for do_mmap_file_memcpy.

Confidence: High — independently identified by both reviewers (GPT-5.6 Sol and Opus 5) reviewing this file/chunk separately.


Verified clean (no action needed)

The rest of the diff (file.rs, net.rs, and the resolver logic in lib.rs) came back clean from both independent reviewers, with matching verified invariants:

  • Every converted syscall resolves its fd exactly once per entry point (no re-resolution across chunked read/write, iovec, or *mmsg loops).
  • Dispatch ordering and error-code precedence (EBADF/ENOTSOCK before EINVAL/EFAULT) match the removed run_on_raw_fd/with_socket code paths.
  • No new use-after-free/refcount hazards: a concurrent close on a resolved Arc<TypedFd> fails closed (EBADF) rather than causing UAF or fd-confusion.
  • No new lock re-entrancy or deadlock risk from holding self.files.borrow() across nested resolutions.

@wdcui Weidong Cui (wdcui) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

One chunked syscall can consume multiple open-file descriptions after fd reuse

2 participants