Skip to content

bpo-38417: Add umask support to subprocess #16726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions Doc/library/subprocess.rst
Original file line number Diff line number Diff line change
Expand Up @@ -339,9 +339,9 @@ functions.
stderr=None, preexec_fn=None, close_fds=True, shell=False, \
cwd=None, env=None, universal_newlines=None, \
startupinfo=None, creationflags=0, restore_signals=True, \
start_new_session=False, pass_fds=(), *, group=None, \
extra_groups=None, user=None, encoding=None, errors=None, \
text=None)
start_new_session=False, pass_fds=(), \*, group=None, \
extra_groups=None, user=None, umask=-1, \
encoding=None, errors=None, text=None)

Execute a child program in a new process. On POSIX, the class uses
:meth:`os.execvp`-like behavior to execute the child program. On Windows,
Expand Down Expand Up @@ -572,6 +572,12 @@ functions.
.. availability:: POSIX
.. versionadded:: 3.9

If *umask* is not negative, the umask() system call will be made in the
child process prior to the execution of the subprocess.

.. availability:: POSIX
.. versionadded:: 3.9

If *env* is not ``None``, it must be a mapping that defines the environment
variables for the new process; these are used instead of the default
behavior of inheriting the current process' environment.
Expand Down
2 changes: 1 addition & 1 deletion Lib/multiprocessing/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ def spawnv_passfds(path, args, passfds):
return _posixsubprocess.fork_exec(
args, [os.fsencode(path)], True, passfds, None, None,
-1, -1, -1, -1, -1, -1, errpipe_read, errpipe_write,
False, False, None, None, None, None)
False, False, None, None, None, -1, None)
finally:
os.close(errpipe_read)
os.close(errpipe_write)
Expand Down
14 changes: 9 additions & 5 deletions Lib/subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,8 @@ class Popen(object):

user (POSIX only)

umask (POSIX only)

pass_fds (POSIX only)

encoding and errors: Text mode encoding and error handling to use for
Expand All @@ -750,7 +752,7 @@ def __init__(self, args, bufsize=-1, executable=None,
startupinfo=None, creationflags=0,
restore_signals=True, start_new_session=False,
pass_fds=(), *, user=None, group=None, extra_groups=None,
encoding=None, errors=None, text=None):
encoding=None, errors=None, text=None, umask=-1):
"""Create new Popen instance."""
_cleanup()
# Held while anything is calling waitpid before returncode has been
Expand Down Expand Up @@ -945,7 +947,7 @@ def __init__(self, args, bufsize=-1, executable=None,
c2pread, c2pwrite,
errread, errwrite,
restore_signals,
gid, gids, uid,
gid, gids, uid, umask,
start_new_session)
except:
# Cleanup if the child failed starting.
Expand Down Expand Up @@ -1318,6 +1320,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
errread, errwrite,
unused_restore_signals,
unused_gid, unused_gids, unused_uid,
unused_umask,
unused_start_new_session):
"""Execute program (MS Windows version)"""

Expand Down Expand Up @@ -1645,7 +1648,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
c2pread, c2pwrite,
errread, errwrite,
restore_signals,
gid, gids, uid,
gid, gids, uid, umask,
start_new_session):
"""Execute program (POSIX version)"""

Expand Down Expand Up @@ -1684,7 +1687,8 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
and not start_new_session
and gid is None
and gids is None
and uid is None):
and uid is None
and umask < 0):
self._posix_spawn(args, executable, env, restore_signals,
p2cread, p2cwrite,
c2pread, c2pwrite,
Expand Down Expand Up @@ -1738,7 +1742,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
errread, errwrite,
errpipe_read, errpipe_write,
restore_signals, start_new_session,
gid, gids, uid,
gid, gids, uid, umask,
preexec_fn)
self._child_created = True
finally:
Expand Down
6 changes: 3 additions & 3 deletions Lib/test/test_capi.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,15 +97,15 @@ class Z(object):
def __len__(self):
return 1
self.assertRaises(TypeError, _posixsubprocess.fork_exec,
1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20)
1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21)
# Issue #15736: overflow in _PySequence_BytesToCharpArray()
class Z(object):
def __len__(self):
return sys.maxsize
def __getitem__(self, i):
return b'x'
self.assertRaises(MemoryError, _posixsubprocess.fork_exec,
1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20)
1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21)

@unittest.skipUnless(_posixsubprocess, '_posixsubprocess required for this test.')
def test_subprocess_fork_exec(self):
Expand All @@ -115,7 +115,7 @@ def __len__(self):

# Issue #15738: crash in subprocess_fork_exec()
self.assertRaises(TypeError, _posixsubprocess.fork_exec,
Z(),[b'1'],3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20)
Z(),[b'1'],3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21)

