Fixes Issue #14992: os.makedirs(path, exist_ok=True) would raise an OSError

when the path existed and had the S_ISGID mode bit set when it was not
explicitly asked for.  This is no longer an exception as mkdir cannot control
if the OS sets that bit for it or not.

diff --git a/Lib/os.py b/Lib/os.py
index 586238334a47b554d050f390597eeba570b78e99..864edfd48e00eefa7f0690ace98f9ae3a7f88f53 100644 (file)
--- a/Lib/os.py
+++ b/Lib/os.py
@@ -152,8 +152,20 @@ def makedirs(name, mode=0o777, exist_ok=False):
         mkdir(name, mode)
     except OSError as e:
         import stat as st
-        if not (e.errno == errno.EEXIST and exist_ok and path.isdir(name) and
-                st.S_IMODE(lstat(name).st_mode) == _get_masked_mode(mode)):
+        dir_exists = path.isdir(name)
+        expected_mode = _get_masked_mode(mode)
+        if dir_exists:
+            # S_ISGID is automatically copied by the OS from parent to child
+            # directories on mkdir.  Don't consider it being set to be a mode
+            # mismatch as mkdir does not unset it when not specified in mode.
+            actual_mode = st.S_IMODE(lstat(name).st_mode) & ~st.S_ISGID
+        else:
+            actual_mode = -1
+        if not (e.errno == errno.EEXIST and exist_ok and dir_exists and
+                actual_mode == expected_mode):
+            if dir_exists and actual_mode != expected_mode:
+                e.strerror += ' (mode %o != expected mode %o)' % (
+                        actual_mode, expected_mode)
             raise
 
 def removedirs(name):

diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py
index 8bc8ba9fa625e0e3e9be7e3a08da3e9c36e6f200..605d2a4632f42517064fac28d19179f3131d72d6 100644 (file)
--- a/Lib/test/test_os.py
+++ b/Lib/test/test_os.py
@@ -15,6 +15,7 @@ from test import support
 import contextlib
 import mmap
 import uuid
+import stat
 from test.script_helper import assert_python_ok
 
 # Detect whether we're on a Linux system that uses the (now outdated
@@ -574,12 +575,39 @@ class MakedirTests(unittest.TestCase):
         path = os.path.join(support.TESTFN, 'dir1')
         mode = 0o777
         old_mask = os.umask(0o022)
-        os.makedirs(path, mode)
-        self.assertRaises(OSError, os.makedirs, path, mode)
-        self.assertRaises(OSError, os.makedirs, path, mode, exist_ok=False)
-        self.assertRaises(OSError, os.makedirs, path, 0o776, exist_ok=True)
-        os.makedirs(path, mode=mode, exist_ok=True)
-        os.umask(old_mask)
+        try:
+            os.makedirs(path, mode)
+            self.assertRaises(OSError, os.makedirs, path, mode)
+            self.assertRaises(OSError, os.makedirs, path, mode, exist_ok=False)
+            self.assertRaises(OSError, os.makedirs, path, 0o776, exist_ok=True)
+            os.makedirs(path, mode=mode, exist_ok=True)
+        finally:
+            os.umask(old_mask)
+
+    def test_exist_ok_s_isgid_directory(self):
+        path = os.path.join(support.TESTFN, 'dir1')
+        S_ISGID = stat.S_ISGID
+        mode = 0o777
+        old_mask = os.umask(0o022)
+        try:
+            existing_testfn_mode = stat.S_IMODE(
+                    os.lstat(support.TESTFN).st_mode)
+            os.chmod(support.TESTFN, existing_testfn_mode | S_ISGID)
+            if (os.lstat(support.TESTFN).st_mode & S_ISGID != S_ISGID):
+                raise unittest.SkipTest('No support for S_ISGID dir mode.')
+            # The os should apply S_ISGID from the parent dir for us, but
+            # this test need not depend on that behavior.  Be explicit.
+            os.makedirs(path, mode | S_ISGID)
+            # http://bugs.python.org/issue14992
+            # Should not fail when the bit is already set.
+            os.makedirs(path, mode, exist_ok=True)
+            # remove the bit.
+            os.chmod(path, stat.S_IMODE(os.lstat(path).st_mode) & ~S_ISGID)
+            with self.assertRaises(OSError):
+                # Should fail when the bit is not already set when demanded.
+                os.makedirs(path, mode | S_ISGID, exist_ok=True)
+        finally:
+            os.umask(old_mask)
 
     def test_exist_ok_existing_regular_file(self):
         base = support.TESTFN

diff --git a/Misc/NEWS b/Misc/NEWS
index 3f99773e8a67ab2b1728be826f874f32d2bd3083..62c1312cddabcd217025f77bd5baca51920dd514 100644 (file)
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -10,6 +10,11 @@ What's New in Python 3.2.4
 Core and Builtins
 -----------------
 
+- Issue #14992: os.makedirs(path, exist_ok=True) would raise an OSError
+  when the path existed and had the S_ISGID mode bit set when it was
+  not explicitly asked for.  This is no longer an exception as mkdir
+  cannot control if the OS sets that bit for it or not.
+
 - Issue #14775: Fix a potential quadratic dict build-up due to the garbage
   collector repeatedly trying to untrack dicts.