diff --git a/Lib/os.py b/Lib/os.py index 586238334a4..864edfd48e0 100644 --- 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 8bc8ba9fa62..605d2a4632f 100644 --- 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 3f99773e8a6..62c1312cdda 100644 --- 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.