fix(fd): tighten FD/CLOEXEC lifetime across accept/pipe/compression#20
fix(fd): tighten FD/CLOEXEC lifetime across accept/pipe/compression#20andypost wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements measures to prevent file descriptor leaks by ensuring the CLOEXEC flag is set on sockets and pipes across various paths, including the addition of pipe2() support and SOCK_CLOEXEC for accept4(). It also addresses resource leaks in the static file compression path by closing file descriptors during mmap failures. Feedback identifies that the leak protection in the compression logic is incomplete, as several other error paths still leak the source file descriptor, and the practice of nullifying the file pointer on failure introduces an inconsistency that could affect the caller's error handling.
| nxt_file_close(task, *f); | ||
| *f = NULL; |
There was a problem hiding this comment.
The fix for the source file descriptor leak (V11) is incomplete and introduces an API inconsistency.
- Incomplete Leak Fix: While
*fis now closed onmmapfailures, it is still leaked on other error paths in this function, such asftruncatefailures (lines 273 and 328) and compression failure (line 313). Since the function takes ownership of the file descriptor (as evidenced by it being closed on success at line 336), it should be closed on all error paths to prevent resource exhaustion. - Inconsistent Pointer Handling: Setting
*f = NULLon failure (lines 285 and 296) is inconsistent with the success path where the pointer*fremains valid while the underlying struct is updated (**f = tfile). This inconsistency may lead to null pointer dereferences in the caller if it attempts to access the file object after a failure, especially if it expects to fall back to uncompressed serving or perform its own cleanup.
Audit-driven pass for V11/V14 in security-audit.md; seven findings, no protocol or config-surface changes. V14 [High] Accepted sockets missing CLOEXEC. nxt_conn_accept.c, nxt_kqueue_engine.c: set FD_CLOEXEC on the fd returned by plain accept() before handing it to the connection layer so it cannot leak into spawned application processes. nxt_epoll_engine.c: pass SOCK_CLOEXEC alongside SOCK_NONBLOCK to accept4(). V14 [High] Pipe FD leak on nxt_fd_nonblocking() failure. nxt_file.c: nxt_pipe_create() now closes both ends via nxt_pipe_close() if either nxt_fd_nonblocking() call fails, instead of leaking the other end. V14 [Medium] Pipe FDs lack CLOEXEC. nxt_file.c: prefer pipe2(pp, O_CLOEXEC) where the kernel supports it, with a fcntl(F_SETFD, FD_CLOEXEC) fallback on the legacy pipe() path. auto/files adds an NXT_HAVE_PIPE2 capability probe modeled on the existing NXT_HAVE_ACCEPT4 detect. V14 [Medium] accept4() ENOSYS detection narrowed. nxt_epoll_engine.c: the capability probe falls back only when the kernel actually returns ENOSYS; any other errno (e.g. EBADF, the expected outcome on a -1 fd) is treated as "supported". Behaviour unchanged on Linux; the comment now explains why. V14 [Medium] nxt_file_redirect close transient. Skipped: nxt_file.c:638 already alerts and returns NXT_ERROR on close() failure after a successful dup2(), and the caller still owns the descriptor's lifetime; no safe behaviour change is available. V14 [Low] socketpair SO_PASSCRED logging. Skipped: nxt_socketpair.c already emits a dedicated "failed to set SO_PASSCRED" alert before falling through to the cleanup-close logging, so operators can already distinguish credential-passing failures from generic socket teardown. V11 [High] Compression mmap FD leak. nxt_http_compression.c: on either mmap-failure return path, nxt_http_comp_compress_static_response() now closes the source file via *f and sets *f = NULL so the caller's fail-label cleanup does not double-close. Also adds nxt_alert() lines for the failures so operators have visibility into FD/mem pressure. CHANGES: add a single 1.35.4 bullet covering the seven findings. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
andypost
force-pushed
the
claude/audit-pr-e-fd-cloexec
branch
from
0972892 to
d3cdb07
Compare
|
Gemini's [high] finding addressed in |
1 participant
Summary
Audit-driven FD/CLOEXEC lifetime pass (PR-E from security-audit.md
/ PR #10). Seven findings; small, no protocol or config-surface
changes.
Findings addressed
Test plan
./configure --opensslsucceeds;NXT_HAVE_PIPE2lands inbuild/include/nxt_auto_config.halongsideNXT_HAVE_ACCEPT4.make -j unitdclean (build sandbox blocked compile in this run; reviewer to confirm locally).pytest-3 test/test_static.py test/test_http_compression.py(compression mmap path).pytest-3 test/test_routing.py(accept path)./proc/<pid>/fd/for any leaked listener/client/pipe descriptors.Upstream
Same fixes apply to
freeunitorg/freeunit; will forward after merge.Generated by Claude Code