unix: use async-signal-safe functions between fork and exec#1167
unix: use async-signal-safe functions between fork and exec#1167jBarz wants to merge 2 commits intolibuv:v1.xfrom
Conversation
|
|
||
| execvp(options->file, options->args); | ||
| execve(options->file, options->args, | ||
| options->env != NULL ? options->env : environ); |
There was a problem hiding this comment.
execve() doesn't search the PATH like execvp() does. I realize it may be more POSIX-compliant but it would break a lot of existing code for mostly academic reasons. We rely on the platform's libc to implement execvp() properly and they all do.
The other change looks fine to me.
|
For reference: #998 |
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
LGTM
For reference: #998
Hah, completely forgot I'd filed that.
Thinking about it again, I don't know if simulating execvp() by calling getenv("PATH") before forking is really better - it's not MT-safe. After the fork you can at least be sure you are the only thread left.
(On the other hand, if another thread was halfway done updating an environment variable, the environment can be in an inconsistent state. Threads and fork, it's all terrible.)
|
I'm fine either way :-) We can probably close it after this lands with a backreference? |
|
One more CI run: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/198/ as the previous one does not exist anymore. |
Refs: #998 PR-URL: #1167 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
|
Landed in 4499336, cheers! |
4 participants
From the manual for fork
So we should force uv__process_child_init to use
I tested this on linux x86 and z/OS. Other posix compliant operating systems should also work.