[Python-checkins] r69481 - in python/trunk: Lib/compileall.py Lib/test/test_compileall.py Misc/ACKS Misc/NEWS

[brett.cannon]

Author: brett.cannon
Date: Tue Feb 10 03:07:38 2009
New Revision: 69481

Log:
compileall used the ctime of bytecode and source to determine if the bytecode
should be recreated. This created a timing hole. Fixed by just doing what
import does; check the mtime and magic number.


Added:
   python/trunk/Lib/test/test_compileall.py
Modified:
   python/trunk/Lib/compileall.py
   python/trunk/Misc/ACKS
   python/trunk/Misc/NEWS

Modified: python/trunk/Lib/compileall.py
==============================================================================
--- python/trunk/Lib/compileall.py	(original)
+++ python/trunk/Lib/compileall.py	Tue Feb 10 03:07:38 2009
@@ -11,10 +11,11 @@
 See module py_compile for details of the actual byte-compilation.
 
 """
-
 import os
 import sys
 import py_compile
+import struct
+import imp
 
 __all__ = ["compile_dir","compile_path"]
 
@@ -54,11 +55,17 @@
         if os.path.isfile(fullname):
             head, tail = name[:-3], name[-3:]
             if tail == '.py':
-                cfile = fullname + (__debug__ and 'c' or 'o')
-                ftime = os.stat(fullname).st_mtime
-                try: ctime = os.stat(cfile).st_mtime
-                except os.error: ctime = 0
-                if (ctime > ftime) and not force: continue
+                if not force:
+                    try:
+                        mtime = os.stat(fullname).st_mtime
+                        expect = struct.pack('<4sl', imp.get_magic(), mtime)
+                        cfile = fullname + (__debug__ and 'c' or 'o')
+                        with open(cfile, 'rb') as chandle:
+                            actual = chandle.read(8)
+                        if expect == actual:
+                            continue
+                    except IOError:
+                        pass
                 if not quiet:
                     print 'Compiling', fullname, '...'
                 try:
@@ -80,7 +87,8 @@
              name != os.curdir and name != os.pardir and \
              os.path.isdir(fullname) and \
              not os.path.islink(fullname):
-            if not compile_dir(fullname, maxlevels - 1, dfile, force, rx, quiet):
+            if not compile_dir(fullname, maxlevels - 1, dfile, force, rx,
+                               quiet):
                 success = 0
     return success
 

Added: python/trunk/Lib/test/test_compileall.py
==============================================================================
--- (empty file)
+++ python/trunk/Lib/test/test_compileall.py	Tue Feb 10 03:07:38 2009
@@ -0,0 +1,63 @@
+import compileall
+import imp
+import os
+import py_compile
+import shutil
+import struct
+import sys
+import tempfile
+import time
+from test import test_support
+import unittest
+
+
+class CompileallTests(unittest.TestCase):
+
+    def setUp(self):
+        self.directory = tempfile.mkdtemp()
+        self.source_path = os.path.join(self.directory, '_test.py')
+        self.bc_path = self.source_path + ('c' if __debug__ else 'o')
+        with open(self.source_path, 'w') as file:
+            file.write('x = 123\n')
+
+    def tearDown(self):
+        shutil.rmtree(self.directory)
+
+    def data(self):
+        with open(self.bc_path, 'rb') as file:
+            data = file.read(8)
+        mtime = int(os.stat(self.source_path).st_mtime)
+        compare = struct.pack('<4sl', imp.get_magic(), mtime)
+        return data, compare
+
+    def recreation_check(self, metadata):
+        """Check that compileall recreates bytecode when the new metadata is
+        used."""
+        if not hasattr(os, 'stat'):
+            return
+        py_compile.compile(self.source_path)
+        self.assertEqual(*self.data())
+        with open(self.bc_path, 'rb') as file:
+            bc = file.read()[len(metadata):]
+        with open(self.bc_path, 'wb') as file:
+            file.write(metadata)
+            file.write(bc)
+        self.assertNotEqual(*self.data())
+        compileall.compile_dir(self.directory, force=False, quiet=True)
+        self.assertTrue(*self.data())
+
+    def test_mtime(self):
+        # Test a change in mtime leads to a new .pyc.
+        self.recreation_check(struct.pack('<4sl', imp.get_magic(), 1))
+
+    def test_magic_number(self):
+        # Test a change in mtime leads to a new .pyc.
+        self.recreation_check(b'\0\0\0\0')
+
+
+def test_main():
+    test_support.run_unittest(CompileallTests)
+
+
+if __name__ == "__main__":
+    test_main()

Modified: python/trunk/Misc/ACKS
==============================================================================
--- python/trunk/Misc/ACKS	(original)
+++ python/trunk/Misc/ACKS	Tue Feb 10 03:07:38 2009
@@ -183,7 +183,7 @@
 Andy Dustman
 Gary Duzan
 Eugene Dvurechenski
-Josip Dzolonga 
+Josip Dzolonga
 Maxim Dzumanenko
 Walter Dörwald
 Hans Eckardt
@@ -233,6 +233,7 @@
 Geoff Furnish
 Ulisses Furquim
 Achim Gaedke
+Martin von Gagern
 Lele Gaifax
 Santiago Gala
 Yitzchak Gale

Modified: python/trunk/Misc/NEWS
==============================================================================
--- python/trunk/Misc/NEWS	(original)
+++ python/trunk/Misc/NEWS	Tue Feb 10 03:07:38 2009
@@ -152,6 +152,10 @@
 Library
 -------
 
+- Issue #5128: Make compileall properly inspect bytecode to determine if needs
+  to be recreated. This avoids a timing hole thanks to the old reliance on the
+  ctime of the files involved.
+
 - Issue #5122: Synchronize tk load failure check to prevent a potential
   deadlock.