@unittest.skipIf(MISSING_C_DOCSTRINGS,
"Signature information for builtins requires docstrings")
Expand Down
26 changes: 24 additions & 2 deletions Lib/test/test_subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -1894,6 +1894,28 @@ def test_extra_groups_error(self):
with self.assertRaises(ValueError):
subprocess.check_call([sys.executable, "-c", "pass"], extra_groups=[])

@unittest.skipIf(mswindows or not hasattr(os, 'umask'),
'POSIX umask() is not available.')
def test_umask(self):
tmpdir = None
try:
tmpdir = tempfile.mkdtemp()
name = os.path.join(tmpdir, "beans")
# We set an unusual umask in the child so as a unique mode
# for us to test the child's touched file for.
subprocess.check_call(
[sys.executable, "-c", f"open({name!r}, 'w')"], # touch
umask=0o053)
# Ignore execute permissions entirely in our test,
# filesystems could be mounted to ignore or force that.
st_mode = os.stat(name).st_mode & 0o666
expected_mode = 0o624
self.assertEqual(expected_mode, st_mode,
msg=f'{oct(expected_mode)} != {oct(st_mode)}')
finally:
if tmpdir is not None:
shutil.rmtree(tmpdir)

def test_run_abort(self):
# returncode handles signal termination
with support.SuppressCrashReport():
Expand Down Expand Up @@ -2950,7 +2972,7 @@ def test_fork_exec(self):
-1, -1, -1, -1,
1, 2, 3, 4,
True, True,
False, [], 0,
False, [], 0, -1,
func)
# Attempt to prevent
# "TypeError: fork_exec() takes exactly N arguments (M given)"
Expand Down Expand Up @@ -2999,7 +3021,7 @@ def __int__(self):
-1, -1, -1, -1,
1, 2, 3, 4,
True, True,
None, None, None,
None, None, None, -1,
None)
self.assertIn('fds_to_keep', str(c.exception))
finally:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Added support for setting the umask in the child process to the subprocess
module on POSIX systems.
14 changes: 9 additions & 5 deletions Modules/_posixsubprocess.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#ifdef HAVE_SYS_TYPES_H
#include <sys/types.h>
#endif
#if defined(HAVE_SYS_STAT_H) && defined(__FreeBSD__)
#if defined(HAVE_SYS_STAT_H)
#include <sys/stat.h>
#endif
#ifdef HAVE_SYS_SYSCALL_H
Expand Down Expand Up @@ -428,7 +428,7 @@ child_exec(char *const exec_array[],
int call_setsid,
int call_setgid, gid_t gid,
int call_setgroups, size_t groups_size, const gid_t *groups,
int call_setuid, uid_t uid,
int call_setuid, uid_t uid, int child_umask,
PyObject *py_fds_to_keep,
PyObject *preexec_fn,
PyObject *preexec_fn_args_tuple)
Expand Down Expand Up @@ -498,6 +498,9 @@ child_exec(char *const exec_array[],
if (cwd)
POSIX_CALL(chdir(cwd));

if (child_umask >= 0)
umask(child_umask); /* umask() always succeeds. */

if (restore_signals)
_Py_RestoreSignals();

Expand Down Expand Up @@ -609,6 +612,7 @@ subprocess_fork_exec(PyObject* self, PyObject *args)
int call_setgid = 0, call_setgroups = 0, call_setuid = 0;
uid_t uid;
gid_t gid, *groups = NULL;
int child_umask;
PyObject *cwd_obj, *cwd_obj2;
const char *cwd;
pid_t pid;
Expand All @@ -619,14 +623,14 @@ subprocess_fork_exec(PyObject* self, PyObject *args)
int saved_errno = 0;

if (!PyArg_ParseTuple(
args, "OOpO!OOiiiiiiiiiiOOOO:fork_exec",
args, "OOpO!OOiiiiiiiiiiOOOiO:fork_exec",
&process_args, &executable_list,
&close_fds, &PyTuple_Type, &py_fds_to_keep,
&cwd_obj, &env_list,
&p2cread, &p2cwrite, &c2pread, &c2pwrite,
&errread, &errwrite, &errpipe_read, &errpipe_write,
&restore_signals, &call_setsid,
&gid_object, &groups_list, &uid_object,
&gid_object, &groups_list, &uid_object, &child_umask,
&preexec_fn))
return NULL;

Expand Down Expand Up @@ -843,7 +847,7 @@ subprocess_fork_exec(PyObject* self, PyObject *args)
errread, errwrite, errpipe_read, errpipe_write,
close_fds, restore_signals, call_setsid,
call_setgid, gid, call_setgroups, num_groups, groups,
call_setuid, uid,
call_setuid, uid, child_umask,
py_fds_to_keep, preexec_fn, preexec_fn_args_tuple);
_exit(255);
return NULL; /* Dead code to avoid a potential compiler warning. */
Expand Down