Issue #17222: Raise FileExistsError when py_compile.compile would

overwrite a symlink or non-regular file with a regular file.

diff --git a/Doc/library/py_compile.rst b/Doc/library/py_compile.rst
index 0c8c99d3bfd6a8285b54dafbcc8e03e38a46892e..bae8450b7c929cff06aebb9635d4ea3ac017f9b8 100644 (file)
--- a/Doc/library/py_compile.rst
+++ b/Doc/library/py_compile.rst
@@ -41,6 +41,13 @@ byte-code cache files in the directory containing the source code.
    is raised.  This function returns the path to byte-compiled file, i.e.
    whatever *cfile* value was used.
 
+   If the path that *cfile* becomes (either explicitly specified or computed)
+   is a symlink or non-regular file, :exc:`FileExistsError` will be raised.
+   This is to act as a warning that import will turn those paths into regular
+   files if it is allowed to write byte-compiled files to those paths. This is
+   a side-effect of import using file renaming to place the final byte-compiled
+   file into place to prevent concurrent file writing issues.
+
    *optimize* controls the optimization level and is passed to the built-in
    :func:`compile` function.  The default of ``-1`` selects the optimization
    level of the current interpreter.
@@ -53,7 +60,9 @@ byte-code cache files in the directory containing the source code.
    .. versionchanged:: 3.4
       Changed code to use :mod:`importlib` for the byte-code cache file writing.
       This means file creation/writing semantics now match what :mod:`importlib`
-      does, e.g. permissions, write-and-move semantics, etc.
+      does, e.g. permissions, write-and-move semantics, etc. Also added the
+      caveat that :exc:`FileExistsError` is raised if *cfile* is a symlink or
+      non-regular file.
 
 
 .. function:: main(args=None)

diff --git a/Doc/whatsnew/3.4.rst b/Doc/whatsnew/3.4.rst
index 1054c687b6984b4f1f4459eada9f1624eaea8717..90b10e9b80b4d48f6ad1c046fcb99f393169e47a 100644 (file)
--- a/Doc/whatsnew/3.4.rst
+++ b/Doc/whatsnew/3.4.rst
@@ -272,3 +272,8 @@ that may require changes to your code.
 
 * :c:func:`PyErr_SetImportError` now sets :exc:`TypeError` when its **msg**
   argument is not set. Previously only ``NULL`` was returned.
+
+* :func:`py_compile.compile` now raises :exc:`FileExistsError` if the file path
+  it would write to is a symlink or a non-regular file. This is to act as a
+  warning that import will overwrite those files with a regular file regardless
+  of what type of file path they were originally.

diff --git a/Lib/py_compile.py b/Lib/py_compile.py
index 701e8acb9a696354dadff3312a9f62e68c78edf2..cee35a5b6b1121311e17a0947867417fcfe63fad 100644 (file)
--- a/Lib/py_compile.py
+++ b/Lib/py_compile.py
@@ -7,6 +7,7 @@ import imp
 import importlib._bootstrap
 import importlib.machinery
 import os
+import os.path
 import sys
 import traceback
 
@@ -96,12 +97,25 @@ def compile(file, cfile=None, dfile=None, doraise=False, optimize=-1):
     See compileall.py for a script/module that uses this module to
     byte-compile all installed files (or all files in selected
     directories).
+
+    Do note that FileExistsError is raised if cfile ends up pointing at a
+    non-regular file or symlink. Because the compilation uses a file renaming,
+    the resulting file would be regular and thus not the same type of file as
+    it was previously.
     """
     if cfile is None:
         if optimize >= 0:
             cfile = imp.cache_from_source(file, debug_override=not optimize)
         else:
             cfile = imp.cache_from_source(file)
+    if os.path.islink(cfile):
+        msg = ('{} is a symlink and will be changed into a regular file if '
+               'import writes a byte-compiled file to it')
+        raise FileExistsError(msg.format(file, cfile))
+    elif os.path.exists(cfile) and not os.path.isfile(cfile):
+        msg = ('{} is a non-regular file and will be changed into a regular '
+               'one if import writes a byte-compiled file to it')
+        raise FileExistsError(msg.format(file, cfile))
     loader = importlib.machinery.SourceFileLoader('<py_compile>', file)
     source_bytes = loader.get_data(file)
     try:

diff --git a/Lib/test/test_py_compile.py b/Lib/test/test_py_compile.py
index e1795546e1f7f831e0a1907d62ce9d110d854cc7..54dc59660ad6a203c109bec1507876beecacd0f2 100644 (file)
--- a/Lib/test/test_py_compile.py
+++ b/Lib/test/test_py_compile.py
@@ -36,6 +36,26 @@ class PyCompileTests(unittest.TestCase):
         self.assertTrue(os.path.exists(self.pyc_path))
         self.assertFalse(os.path.exists(self.cache_path))
 
+    def test_do_not_overwrite_symlinks(self):
+        # In the face of a cfile argument being a symlink, bail out.
+        # Issue #17222
+        try:
+            os.symlink(self.pyc_path + '.actual', self.pyc_path)
+        except OSError:
+            self.skipTest('need to be able to create a symlink for a file')
+        else:
+            assert os.path.islink(self.pyc_path)
+            with self.assertRaises(FileExistsError):
+                py_compile.compile(self.source_path, self.pyc_path)
+
+    @unittest.skipIf(not os.path.exists(os.devnull) or os.path.isfile(os.devnull),
+                     'requires os.devnull and for it to be a non-regular file')
+    def test_do_not_overwrite_nonregular_files(self):
+        # In the face of a cfile argument being a non-regular file, bail out.
+        # Issue #17222
+        with self.assertRaises(FileExistsError):
+            py_compile.compile(self.source_path, os.devnull)
+
     def test_cache_path(self):
         py_compile.compile(self.source_path)
         self.assertTrue(os.path.exists(self.cache_path))

diff --git a/Misc/NEWS b/Misc/NEWS
index 1fcb9c9cf59efeb76b6a4033609223f7c4c33888..fb1fedf4c6237535216c276bbb8f9bf21432bebe 100644 (file)
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -1005,6 +1005,9 @@ Library
   Python file.  Patch by Ben Morgan.
 
 - Have py_compile use importlib as much as possible to avoid code duplication.
+  Code now raises FileExistsError if the file path to be used for the
+  byte-compiled file is a symlink or non-regular file as a warning that import
+  will not keep the file path type if it writes to that path.
 
 - Issue #180022: Have site.addpackage() consider already known paths even when
   none are explicitly passed in. Bug report and fix by Kirill.