bpo-38417: Add umask support to subprocess (GH-16726)

On POSIX systems, allow the umask to be set in the child process before we exec.

7 files changed, 57 insertions, 19 deletions

diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst
index 1a98bb33d0..19290bfc34 100644
--- a/Doc/library/subprocess.rst
+++ b/Doc/library/subprocess.rst
@@ -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,
@@ -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.
diff --git a/Lib/multiprocessing/util.py b/Lib/multiprocessing/util.py
index 207a2f70b2..3e640b944e 100644
--- a/Lib/multiprocessing/util.py
+++ b/Lib/multiprocessing/util.py
@@ -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)
diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index 1016874c4a..b5a45d9fea 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -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
@@ -750,7 +752,7 @@ class Popen(object):
                  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
@@ -945,7 +947,7 @@ class Popen(object):
                                 c2pread, c2pwrite,
                                 errread, errwrite,
                                 restore_signals,
-                                gid, gids, uid,
+                                gid, gids, uid, umask,
                                 start_new_session)
         except:
             # Cleanup if the child failed starting.
@@ -1318,6 +1320,7 @@ class Popen(object):
                            errread, errwrite,
                            unused_restore_signals,
                            unused_gid, unused_gids, unused_uid,
+                           unused_umask,
                            unused_start_new_session):
             """Execute program (MS Windows version)"""
 
@@ -1645,7 +1648,7 @@ class Popen(object):
                            c2pread, c2pwrite,
                            errread, errwrite,
                            restore_signals,
-                           gid, gids, uid,
+                           gid, gids, uid, umask,
                            start_new_session):
             """Execute program (POSIX version)"""
 
@@ -1684,7 +1687,8 @@ class Popen(object):
                     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,
@@ -1738,7 +1742,7 @@ class Popen(object):
                             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:
diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py
index 6731447af4..2024435109 100644
--- a/Lib/test/test_capi.py
+++ b/Lib/test/test_capi.py
@@ -97,7 +97,7 @@ class CAPITest(unittest.TestCase):
             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):
@@ -105,7 +105,7 @@ class CAPITest(unittest.TestCase):
             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):
@@ -115,7 +115,7 @@ class CAPITest(unittest.TestCase):
 
         # 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")
diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
index 22516402da..5cc324b878 100644
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -1894,6 +1894,28 @@ class POSIXProcessTestCase(BaseTestCase):
         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():
@@ -2950,7 +2972,7 @@ class POSIXProcessTestCase(BaseTestCase):
                         -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)"
@@ -2999,7 +3021,7 @@ class POSIXProcessTestCase(BaseTestCase):
                         -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:
diff --git a/Misc/NEWS.d/next/Library/2019-10-12-00-13-47.bpo-38417.W7x_aS.rst b/Misc/NEWS.d/next/Library/2019-10-12-00-13-47.bpo-38417.W7x_aS.rst
new file mode 100644
index 0000000000..c2356ddbf1
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2019-10-12-00-13-47.bpo-38417.W7x_aS.rst
@@ -0,0 +1,2 @@
+Added support for setting the umask in the child process to the subprocess
+module on POSIX systems.
diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
index 66db93e450..80bb44dce9 100644
--- a/Modules/_posixsubprocess.c
+++ b/Modules/_posixsubprocess.c
@@ -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
@@ -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)
@@ -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();
 
@@ -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;
@@ -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;
 
@@ -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. */