gh-118107: Fix zipimporter ZIP64 handling. by jsirois · Pull Request #118108 · python/cpython

jsirois · 2024-04-19T16:39:04Z

The code that handles too large files and offsets was missing an import
and was manipulating a tuple as if it was a list.

Issue: zipimport.zipimporter breaks for zips containing files with size > 0xFFFFFFFF #118107

The code that handles too large files and offsets was missing an import.

ghost · 2024-04-19T16:39:07Z

All commit authors signed the Contributor License Agreement.

itamaro

thank you for catching this and the quick patch!

could you add a test case that covers this code path please?

Lib/zipimport.py

jsirois · 2024-04-19T16:51:03Z

could you add a test case that covers this code path please?

@itamaro Absolutely. In Pex my test builds a >4GB file to add to the zip to trigger this (I run the test if cat is present and use that in 32 doubling operations to grow a file with a single newline into the required size). Is it OK to add a test that generates a >4GB temp file? I was wary since CPython has such a diverse dev community and I suspect a diverse range of personal and otherwise machines that its tests run on.

Eclips4 · 2024-04-19T16:59:51Z

could you add a test case that covers this code path please?

@itamaro Absolutely. In Pex my test builds a >4GB file to add to the zip to trigger this (I run the test if cat is present and use that in 32 doubling operations to grow a file with a single newline into the required size). Is it OK to add a test that generates a >4GB temp file? I was wary since CPython has such a diverse dev community and I suspect a diverse range of personal and otherwise machines that its tests run on.

