Conversation
Covers the working directory applying only to the child, a missing working directory failing Start, a redirected stdin/stdout/stderr round trip, redirected pipes not leaking into other spawned processes, only redirected streams being attachable (and only once), and Close releasing a stdin pipe the caller never took. Several of these fail against the current POSIX BfpSpawn implementation; the following commits fix them.
BfpSpawn_Create called chdir(workingDir) in the parent before forking and tried to restore the previous directory afterwards, but getcwd was called after the chdir so the 'restore' was a no-op. Every spawn with a working directory permanently moved the parent's cwd, and even a correct restore would race with other threads resolving relative paths. Validate the directory up front in the parent (preserving the synchronous error result) and perform the chdir in the child before exec, matching the Windows behavior.
After dup2'ing the stderr pipe onto STDERR_FILENO the child closed stdErrFD[0] (already closed) instead of stdErrFD[1], leaking an extra write end of the pipe into the exec'd process.
If exec (or the working directory chdir) fails in the forked child, exit() would run the parent's atexit handlers and flush stdio buffers inherited from the parent, duplicating any pending parent output. Use _exit instead.
The stdio redirection pipes were created without close-on-exec, so any process spawned concurrently (or later, for the long-lived stdin write end) inherited them. A child whose stdin write end leaked into another process would never see EOF when the parent closed its stdin, and stdout and stderr readers could likewise block until the unrelated process exited. Create the pipes close-on-exec (pipe2 where available, fcntl otherwise). dup2 clears the flag on the redirected std handle; the case where the pipe end already is the target handle is handled explicitly.
The forked child reported chdir/exec failures with fprintf, which is not async-signal-safe and may deadlock after fork in a multithreaded parent. The exec failure message was also never seen, because stdout, stderr and stdin were closed just before printing it. Write the messages directly to STDERR_FILENO instead and drop the redundant closes (_exit closes them).
BfpSpawn used 0 to mean 'no pipe' for streams that weren't redirected or whose handle was already taken. On POSIX fd 0 is the parent's stdin, so BfpSpawn_GetStdHandles wrapped it in a BfpFile and releasing that file closed the parent's stdin; the next open or pipe then reused fd 0. Use -1 for 'no pipe' and return NULL from BfpSpawn_GetStdHandles in that case, which the existing callers already check for.
Pipe ends that were redirected but never retrieved through BfpSpawn_GetStdHandles were never closed. Besides leaking the fds, a leaked stdin write end left the child blocked waiting for input forever. Close them on release, matching what ~BfpSpawn already does on Windows.
If creating one of the redirection pipes or the fork failed, the pipes already created and the strdup'd argv strings were leaked. Close any created pipe ends and free argv before returning the error.
Drop the unused shell-execute verb (still stripping the |verb suffix from the target path), a redundant copy of the target path, an unused argv variable, the abandoned commented-out posix_spawn call, and a commented-out printf that referenced its no-longer-existing status variable.
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
1 participant
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.
This fixes bugs in BfpSpawn on Linux, macOS and Android, the code behind SpawnedProcess. Windows isn't changed.
The first commit adds tests to corlib (SpawnedProcessTests, non-Windows only). Four of them fail on master, and each fix commit makes its own test pass, so you can follow the series commit by commit. All 54 corlib tests pass at the end.
Some additional notes: