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

matrixise · 2018-10-15T18:40:05Z

Use 'L' as an unsigned long for the timestamp

https://bugs.python.org/issue34990

Use 'L' as an unsigned long for the timestamp

bmwiedemann · 2018-10-15T18:45:05Z

Lib/test/test_compileall.py also contains 2 instances of <4sll that probably should be updated accordingly.
and you could add the test code provided by xtreak in the bug.

matrixise · 2018-10-15T18:46:26Z

@bmwiedemann yep, I was in the plane from PySS and I just commited my code, and now, I just saw the test of @tirkarthi . I am going to include it.

…-01-20 Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>

tirkarthi · 2018-10-15T18:52:26Z

@matrixise Thanks for the PR. I think it's a NEWS entry worthy fix to add a short note that compileall now supports files with modification time greater than or equal to 19-01-2038 (I guess)

tirkarthi · 2018-10-15T18:53:47Z

@matrixise Sorry, I just noticed you have added one :)

matrixise · 2018-10-15T18:55:07Z

@tirkarthi please, could you check my PR with the last commits.

bmwiedemann · 2018-10-15T19:00:10Z

btw: the first bad mtime is 0x80000000 or 2147483648 or 2038-01-19T03:14:08+00:00

bmwiedemann · 2018-10-15T19:04:57Z

Lib/test/test_compileall.py

+    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)))


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

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

It should be moved to CompileallTestsBase.

tirkarthi · 2018-10-15T19:12:07Z

@matrixise Thanks, Looks good to me from the context of the issue reported but I have limited understanding about the code so I hope someone else might have a better look. Seems there was some discussion about handling 2038 problem as part of the original PR where this code was added : #4575 (comment)

bmwiedemann · 2018-10-15T19:23:43Z

I think the discussion in PR 4575 was about future-proofing the new .pyc format, but it failed to notice that by 2038, only 31 bits are exhausted and bit 32 (which was erroneously interpreted as the sign-bit here), allows to keep the format alive until 2106 - which we can safely assume out-of-scope for today.

matrixise · 2018-10-15T19:29:04Z

This is the reason why I move to 'L' instead of 'l'

serhiy-storchaka

Check also other parts of the stdlib that work with mtime in pyc files. For example zipimport and checkpyc.py need updates. Maybe there are other parts.

Lib/compileall.py

bedevere-bot · 2018-10-16T05:42:45Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

serhiy-storchaka · 2018-10-16T05:44:05Z

Lib/compileall.py

@@ -139,7 +139,7 @@ def compile_file(fullname, ddir=None, force=False, rx=None, quiet=0,
            if not force:
                try:
                    mtime = int(os.stat(fullname).st_mtime)
-                    expect = struct.pack('<4sll', importlib.util.MAGIC_NUMBER,
+                    expect = struct.pack('<4slL', importlib.util.MAGIC_NUMBER,


Also, it would be better to handle the second field as an unsigned integer too.

importlib uses int(mtime) & 0xFFFFFFFF, I suggest... int(mtime) & 0xFFFF_FFFF for readability :-D

matrixise · 2018-10-17T11:57:20Z

@serhiy-storchaka last update for Tools/scripts/cherrypyc.py by in June 2013, we only read the magic number and timestamp, just that: https://github.com/python/cpython/blob/master/Tools/scripts/checkpyc.py#L45-L46

In https://github.com/python/cpython/blob/master/Lib/zipimport.py#L602 you convert the dword as a uint32 and LGTM.

matrixise · 2018-10-17T12:16:05Z

@vstinner and @serhiy-storchaka I have fixed mtime with 0xFFFF_FFFF and use an unsigned long, where I found <4slL but for the other files, I am not an expert of the API and I don't know where I could search, zipimport and checkpy.py seem to be good.

bmwiedemann · 2018-10-17T12:45:04Z

Tools/scripts/checkpyc.py

@@ -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


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

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

serhiy-storchaka

zipimport needs truncating the timestamp to 32 bit for comparison.

Also update _code_to_timestamp_pyc() in _bootstrap_external.

serhiy-storchaka · 2018-10-17T12:29:22Z

Tools/scripts/checkpyc.py

@@ -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
                if mtime in {0, -1}:


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

maybe add the 0xFFFF_FFFF on the line 57?

serhiy-storchaka · 2018-10-17T12:29:57Z

Lib/test/test_zipimport.py

@@ -40,7 +40,7 @@ def make_pyc(co, mtime, size):
        else:
            mtime = int(-0x100000000 + int(mtime))
    pyc = (importlib.util.MAGIC_NUMBER +


I think the above few lines can be removed.

the lines from 38->41?

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

serhiy-storchaka · 2018-10-17T12:30:35Z

Lib/test/test_zipimport.py

@@ -40,7 +40,7 @@ def make_pyc(co, mtime, size):
        else:
            mtime = int(-0x100000000 + int(mtime))
    pyc = (importlib.util.MAGIC_NUMBER +
-        struct.pack("<iii", 0, int(mtime), size & 0xFFFFFFFF) + data)


Signed format units are used.

serhiy-storchaka · 2018-10-17T12:39:09Z

Tools/scripts/checkpyc.py

@@ -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


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 · 2018-10-17T13:03:04Z

Lib/test/test_zipimport.py

@@ -40,7 +40,7 @@ def make_pyc(co, mtime, size):
        else:
            mtime = int(-0x100000000 + int(mtime))
    pyc = (importlib.util.MAGIC_NUMBER +
-        struct.pack("<iii", 0, int(mtime) & 0xFFFF_FFFF, size & 0xFFFF_FFFF) + data)
+        struct.pack("<iii", 0, int(mtime), size & 0xFFFF_FFFF) + data)


why this revert?

just because @serhiy-storchaka indicated that the signed was used here.

I meant that you need to update the format string.

vstinner · 2018-10-17T13:32:56Z

Lib/importlib/_bootstrap_external.py

@@ -586,7 +586,7 @@ def _code_to_timestamp_pyc(code, mtime=0, source_size=0):
    "Produce the data for a timestamp-based pyc."
    data = bytearray(MAGIC_NUMBER)
    data.extend(_pack_uint32(0))
-    data.extend(_pack_uint32(mtime))
+    data.extend(_pack_uint32(mtime) & 0xFFFF_FFFF)


This change is useless, _pack_uint32() already does "& 0xFFFFFFFF". importlib can be left unchanged.

vstinner · 2018-10-17T13:33:32Z

Lib/test/test_compileall.py

@@ -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


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

vstinner · 2018-10-17T13:34:36Z

Lib/test/test_compileall.py

+    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)))


It should be moved to CompileallTestsBase.

vstinner · 2018-10-17T13:35:43Z

Misc/NEWS.d/next/Core and Builtins/2018-10-15-20-41-18.bpo-34990.NVDjdP.rst

@@ -0,0 +1 @@
+Use an unsigned long for the timestamp for the .pyc file in compileall.py


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

vstinner · 2018-10-17T13:36:28Z

Tools/scripts/checkpyc.py

@@ -54,7 +54,7 @@ def main():
                mtime = get_long(mtime_str)
                if mtime in {0, -1}:
                    print('Bad ".pyc" file', repr(name_c))
-                elif mtime != st[ST_MTIME]:


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

like that?

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

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

matrixise · 2018-10-17T13:50:52Z

@serhiy-storchaka can you continue the review with me, because I am a little bit like a chicken without head ;-) I try to understand with 0xFFFF_FFFF ;-)

_pack_uint32() already does "& 0xFFFFFFFF"

serhiy-storchaka · 2018-10-18T11:35:40Z

Lib/test/test_zipimport.py

@@ -40,7 +40,7 @@ def make_pyc(co, mtime, size):
        else:
            mtime = int(-0x100000000 + int(mtime))
    pyc = (importlib.util.MAGIC_NUMBER +
-        struct.pack("<iii", 0, int(mtime) & 0xFFFF_FFFF, size & 0xFFFF_FFFF) + data)
+        struct.pack("<iii", 0, int(mtime), size & 0xFFFF_FFFF) + data)


