bpo-36046: posix_spawn() doesn't support uid/gid #16384
Conversation
* subprocess.Popen now longer uses posix_spawn() if uid, gid or gids are set. * test_subprocess: add "nobody" and "nfsnobody" group names for test_group(). * test_subprocess: test_user() and test_group() are now also tested with close_fds=False.
Yhg1s
left a comment
There was a problem hiding this comment.
This change looks like the correct fix, and with proper tests for it.
I do wonder if the large conditional to check whether posix_spawn should be used should instead be pulled into the _posix_spawn method, with an appropriate comment explaining what is or isn't supported by posix_spawn, and perhaps why... but this fix should probably go in regardless of that.
Maybe, yes. But having tests in the caller avoids to pass the very long list of arguments. Especially, it avoids to pass arguments which are not supposed by posix_spawn() :-) _execute_child() has 21 arguments... is that really reasonable? Maybe we should put these stuff into a temporary object? |
|
Thanks for catching this! There is a lot of worthwhile refactoring that could be done in subprocess.py... Ideally we'd want to make it so that posix_spawn can only ever be used in very specific circumstances without a need to remember to add new negative conditionals when adding new non-spawn-available features. Using a temporary object rather than argument soup wouldn't change that. |
* subprocess.Popen now longer uses posix_spawn() if uid, gid or gids are set. * test_subprocess: add "nobody" and "nfsnobody" group names for test_group(). * test_subprocess: test_user() and test_group() are now also tested with close_fds=False.
are set.
test_group().
with close_fds=False.
https://bugs.python.org/issue36046