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

Have _posixsubprocess.c stop using boolean flags to say if gid and uid values were supplied and action is required.  Such an implicit "either initialized or look somewhere else" confused both the reader (another mental connection to constantly track between functions) and a compiler (warnings on potentially uninitialized variables being passed). Instead, we can utilize a special group/user id as a flag value -1 defined by POSIX but used 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 #94519.

diff --git a/Misc/NEWS.d/next/Library/2022-07-22-13-38-37.gh-issue-94518._ZP0cz.rst b/Misc/NEWS.d/next/Library/2022-07-22-13-38-37.gh-issue-94518._ZP0cz.rst
new file mode 100644 (file)
index 0000000..a9d6d69
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2022-07-22-13-38-37.gh-issue-94518._ZP0cz.rst
@@ -0,0 +1,2 @@
+``_posixsubprocess`` now initializes all UID and GID variables using a
+reserved ``-1`` value instead of a separate flag. Patch by Oleg Iarygin.

diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
index b7563ee8250a9458aa394a0a0ab50be718d1e5e4..516f11d3543d58a89eaf06c723a49493f7328d35 100644 (file)
--- a/Modules/_posixsubprocess.c
+++ b/Modules/_posixsubprocess.c
@@ -518,9 +518,9 @@ child_exec(char *const exec_array[],
            int errpipe_read, int errpipe_write,
            int close_fds, int restore_signals,
            int call_setsid, pid_t pgid_to_set,
-           int call_setgid, gid_t gid,
-           int call_setgroups, size_t groups_size, const gid_t *groups,
-           int call_setuid, uid_t uid, int child_umask,
+           gid_t gid,
+           Py_ssize_t groups_size, const gid_t *groups,
+           uid_t uid, int child_umask,
            const void *child_sigmask,
            PyObject *py_fds_to_keep,
            PyObject *preexec_fn,
@@ -619,17 +619,17 @@ child_exec(char *const exec_array[],
 #endif
 
 #ifdef HAVE_SETGROUPS
-    if (call_setgroups)
+    if (groups_size > 0)
         POSIX_CALL(setgroups(groups_size, groups));
 #endif /* HAVE_SETGROUPS */
 
 #ifdef HAVE_SETREGID
-    if (call_setgid)
+    if (gid != (gid_t)-1)
         POSIX_CALL(setregid(gid, gid));
 #endif /* HAVE_SETREGID */
 
 #ifdef HAVE_SETREUID
-    if (call_setuid)
+    if (uid != (uid_t)-1)
         POSIX_CALL(setreuid(uid, uid));
 #endif /* HAVE_SETREUID */
 
@@ -724,9 +724,9 @@ do_fork_exec(char *const exec_array[],
              int errpipe_read, int errpipe_write,
              int close_fds, int restore_signals,
              int call_setsid, pid_t pgid_to_set,
-             int call_setgid, gid_t gid,
-             int call_setgroups, size_t groups_size, const gid_t *groups,
-             int call_setuid, uid_t uid, int child_umask,
+             gid_t gid,
+             Py_ssize_t groups_size, const gid_t *groups,
+             uid_t uid, int child_umask,
              const void *child_sigmask,
              PyObject *py_fds_to_keep,
              PyObject *preexec_fn,
@@ -738,9 +738,9 @@ do_fork_exec(char *const exec_array[],
 #ifdef VFORK_USABLE
     if (child_sigmask) {
         /* These are checked by our caller; verify them in debug builds. */
-        assert(!call_setuid);
-        assert(!call_setgid);
-        assert(!call_setgroups);
+        assert(uid == (uid_t)-1);
+        assert(gid == (gid_t)-1);
+        assert(groups_size < 0);
         assert(preexec_fn == Py_None);
 
         pid = vfork();
@@ -777,8 +777,8 @@ do_fork_exec(char *const exec_array[],
                p2cread, p2cwrite, c2pread, c2pwrite,
                errread, errwrite, errpipe_read, errpipe_write,
                close_fds, restore_signals, call_setsid, pgid_to_set,
-               call_setgid, gid, call_setgroups, groups_size, groups,
-               call_setuid, uid, child_umask, child_sigmask,
+               gid, groups_size, groups,
+               uid, child_umask, child_sigmask,
                py_fds_to_keep, preexec_fn, preexec_fn_args_tuple);
     _exit(255);
     return 0;  /* Dead code to avoid a potential compiler warning. */
@@ -799,9 +799,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
     int errpipe_read, errpipe_write, close_fds, restore_signals;
     int call_setsid;
     pid_t pgid_to_set = -1;
-    int call_setgid = 0, call_setgroups = 0, call_setuid = 0;
-    uid_t uid;
-    gid_t gid, *groups = NULL;
+    gid_t *groups = NULL;
     int child_umask;
     PyObject *cwd_obj, *cwd_obj2 = NULL;
     const char *cwd;
@@ -899,9 +897,6 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
 
     if (groups_list != Py_None) {
 #ifdef HAVE_SETGROUPS
-        Py_ssize_t i;
-        gid_t gid;
-
         if (!PyList_Check(groups_list)) {
             PyErr_SetString(PyExc_TypeError,
                     "setgroups argument must be a list");
@@ -917,13 +912,17 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
             goto cleanup;
         }
 
-        if ((groups = PyMem_RawMalloc(num_groups * sizeof(gid_t))) == NULL) {
-            PyErr_SetString(PyExc_MemoryError,
-                    "failed to allocate memory for group list");
-            goto cleanup;
+        /* Deliberately keep groups == NULL for num_groups == 0 */
+        if (num_groups > 0) {
+            groups = PyMem_RawMalloc(num_groups * sizeof(gid_t));
+            if (groups == NULL) {
+                PyErr_SetString(PyExc_MemoryError,
+                        "failed to allocate memory for group list");
+                goto cleanup;
+            }
         }
 
-        for (i = 0; i < num_groups; i++) {
+        for (Py_ssize_t i = 0; i < num_groups; i++) {
             PyObject *elem;
             elem = PySequence_GetItem(groups_list, i);
             if (!elem)
@@ -934,6 +933,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
                 Py_DECREF(elem);
                 goto cleanup;
             } else {
+                gid_t gid;
                 if (!_Py_Gid_Converter(elem, &gid)) {
                     Py_DECREF(elem);
                     PyErr_SetString(PyExc_ValueError, "invalid group id");
@@ -943,7 +943,6 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
             }
             Py_DECREF(elem);
         }
-        call_setgroups = 1;
 
 #else /* HAVE_SETGROUPS */
         PyErr_BadInternalCall();
@@ -951,26 +950,24 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
 #endif /* HAVE_SETGROUPS */
     }
 
+    gid_t gid = (gid_t)-1;
     if (gid_object != Py_None) {
 #ifdef HAVE_SETREGID
         if (!_Py_Gid_Converter(gid_object, &gid))
             goto cleanup;
 
-        call_setgid = 1;
-
 #else /* HAVE_SETREGID */
         PyErr_BadInternalCall();
         goto cleanup;
 #endif /* HAVE_SETREUID */
     }
 
+    uid_t uid = (uid_t)-1;
     if (uid_object != Py_None) {
 #ifdef HAVE_SETREUID
         if (!_Py_Uid_Converter(uid_object, &uid))
             goto cleanup;
 
-        call_setuid = 1;
-
 #else /* HAVE_SETREUID */
         PyErr_BadInternalCall();
         goto cleanup;
@@ -994,7 +991,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
     /* Use vfork() only if it's safe. See the comment above child_exec(). */
     sigset_t old_sigs;
     if (preexec_fn == Py_None && allow_vfork &&
-        !call_setuid && !call_setgid && !call_setgroups) {
+        uid == (uid_t)-1 && gid == (gid_t)-1 && num_groups < 0) {
         /* Block all signals to ensure that no signal handlers are run in the
          * child process while it shares memory with us. Note that signals
          * used internally by C libraries won't be blocked by
@@ -1017,8 +1014,8 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
                        p2cread, p2cwrite, c2pread, c2pwrite,
                        errread, errwrite, errpipe_read, errpipe_write,
                        close_fds, restore_signals, call_setsid, pgid_to_set,
-                       call_setgid, gid, call_setgroups, num_groups, groups,
-                       call_setuid, uid, child_umask, old_sigmask,
+                       gid, num_groups, groups,
+                       uid, child_umask, old_sigmask,
                        py_fds_to_keep, preexec_fn, preexec_fn_args_tuple);
 
     /* Parent (original) process */