Fix file descriptor leaks and unsafe exec in Async::Exec - #799
Open
MarkRose wants to merge 1 commit into
Open
Conversation
- Exec::closeStdin() closed stdin_fd but never reset it to -1, so the destructor would close it a second time, potentially closing an unrelated fd that had since been recycled by the OS. closeStdin() now sets stdin_fd to -1 after closing (and is a no-op if already closed). - Exec::run() leaked the stdin/stdout/stderr pipe file descriptors it had already opened whenever a later pipe() or fork() call failed (2, 4, or 6 leaked fds depending on which call failed). Added a small helper that closes a set of fds and call it on each failure path so already-opened pipes are released before returning. - Exec::run() did not validate the command line before use. With an empty command line, the child process would call execv() with a NULL path and later code would read args[0] on an empty vector (undefined behavior). run() now rejects an empty command line up front and returns false instead of proceeding. Co-Authored-By: Claude Opus 4.8 <[email protected]>
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
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.
destructor would close it a second time, potentially closing an
unrelated fd that had since been recycled by the OS. closeStdin() now
sets stdin_fd to -1 after closing (and is a no-op if already closed).
had already opened whenever a later pipe() or fork() call failed
(2, 4, or 6 leaked fds depending on which call failed). Added a small
helper that closes a set of fds and call it on each failure path so
already-opened pipes are released before returning.
empty command line, the child process would call execv() with a NULL
path and later code would read args[0] on an empty vector (undefined
behavior). run() now rejects an empty command line up front and
returns false instead of proceeding.
Co-Authored-By: Claude Opus 4.8 [email protected]
This PR also adds a unit test (
AsyncExecTest.cpp). It is auto-discovered and executed by the CTest suite proposed in #762 once that is merged; without that suite present the test file is inert and does not affect the build.