Skip to content

WIP: bpo-34990: year 2038 problem in compileall.py #9892

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
matrixise wants to merge 12 commits into from
Closed

WIP: bpo-34990: year 2038 problem in compileall.py #9892

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f4757a7
bpo-34990: year 2038 problem in compileall.py
matrixise Oct 15, 2018
96dbdab
Add a blurb entry
matrixise Oct 15, 2018
de197d1
Support unsigned long in the tests of compileall.py
matrixise Oct 15, 2018
377f565
Add a test with compileall and the .pyc file with a timestamp >= 2018…
matrixise Oct 15, 2018
b7e89cc
Use unsigned long for the header size
matrixise Oct 17, 2018
eca7980
Use a bitwise 0xFFFF_FFFF on the mtime of the .pyc files
matrixise Oct 17, 2018
2cc344c
Update
matrixise Oct 17, 2018
dc57dde
Fix the mask
matrixise Oct 17, 2018
efdae35
Move test_compile_all_2038 to CompileallTestsBase
matrixise Oct 17, 2018
1f30292
reword the blurb entry
matrixise Oct 17, 2018
4ed1e74
Update importlib_external after the modifications in importlib
matrixise Oct 17, 2018
ca6c049
Revert my modification in Lib/importlib/_bootstrap_external.py: useless,
matrixise Oct 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Lib/compileall.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,8 @@ def compile_file(fullname, ddir=None, force=False, rx=None, quiet=0,
if tail == '.py':
if not force:
try:
mtime = int(os.stat(fullname).st_mtime)
expect = struct.pack('<4sll', importlib.util.MAGIC_NUMBER,
mtime = int(os.stat(fullname).st_mtime) & 0xFFFF_FFFF
expect = struct.pack('<4sLL', importlib.util.MAGIC_NUMBER,
0, mtime)
Comment thread
vstinner marked this conversation as resolved.
Resolved
with open(cfile, 'rb') as chandle:
actual = chandle.read(12)
Expand Down
11 changes: 7 additions & 4 deletions Lib/test/test_compileall.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ def add_bad_source_file(self):
def timestamp_metadata(self):
with open(self.bc_path, 'rb') as file:
data = file.read(12)
mtime = int(os.stat(self.source_path).st_mtime)
compare = struct.pack('<4sll', importlib.util.MAGIC_NUMBER, 0, mtime)
mtime = int(os.stat(self.source_path).st_mtime) & 0xFFFF_FFF

@vstinner vstinner Oct 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I like the underscore: it's easier to spot bugs. You missed something :-)

compare = struct.pack('<4sLL', importlib.util.MAGIC_NUMBER, 0, mtime)
return data, compare

@unittest.skipUnless(hasattr(os, 'stat'), 'test needs os.stat()')
Expand All @@ -76,7 +76,7 @@ def recreation_check(self, metadata):

def test_mtime(self):
# Test a change in mtime leads to a new .pyc.
self.recreation_check(struct.pack('<4sll', importlib.util.MAGIC_NUMBER,
self.recreation_check(struct.pack('<4sLL', importlib.util.MAGIC_NUMBER,
0, 1))

def test_magic_number(self):
Expand Down Expand Up @@ -200,7 +200,10 @@ class CompileallTestsWithSourceEpoch(CompileallTestsBase,
unittest.TestCase,
metaclass=SourceDateEpochTestMeta,
source_date_epoch=True):
pass
def test_compile_all_2038(self):
with open(self.source_path, 'r') as f:
os.utime(f.name, (2147558400, 2147558400)) # Jan 20, 2038 as touch
self.assertTrue(compileall.compile_file(pathlib.Path(self.source_path)))

@bmwiedemann bmwiedemann Oct 15, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, but maybe this is not a good place in this file to add the test in, because SOURCE_DATE_EPOCH changes the default .pyc type to be hash-based, which would not hit the error condition. See #9607

@matrixise matrixise Oct 15, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, just copy the code of @tirkarthi . I will wait for a review from @vstinner or an other core-dev.

@vstinner vstinner Oct 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be moved to CompileallTestsBase.

@matrixise matrixise Oct 17, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done



@serhiy-storchaka serhiy-storchaka Oct 18, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change.

class CompileallTestsWithoutSourceEpoch(CompileallTestsBase,
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_zipimport.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def make_pyc(co, mtime, size):
else:
mtime = int(-0x100000000 + int(mtime))
pyc = (importlib.util.MAGIC_NUMBER +

@serhiy-storchaka serhiy-storchaka Oct 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the above few lines can be removed.

@matrixise matrixise Oct 17, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the lines from 38->41?

@serhiy-storchaka serhiy-storchaka Oct 18, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. They are roughly equivalent to mtime = int(mtime) & 0xFFFF_FFFF if use an unsigned integer format.

struct.pack("<iii", 0, int(mtime), size & 0xFFFFFFFF) + data)

@serhiy-storchaka serhiy-storchaka Oct 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed format units are used.

struct.pack("<iii", 0, int(mtime) & 0xFFFF_FFFF, size & 0xFFFF_FFFF) + data)
return pyc

def module_path_to_dotted_name(path):
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core and Builtins/2018-10-15-20-41-18.bpo-34990.NVDjdP.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Use an unsigned long for the timestamp for the .pyc file in compileall.py

@vstinner vstinner Oct 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You describe the fix, but you should describe the fixed bug instead: compileall now supports timestamps newer than year 2038.

@matrixise matrixise Oct 17, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

2 changes: 1 addition & 1 deletion Tools/scripts/checkpyc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def main():
print('Bad MAGIC word in ".pyc" file', end=' ')
print(repr(name_c))
continue
mtime = get_long(mtime_str)
mtime = get_long(mtime_str) & 0xFFFF_FFFF

@serhiy-storchaka serhiy-storchaka Oct 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sees the definition of get_long(). It returns an unsigned 32-bit integer, and returns -1 only on error. You need to convert not it, but the file timestamp.

@bmwiedemann bmwiedemann Oct 17, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a NOP, because mtime_str is just 4 bytes, that cannot encode anything larger than 0xFFFF_FFFF

@matrixise matrixise Oct 17, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I am not familiar with the operations on the bits.

if mtime in {0, -1}:

@serhiy-storchaka serhiy-storchaka Oct 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mtime can't be -1 after adding & 0xFFFF_FFFF.

@matrixise matrixise Oct 17, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add the 0xFFFF_FFFF on the line 57?

@serhiy-storchaka serhiy-storchaka Oct 18, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

print('Bad ".pyc" file', repr(name_c))
elif mtime != st[ST_MTIME]:

@vstinner vstinner Oct 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this code should be modified, but it is modified: should we also cast to int()?

@matrixise matrixise Oct 17, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like that?

elif int(mtime) != int(st[ST_MTIME] & 0xFFFF_FFFF)

@serhiy-storchaka serhiy-storchaka Oct 18, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mtime is already int. But st[ST_MTIME] is float.

Expand Down