[Python-checkins] gh-94518: [_posixsubprocess] Replace variable validity flags with reserved values (#94687)
gpshead
webhook-mailer at python.org
Sat Jan 14 15:11:15 EST 2023
https://github.com/python/cpython/commit/124af17b6e49f0f22fbe646fb57800393235d704
commit: 124af17b6e49f0f22fbe646fb57800393235d704
branch: main
author: Oleg Iarygin <oleg at arhadthedev.net>
committer: gpshead <greg at krypto.org>
date: 2023-01-14T12:11:04-08:00
summary:
gh-94518: [_posixsubprocess] Replace variable validity flags with reserved values (#94687)
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.
files:
A Misc/NEWS.d/next/Library/2022-07-22-13-38-37.gh-issue-94518._ZP0cz.rst
M Modules/_posixsubprocess.c
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
index 000000000000..a9d6d69f7eff
--- /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 b7563ee8250a..516f11d3543d 100644
--- 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 */
More information about the Python-checkins
mailing list