Mark file descriptors close-on-exec at creation (fixes #887) - #888
Open
wharris623 wants to merge 6 commits into
Open
Mark file descriptors close-on-exec at creation (fixes #887)#888wharris623 wants to merge 6 commits into
wharris623 wants to merge 6 commits into
Conversation
Follow-up to nzbgetcom#886 (closed) / addresses nzbgetcom#887, per maintainer feedback: closing every inherited fd in a loop right before execvp() treats the symptom, not the cause, and is fragile (misses anything opened after the loop is written) and costly (has to iterate up to the process's fd ceiling on every single fork). The root problem is that descriptors are created inheritable in the first place. This instead marks each descriptor non-inheritable (FD_CLOEXEC) at the point it's created, matching the pattern the codebase already uses in a couple of places (the lock-file open() already sets O_CLOEXEC; the Windows side of Connection.cpp already clears HANDLE_FLAG_INHERIT on some sockets) - just applied consistently everywhere a descriptor is created on POSIX: - ScriptController::StartProcess(): both pipe() calls. The ends that become the child's stdin/stdout/stderr are unaffected, since dup2() never carries over FD_CLOEXEC from its source fd - this only stops the original pipe fd numbers from leaking into any *other* child forked while they're still open. - Connection.cpp: all 6 raw socket() calls and the accept() call, via a small SetCloseOnExec(SOCKET) helper mirroring the existing SetHandleInformation(..., HANDLE_FLAG_INHERIT, 0) calls already used for Windows in two of these spots. This covers both the WebUI/ control-port listening socket (explains "port still busy after restart" from the issue) and outbound/accepted connection sockets. - FileSystem.cpp: all 4 fopen() calls, via a SetCloseOnExec(FILE*) helper, most importantly DiskFile::Open() - the general-purpose file I/O class used throughout the app for downloads, par2, and unpacking, and the most likely actual source of the original EBUSY reports.
Directly checks the FD_CLOEXEC flag via fcntl(fd, F_GETFD) on a file opened through DiskFile::Open() - the general-purpose file I/O class used throughout the app, and the most likely source of the original EBUSY reports in nzbgetcom#887.
dnzbk
self-requested a review
August 12, 2026 05:40
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The original solution I worked through with Claude was to create a loop that closed every inherited file handle right before
execvp(). The maintainer wanted something that treated the root cause, not the symptom. We agree — the root problem is that the descriptors are created in the inheritable state in the first place, so the fix is to make them non-inheritable at creation instead.In
Connection.cpp, this exact problem was already accounted for on Windows (SetHandleInformation(..., HANDLE_FLAG_INHERIT, 0)) — we just needed to extend the same treatment to POSIX.nzbget.cpp's lock-fileopen()already usedO_CLOEXECtoo. So this was already being addressed in a couple of places, just not consistently. We engineered a small helper (SetCloseOnExec) and applied it to every rawsocket()/accept()call inConnection.cpp, and a second helper for the 4fopen()calls inFileSystem.cpp, most importantlyDiskFile::Open(). InScriptController.cpp, we replaced the old close-everything loop withfcntl(fd, F_SETFD, FD_CLOEXEC)called right after each of the twopipe()calls. We scoped this to POSIX only — no Windows changes.Addresses #887.
Util::SetCloseOnExec() replaces file-local helpers
Sockets handled in InitSocketOpts() (including the listening socket via Bind())
Two regression tests: flag check + fork/exec file-deletion behavior
AI assistance
Claude did the code changes and testing. I reviewed and directed it — Claude has the technical skill, I have the people guidance, judgment on what makes sense, and troubleshooting direction.
Lib changes
N/A — no vendored libraries changed.
Testing
We did full compiles of the source both with and without our changes, and both passed all 10 test groups. We then added a new test case (within the existing
UtilTestgroup) that checksFD_CLOEXECdirectly viafcntl(fd, F_GETFD)on a file opened throughDiskFile::Open(). For live verification, we built and ran the actual compiled patched and baseline binaries as real daemons, then usedgdbto query the real listening socket'sFD_CLOEXECflag on each running process directly.