util: report back child errors to parent and throw

[ViniciusCestarii]

Report execvp() and close() failures to the parent via a socket and throw from SpawnProcess.

Previously the parent didn't have a way of knowing whether the child process successfully execute or not without polling for it or waiting for it.

Now SpawnProcess waits for the exec() to happen before returning and before throwing it also reaps the child, preventing zombie process.

Also make post-fork child not rely on on perror(), which is not async-signal-safe.

This addresses the TODO on commit 69652f0

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	xyzconstant
Stale ACK	ryanofsky

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #231 (Add windows support by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[ryanofsky]

Concept ACK dee4383. I plan to review more and left a suggestion below.

I think PR description might be underselling the change, because it describes the main advantage as avoiding a perror call that is not signal safe (so could hang). But I think a bigger improvement is translating the execvp failure in the child into an exception in the parent. Previously the parent didn't have a way of knowing whether the child process successfully execute or not without polling for it or waiting for it.

[ViniciusCestarii]

Thanks for reviewing @ryanofsky! I updated the PR description to sell this PR more and I will address your comment.

[xyzconstant]

Concept ACK dee4383

Left some comments about descriptor leaks on the error paths. Planning to review more once you update on ryanofsky's feedback.

[Sjors]

Note that this change happens very close to the code touched by #311, but does seem orthogonal at first glance.

[ViniciusCestarii]

Thanks @xyzconstant for reviewing! I have force-pushed d417706 and pushed a761aca and updated PR title and description.

I have addresed:

• @xyzconstant comment about leaking fds and throwing wrong errno and @ryanofsky comment for reporting a struct ChildError to cover execv and close errors on d417706.
• @xyzconstant comment that this PR could benefit with a function for reading the error from child so I added ReadSpawnResult on a761aca and also renamed ChildError to SpawnError to cover the parent error on reading from socket.

I believe this now conflicts with #311 but it should be a simple rebase for whichever PR merges second.

I also noticed that there are several throws after fork that could benefit from reaping the child to prevent zombie process, but I believe this could be a follow up.

[ryanofsky]

Code review ACK a761aca. Nice changes that should substantially improve error reporting.

I left some suggestions below but they are not important, this should already be an improvement over current code. Also I think #311 will be merged first and this will need to be rebased, but the two pr's should complement each other and not conflict very much.

Would note that even with #311 there is a still a race condition since CLOEXEC flag is not applied right away and even with this PR there are still errors not handled like connect_info_to_args or KJ_SYSCALL syscall throwing which could cause leaks, so more improvements could be made as followups.

The posix code is definitely getting more complicated as a result of these error handling improvements (especially compared to the windows code) but it seems like there might not be a way to avoid this with current Bitcoin core code which is not applying CLOEXEC flags, so file descriptors will leak into child processes. This prevents using posix_spawn instead of fork/exec.

[ViniciusCestarii]

Thanks again for the reviews! Forced-push eecda07 squashing into a single commit and basing these changes on #311. I also addressed @ryanofsky comments.

[ryanofsky]

Code review ACK eecda07. Left minor suggests below but this looks good as-is.

Might want to note in commit messages or PR description that this does change SpawnProcess behavior a little bit beyond error reporting since it now waits for the exec() to happen before returning instead of just the fork() so it might be a little slower (not that it should matter in practice)

[ryanofsky]

In commit "util: report back child error to parent and throw" (eecda07)

In the places there this function is throwing after calling fork() and it returns a child pid (lines 248, 289, and 296) it looks like this code will leak the pid and leave behind a zombie process. I think it would make sense to call waitpid these places, with the pid and maybe with NOHANG.

[ViniciusCestarii]

Good catch. Fixed it in a separate commit: 3f05b11

I went with SIGKILL followed by a blocking wait rather than NOHANG because at some places the child may still be alive and racing toward exec, so NOHANG would usually return 0 and leave the zombie behind. After a SIGKILL the wait is guaranteed to reap and returns immediately.

[ryanofsky]

In commit "util: report back child error to parent and throw" (eecda07)

Would be good to note that an EOF can also happen if the child is killed before running exec, and this will return success in that case. There isn't really a good way for this function to avoid this but callers should see the failure when they call WaitProcess

[ViniciusCestarii]

Nice catch. I added a note in 4a56c18

[ViniciusCestarii]

Thanks for the review! Forced push 3f05b11 addressing #312 (comment) adding a note on function ReadSpawnResult and #312 (comment) by adding the function KillAndReapChild and calling it before throwing.

Updated PR description with: SpawnProcess waits for the exec() to happen before returning and before throwing it also reaps the child, preventing zombie process.