[Python-checkins] cpython: Close #2501: Permission bits are once again correctly copied from the source

[nick.coghlan]

http://hg.python.org/cpython/rev/3a831a0a29c4
changeset:   78722:3a831a0a29c4
user:        Nick Coghlan <ncoghlan at gmail.com>
date:        Fri Aug 24 18:32:40 2012 +1000
summary:
  Close #2501: Permission bits are once again correctly copied from the source file to the cached bytecode file. Test by Eric Snow.

files:
  Lib/importlib/_bootstrap.py |    31 +-
  Lib/test/test_import.py     |    25 +-
  Misc/NEWS                   |     5 +
  Python/importlib.h          |  8517 +++++++++++-----------
  4 files changed, 4345 insertions(+), 4233 deletions(-)


diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py
--- a/Lib/importlib/_bootstrap.py
+++ b/Lib/importlib/_bootstrap.py
@@ -118,13 +118,14 @@
     return _path_is_mode_type(path, 0o040000)
 
 
-def _write_atomic(path, data):
+def _write_atomic(path, data, mode=0o666):
     """Best-effort function to write data to a path atomically.
     Be prepared to handle a FileExistsError if concurrent writing of the
     temporary file is attempted."""
     # id() is used to generate a pseudo-random filename.
     path_tmp = '{}.{}'.format(path, id(path))
-    fd = _os.open(path_tmp, _os.O_EXCL | _os.O_CREAT | _os.O_WRONLY, 0o666)
+    fd = _os.open(path_tmp,
+                  _os.O_EXCL | _os.O_CREAT | _os.O_WRONLY, mode & 0o666)
     try:
         # We first write data to a temporary file, and then use os.replace() to
         # perform an atomic rename.
@@ -887,6 +888,16 @@
         """
         return {'mtime': self.path_mtime(path)}
 
+    def _cache_bytecode(self, source_path, cache_path, data):
+        """Optional method which writes data (bytes) to a file path (a str).
+
+        Implementing this method allows for the writing of bytecode files.
+
+        The source path is needed in order to correctly transfer permissions
+        """
+        # For backwards compatibility, we delegate to set_data()
+        return self.set_data(cache_path, data)
+
     def set_data(self, path, data):
         """Optional method which writes data (bytes) to a file path (a str).
 
@@ -974,7 +985,7 @@
             data.extend(_w_long(len(source_bytes)))
             data.extend(marshal.dumps(code_object))
             try:
-                self.set_data(bytecode_path, data)
+                self._cache_bytecode(source_path, bytecode_path, data)
                 _verbose_message('wrote {!r}', bytecode_path)
             except NotImplementedError:
                 pass
@@ -1029,7 +1040,11 @@
         st = _os.stat(path)
         return {'mtime': st.st_mtime, 'size': st.st_size}
 
-    def set_data(self, path, data):
+    def _cache_bytecode(self, source_path, bytecode_path, data):
+        # Adapt between the two APIs
+        return self.set_data(bytecode_path, data, source_path=source_path)
+
+    def set_data(self, path, data, *, source_path=None):
         """Write bytes data to a file."""
         parent, filename = _path_split(path)
         path_parts = []
@@ -1049,8 +1064,14 @@
                 # If can't get proper access, then just forget about writing
                 # the data.
                 return
+        mode = 0o666
+        if source_path is not None:
+            try:
+                mode = _os.stat(source_path).st_mode
+            except OSError:
+                pass
         try:
-            _write_atomic(path, data)
+            _write_atomic(path, data, mode)
             _verbose_message('created {!r}', path)
         except (PermissionError, FileExistsError):
             # Don't worry if you can't write bytecode or someone is writing
diff --git a/Lib/test/test_import.py b/Lib/test/test_import.py
--- a/Lib/test/test_import.py
+++ b/Lib/test/test_import.py
@@ -120,12 +120,35 @@
                 s = os.stat(fn)
                 # Check that the umask is respected, and the executable bits
                 # aren't set.
-                self.assertEqual(stat.S_IMODE(s.st_mode), 0o666 & ~mask)
+                self.assertEqual(oct(stat.S_IMODE(s.st_mode)), oct(0o666 & ~mask))
             finally:
                 del sys.path[0]
                 remove_files(TESTFN)
                 unload(TESTFN)
 
+    @unittest.skipUnless(os.name == 'posix',
+                         "test meaningful only on posix systems")
+    def test_cached_mode_issue_2051(self):
+        mode = 0o600
+        source = TESTFN + ".py"
+        with script_helper.temp_dir() as tempdir:
+            path = script_helper.make_script(tempdir, TESTFN,
+                                             "key='top secret'")
+            os.chmod(path, mode)
+            compiled = imp.cache_from_source(path)
+            sys.path.insert(0, tempdir)
+            try:
+                __import__(TESTFN)
+            finally:
+                sys.path.remove(tempdir)
+
+            if not os.path.exists(compiled):
+                self.fail("__import__ did not result in creation of "
+                          "either a .pyc or .pyo file")
+            stat_info = os.stat(compiled)
+
+        self.assertEqual(oct(stat.S_IMODE(stat_info.st_mode)), oct(mode))
+
     def test_imp_module(self):
         # Verify that the imp module can correctly load and find .py files
         # XXX (ncoghlan): It would be nice to use support.CleanImport
diff --git a/Misc/NEWS b/Misc/NEWS
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -10,6 +10,11 @@
 Core and Builtins
 -----------------
 
+- Issue #2501: Source file permission bits are once again correctly
+  copied to the cached bytecode file. (The migration to importlib
+  reintroduced this problem because these was no regression test. A test
+  has been added as part of this patch)
+
 - Issue #15761: Fix crash when PYTHONEXECUTABLE is set on Mac OS X.
 
 - Issue #15726: Fix incorrect bounds checking in PyState_FindModule.
diff --git a/Python/importlib.h b/Python/importlib.h
--- a/Python/importlib.h
+++ b/Python/importlib.h
[stripped]

-- 
Repository URL: http://hg.python.org/cpython