That's a good point!
Sadly, we do not have helper like skip_no_disk_space in our support package (It's already present in Lib/test/test_largefile.py). Maybe it's a good reason to add it to the support? If so, it's should be done in a separate PR.

itamaro · 2024-04-19T17:07:24Z

right, I think either using skip_no_disk_space + requires('largefile' ...) to gate the test, or making it a bigmemtest if it can be done in-memory instead of using actual files on disk

jsirois · 2024-04-19T17:09:48Z

Maybe it's a good reason to add it to the support? If so, it's should be done in a separate PR.

@Eclips4 and @itamaro how about I add a test in this PR with code similar to that large file test setup code you pointed me at with an eye to parameterization of the file size and then if I get another positive ACK that that code should be moved to support and de-deduped, then I'll detour to pull it out to a new PR?

itamaro · 2024-04-19T17:15:55Z

@Eclips4 and @itamaro how about I add a test in this PR with code similar to that large file test setup code you pointed me at with an eye to parameterization of the file size and then if I get another positive ACK that that code should be moved to support and de-deduped, then I'll detour to pull it out to a new PR?

sounds good to me

Lib/zipimport.py

jsirois · 2024-04-20T03:16:13Z

Lib/test/test_zipimport.py

        self.doTest(".py", files, "f65536", comment=b"c" * ((1 << 16) - 1))

+    def testZip64LargeFile(self):
+        support.requires(


I had introduced the decorator from Lib/test/test_largefile.py and then stumbled upon the resources support (-u) that seems more widely used; so I went with it instead. The tests only run (and pass) with -ulargefile:

./python -m test test_zipimport -ulargefile -v -m testZip64LargeFile == CPython 3.13.0a6+ (heads/fix-issue-118107:8d55e2f226, Apr 19 2024, 20:12:18) [GCC 11.4.0] == Linux-5.15.146.1-microsoft-standard-WSL2-x86_64-with-glibc2.35 little-endian == Python build: debug == cwd: /home/jsirois/dev/python/cpython/build/test_python_worker_191158æ == CPU count: 16 == encodings: locale=UTF-8 FS=utf-8 == resources (1): largefile Using random seed: 3331549713 0:00:00 load avg: 3.80 Run 1 test sequentially 0:00:00 load avg: 3.80 [1/1] test_zipimport testZip64LargeFile (test.test_zipimport.CompressedZipImportTestCase.testZip64LargeFile) ... ok testZip64LargeFile (test.test_zipimport.UncompressedZipImportTestCase.testZip64LargeFile) ... ok ---------------------------------------------------------------------- Ran 2 tests in 42.905s OK test_zipimport passed in 43.0 sec == Tests result: SUCCESS == 1 test OK. Total duration: 43.0 sec Total tests: run=2 (filtered) Total test files: run=1/1 (filtered) Result: SUCCESS

jsirois · 2024-04-20T03:18:09Z

Lib/zipimport.py

-                            values = struct.unpack_from(f"<{min(num_extra_values, 3)}Q",
-                                                        extra_data, offset=4)
+                            import struct
+                            values = list(struct.unpack_from(f"<{min(num_extra_values, 3)}Q",


The new test caught the need for the list here. Prior to the fix you'd see:

ERROR: testZip64LargeFile (test.test_zipimport.UncompressedZipImportTestCase.testZip64LargeFile) ---------------------------------------------------------------------- Traceback (most recent call last): File "<frozen importlib._bootstrap_external>", line 1510, in _path_importer_cache KeyError: '/home/jsirois/dev/python/cpython/build/test_python_worker_140690æ/junk95142.zip' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/jsirois/dev/python/cpython/Lib/test/test_zipimport.py", line 89, in wrapper return fun(*args, **kwargs) ~~~^^^^^^^^^^^^^^^^^ File "/home/jsirois/dev/python/cpython/Lib/test/test_zipimport.py", line 89, in wrapper return fun(*args, **kwargs) ~~~^^^^^^^^^^^^^^^^^ File "/home/jsirois/dev/python/cpython/Lib/test/test_zipimport.py", line 849, in testZip64LargeFile self.doTestWithPreBuiltZip(".py", "large_file") ~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^ File "/home/jsirois/dev/python/cpython/Lib/test/test_zipimport.py", line 157, in doTestWithPreBuiltZip mod = importlib.import_module(".".join(modules)) ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^ File "/home/jsirois/dev/python/cpython/Lib/importlib/__init__.py", line 88, in import_module return _bootstrap._gcd_import(name[level:], package, level) ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "<frozen importlib._bootstrap>", line 1387, in _gcd_import File "<frozen importlib._bootstrap>", line 1360, in _find_and_load File "<frozen importlib._bootstrap>", line 1322, in _find_and_load_unlocked File "<frozen importlib._bootstrap>", line 1262, in _find_spec File "<frozen importlib._bootstrap_external>", line 1553, in find_spec File "<frozen importlib._bootstrap_external>", line 1525, in _get_spec File "<frozen importlib._bootstrap_external>", line 1512, in _path_importer_cache File "<frozen importlib._bootstrap_external>", line 1488, in _path_hooks File "<frozen zipimport>", line 98, in __init__ File "<frozen zipimport>", line 528, in _read_directory AttributeError: 'tuple' object has no attribute 'pop'

another good catch! glad you added the test case!

Lib/test/test_zipimport.py

itamaro

thank you, this lgtm!

itamaro · 2024-04-21T01:07:22Z

Lib/zipimport.py

-                            values = struct.unpack_from(f"<{min(num_extra_values, 3)}Q",
-                                                        extra_data, offset=4)
+                            import struct
+                            values = list(struct.unpack_from(f"<{min(num_extra_values, 3)}Q",


another good catch! glad you added the test case!

Misc/NEWS.d/next/Library/2024-04-19-09-28-43.gh-issue-118107.Mdsr1J.rst

…dsr1J.rst Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>

The code to generate the test data is included in the test itself and will run when testdata is not found.

…sue-118107

jsirois · 2024-05-05T01:36:16Z

Alrighty @gpshead, I think this is good to go.

encukou

The checked-in files contain mtimes, so they aren't reproducible. A few bytes were different when I re-ran the command.

I don't think this should block the PR.

Lib/test/test_zipimport.py

…ts of test.

encukou · 2024-05-07T06:37:44Z

Thanks! Deleting the files and running the test now gives bit-for-bit identical results.

I've added zipimport_data to TESTSUBDIRS in Makefile.

Add missing import to code that handles too large files and offsets. Use list, not tuple, for a mutable sequence. Add tests to prevent similar mistakes. --------- Co-authored-by: Gregory P. Smith [Google LLC] <greg@krypto.org> Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>

Summary: My fix matches upstream python/cpython#118108 (comment) Reviewed By: itamaro Differential Revision: D72491515 fbshipit-source-id: 8a469cb9c6612997dd42e8a349c947df32a6586a

pythongh-118107: Fix zipimporter ZIP64 handling.

702ece5

The code that handles too large files and offsets was missing an import.

jsirois requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners April 19, 2024 16:39

bedevere-app bot mentioned this pull request Apr 19, 2024

zipimport.zipimporter breaks for zips containing files with size > 0xFFFFFFFF #118107

Closed

bedevere-app bot added the awaiting review label Apr 19, 2024

jsirois mentioned this pull request Apr 19, 2024

gh-89739: gh-77140: Support zip64 in zipimport #94146

Merged

itamaro requested changes Apr 19, 2024

View reviewed changes

Lib/zipimport.py Show resolved Hide resolved

bedevere-app bot added awaiting core review and removed awaiting review labels Apr 19, 2024

jsirois commented Apr 19, 2024

View reviewed changes

Lib/zipimport.py Show resolved Hide resolved

jsirois added 2 commits April 19, 2024 09:57

Move the struct import to the top level.

923cdce

Fix whitespace to match local convention for import comments.

6fb7668

itamaro reviewed Apr 19, 2024

View reviewed changes

Lib/zipimport.py Outdated Show resolved Hide resolved

Add a test for large files and fix fallout in zipimporter.

8d55e2f

jsirois commented Apr 20, 2024

View reviewed changes

jsirois mentioned this pull request Apr 20, 2024

Upgrade Python 3.13 to alpha 6. pex-tool/pex#2398

Merged

jsirois commented Apr 20, 2024

View reviewed changes

Lib/test/test_zipimport.py Show resolved Hide resolved

Merge branch 'main' into fix-issue-118107

86b23e3

itamaro approved these changes Apr 21, 2024

View reviewed changes

Eclips4 reviewed Apr 21, 2024

View reviewed changes

Misc/NEWS.d/next/Library/2024-04-19-09-28-43.gh-issue-118107.Mdsr1J.rst Outdated Show resolved Hide resolved

Update Misc/NEWS.d/next/Library/2024-04-19-09-28-43.gh-issue-118107.M…

13edef2

…dsr1J.rst Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>

Merge branch 'main' into fix-issue-118107

d5df3fd

gpshead self-assigned this Apr 22, 2024

jsirois and others added 4 commits April 24, 2024 12:29

Merge branch 'main' into fix-issue-118107

5d1f88d

Speed testZip64LargeFile up by checking in test data.

debba75

The code to generate the test data is included in the test itself and will run when testdata is not found.

Kill ZIP_DEFLATED sparse trick and document.

83bfe20

Merge remote-tracking branch 'refs/remotes/upstream/main' into fix-is…

b1bdd8f

…sue-118107

encukou approved these changes May 6, 2024

View reviewed changes

Lib/test/test_zipimport.py Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting core review labels May 6, 2024

jsirois and others added 3 commits May 6, 2024 10:46

Fix 80 cols and make generated test data reproducible, with added tes…

00e30b2

…ts of test.

Merge remote-tracking branch 'upstream/main' into fix-issue-118107

3dc2325

Merge branch 'main' into fix-issue-118107

801d0f4

encukou self-assigned this May 7, 2024

Add test/zipimport_data to Makefile

9a15380

encukou merged commit 49258ef into python:main May 7, 2024

bedevere-app bot removed the awaiting merge label May 7, 2024

jsirois deleted the fix-issue-118107 branch May 7, 2024 14:00

Uh oh!

Conversation

jsirois commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 19, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itamaro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jsirois commented Apr 19, 2024

Uh oh!

Eclips4 commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itamaro commented Apr 19, 2024

Uh oh!

jsirois commented Apr 19, 2024

Uh oh!

itamaro commented Apr 19, 2024

Uh oh!

Uh oh!

jsirois Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsirois Apr 20, 2024

Choose a reason for hiding this comment

Uh oh!

itamaro Apr 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

itamaro left a comment

Choose a reason for hiding this comment

Uh oh!

itamaro Apr 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsirois commented May 5, 2024

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

encukou commented May 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jsirois commented Apr 19, 2024 •

edited

Loading

ghost commented Apr 19, 2024 •

edited by ghost

Loading

Eclips4 commented Apr 19, 2024 •

edited

Loading

jsirois Apr 20, 2024 •

edited

Loading