I meant that you need to update the format string.

serhiy-storchaka · 2018-10-18T11:37:07Z

Tools/scripts/checkpyc.py

@@ -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
                if mtime in {0, -1}:


serhiy-storchaka · 2018-10-18T11:38:07Z

Tools/scripts/checkpyc.py

@@ -54,7 +54,7 @@ def main():
                mtime = get_long(mtime_str)
                if mtime in {0, -1}:
                    print('Bad ".pyc" file', repr(name_c))
-                elif mtime != st[ST_MTIME]:


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

serhiy-storchaka · 2018-10-18T11:39:47Z

Misc/NEWS.d/next/Core and Builtins/2018-10-15-20-41-18.bpo-34990.NVDjdP.rst

@@ -0,0 +1 @@
+compileall now supports timestamps newer than year 2038


Please use punctuation.

serhiy-storchaka · 2018-10-18T11:43:01Z

Lib/test/test_zipimport.py

@@ -40,7 +40,7 @@ def make_pyc(co, mtime, size):
        else:
            mtime = int(-0x100000000 + int(mtime))
    pyc = (importlib.util.MAGIC_NUMBER +


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

serhiy-storchaka · 2018-10-18T11:43:24Z

Lib/test/test_compileall.py


 class CompileallTestsWithSourceEpoch(CompileallTestsBase,
                                     unittest.TestCase,
                                     metaclass=SourceDateEpochTestMeta,
                                     source_date_epoch=True):
    pass

-


Unrelated change.

brettcannon · 2018-11-02T16:57:37Z

Due to the WIP in the title I added the DO-NOT-MERGE label.

matrixise · 2018-11-03T09:03:57Z

@brettcannon Thank you, I will continue to work on this issue on the next week, this week I am at the hospital with my daughter.

bmwiedemann · 2019-01-21T15:18:25Z

Only 19 years to go... Can we get this moving again?

vstinner · 2019-01-22T16:20:03Z

Ping @matrixise: they are still some comments of @serhiy-storchaka that you didn't address.

matrixise · 2019-02-04T08:34:27Z

@vstinner pong, ok thanks for the reminder.

matrixise · 2019-03-25T19:47:50Z

So, I am not serious with this bug, I am going to continue on this bug, I would like to close all my PRs.

vstinner · 2019-03-26T12:21:29Z

@vstinner pong, ok thanks for the reminder.

FYI I ignore this issue since it's still tagged as "WIP". I prefer to wait until it's no longer a WIP to review it.

brettcannon · 2019-04-17T22:45:49Z

Removing the import team from reviewing as this is still a WIP. We can be added back when it's ready to go.

bmwiedemann · 2019-08-22T12:13:30Z

@matrixise any chance to get this finished this year?

vstinner · 2019-08-22T13:03:00Z

@matrixise any chance to get this finished this year?

It's ok as soon as it's fixed before 2038 :-D

bmwiedemann · 2019-08-22T13:38:22Z

It's ok as soon as it's fixed before 2038 :-D

I am afraid there are way too many places where software is used unchanged for a long time. Even 18 years. And of course this is not the only timebomb waiting to trigger.

vstinner · 2019-08-22T22:18:39Z

It was a joke. Hint: see the emoticon.

ammaraskar · 2020-04-22T10:09:47Z

Since Stéphane said:

I would like to close all my PRs.

I opened up #19651 with the more permanent solution of using a 64-bit timestamp value as Benjamin originally suggested in their PEP 552 PR.

ammaraskar · 2020-04-25T03:56:01Z

I pinged @matrixise about this and they said I can feel free to take this on since they're too busy to move forward with it. #19708 is a continuation of this PR to make compileall use an unsigned int.

It should be possible to compare the code for both the solutions now.

tirkarthi · 2021-08-24T10:08:34Z

I guess this can be close since #19708 is merged

encukou · 2021-08-24T10:13:48Z

Indeed. Thanks for noticing.

bpo-34990: year 2038 problem in compileall.py

f4757a7

Use 'L' as an unsigned long for the timestamp

the-knights-who-say-ni added the CLA signed label Oct 15, 2018

bedevere-bot added the awaiting review label Oct 15, 2018

Add a blurb entry

96dbdab

matrixise added 2 commits October 15, 2018 20:52

Support unsigned long in the tests of compileall.py

de197d1

Add a test with compileall and the .pyc file with a timestamp >= 2018…

377f565

…-01-20 Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>

bmwiedemann reviewed Oct 15, 2018

View reviewed changes

serhiy-storchaka requested changes Oct 16, 2018

View reviewed changes

Lib/compileall.py Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting review labels Oct 16, 2018

serhiy-storchaka reviewed Oct 16, 2018

View reviewed changes

Use unsigned long for the header size

b7e89cc

Use a bitwise 0xFFFF_FFFF on the mtime of the .pyc files

eca7980

matrixise requested a review from a team October 17, 2018 12:14

bmwiedemann reviewed Oct 17, 2018

View reviewed changes

serhiy-storchaka requested changes Oct 17, 2018

View reviewed changes

Update

2cc344c

matrixise closed this Oct 17, 2018

bmwiedemann reviewed Oct 17, 2018

View reviewed changes

vstinner requested changes Oct 17, 2018

View reviewed changes

matrixise added 3 commits October 17, 2018 15:37

Fix the mask

dc57dde

Move test_compile_all_2038 to CompileallTestsBase

efdae35

reword the blurb entry

1f30292

matrixise added 2 commits October 17, 2018 15:52

Update importlib_external after the modifications in importlib

4ed1e74

Revert my modification in Lib/importlib/_bootstrap_external.py: useless,

ca6c049

_pack_uint32() already does "& 0xFFFFFFFF"

serhiy-storchaka reviewed Oct 18, 2018

View reviewed changes

matrixise changed the title ~~bpo-34990: year 2038 problem in compileall.py~~ WIP: bpo-34990: year 2038 problem in compileall.py Oct 25, 2018

brettcannon added the DO-NOT-MERGE label Nov 2, 2018

brettcannon removed the request for review from a team April 17, 2019 22:45

serhiy-storchaka mentioned this pull request Apr 29, 2020

bpo-34990: Treat the pyc header's mtime in compileall as an unsigned int #19708

Merged

encukou closed this Aug 24, 2021

		@@ -0,0 +1 @@
		Use an unsigned long for the timestamp for the .pyc file in compileall.py

		@@ -0,0 +1 @@
		compileall now supports timestamps newer than year 2038

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

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

Conversation

matrixise commented Oct 15, 2018 • edited by bedevere-bot Loading

bmwiedemann commented Oct 15, 2018

matrixise commented Oct 15, 2018

tirkarthi commented Oct 15, 2018

tirkarthi commented Oct 15, 2018

matrixise commented Oct 15, 2018

bmwiedemann commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tirkarthi commented Oct 15, 2018

bmwiedemann commented Oct 15, 2018

matrixise commented Oct 15, 2018

serhiy-storchaka left a comment

Choose a reason for hiding this comment

bedevere-bot commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matrixise commented Oct 17, 2018

matrixise commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matrixise commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettcannon commented Nov 2, 2018

matrixise commented Nov 3, 2018

bmwiedemann commented Jan 21, 2019

vstinner commented Jan 22, 2019

matrixise commented Feb 4, 2019

matrixise commented Mar 25, 2019

vstinner commented Mar 26, 2019

brettcannon commented Apr 17, 2019

bmwiedemann commented Aug 22, 2019

vstinner commented Aug 22, 2019

bmwiedemann commented Aug 22, 2019

vstinner commented Aug 22, 2019

ammaraskar commented Apr 22, 2020

ammaraskar commented Apr 25, 2020

tirkarthi commented Aug 24, 2021

encukou commented Aug 24, 2021

matrixise commented Oct 15, 2018 •

edited by bedevere-bot

Loading