gh-94518: [_posixsubprocess] Replace variable validity flags with reserved values

[arhadthedev]

Currently, _posixsubprocess.c uses group and user ids together with flags of whether the corrsponding variables were initialized. However such an implicit "either initialized or look somewhere else" confuses both a reader (another mental connection to constantly track between functions) and a compiler (warnings on potentially uninitialized variables). Instead, we can utilize a special group/user id as a flag value defined by POSIX but uses nowhere else. Namely:

• gid: call_setgid = False → gid = -1
• uid: call_setuid = False → uid = -1
• groups: call_setgroups = False → groups = NULL (obtained with (groups_list != Py_None) ? groups : NULL)

This PR is required for gh-94519.

---

PS.: @tiran I applied your suggestion from gh-94519 as this, separate PR to keep that one limited in scope and reviewable.

• Issue: Port 23-argument _posixsubprocess.fork_exec to Argument Clinic #94518

[arhadthedev]

This change has no user-visible effect so no NEWS entry is required.

[arhadthedev]

Please include a news entry.

We typically only skip news if the change is a trivial internal change or change is already covered by a previous PR. Non-trivial commits get a news entry so other core devs, release manager, distributors, and users have an easier way to locate a problem when a change introduces a regression.

@tiran Done (initially I've thought this change is invisible to users so skipped it).

[gpshead]

I like this PR :)

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[arhadthedev]

if (groups_size >= 0)

setgroups(0, NULL) is valid.

For some reason, setgroups(0, NULL) causes Doctest, Check if generated files are up to date, and SSL workflows to fail child processes with PermissionError: [Errno 1] Operation not permitted exception:

Stacktrace

Run ./python Lib/test/ssltests.py
Traceback (most recent call last):
OpenSSL 1.1.1n  15 Mar 2022
  File "/home/runner/work/cpython/cpython/Lib/test/ssltests.py", line 37, in <module>
    run_regrtests(*sys.argv[1:])
  File "/home/runner/work/cpython/cpython/Lib/test/ssltests.py", line 33, in run_regrtests
    result = subprocess.call(args)
             ^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/cpython/cpython/Lib/subprocess.py", line 378, in call
    with Popen(*popenargs, **kwargs) as p:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/cpython/cpython/Lib/subprocess.py", line 1011, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/home/runner/work/cpython/cpython/Lib/subprocess.py", line 1888, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
PermissionError: [Errno 1] Operation not permitted
Error: Process completed with exit code 1.

However, this is fixed by excluding a zero from the comparison: if (groups_size > 0). I think we may do it this way because setgroups(0, NULL) is a no-op in child_exec (a positive returned value is dropped anyway):

If size is zero, list is not modified, but the total number of supplementary group IDs for the process is returned.

-- https://linux.die.net/man/2/setgroups

For Bedevere: I have made the requested changes; please review again. For correct jump from Mentioned to this message instead of the following Bedevere's one: @gpshead.

[bedevere-bot]

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

[arhadthedev]

@gpshead @tiran could you reconsider this PR please? This is a prerequirement for gh-94519, created to get rid of uninitialized variables.

[tiran]

We are currently busy with 3.11 release. It may take until 3.11.0 final before we come back to this patch. It's definitely on the list for 3.12.

[arhadthedev]

Todo: replace groups_size/groups parameter names with extra_group_size/extra_groups across the whole file to avoid confusion of passers-by. It's necessary to mitigate a misleading name of setgroups that supposes replacement of not supplimentary group affinities, but all ones. The mislead results in seeing the omission of setgroups call for groups_size == 0 as a bug (like we should reset the inherited affinities but do nothing instead).

I'll do it in a separate PR because both this PR and gh-94519 are already big enough to clobber them with extra details.

[arhadthedev]

@tiran could you review and possibly merge this PR please? It's an implementation of your proposal #94519 (comment) created as a separate PR to preserve diffs of both PRs readable.

[gpshead]

thanks for doing this!

[arhadthedev]

Thank you for merging, Gregory!