GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-118107: Fix zipimporter ZIP64 handling. by jsirois · Pull Request #118108 · python/cpython

Conversation

@jsirois

@jsirois jsirois commented Apr 19, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jsirois
pythongh-118107: Fix zipimporter ZIP64 handling.
702ece5
The code that handles too large files and offsets was missing an import.
@jsirois
jsirois requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners April 19, 2024 16:39
@ghost

ghost commented Apr 19, 2024 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot mentioned this pull request Apr 19, 2024
zipimport.zipimporter breaks for zips containing files with size > 0xFFFFFFFF #118107
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Apr 19, 2024
@jsirois jsirois mentioned this pull request Apr 19, 2024
gh-89739: gh-77140: Support zip64 in zipimport #94146
Merged
itamaro
itamaro requested changes Apr 19, 2024
View reviewed changes

@itamaro itamaro left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

thank you for catching this and the quick patch!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Apr 19, 2024
jsirois
jsirois commented Apr 19, 2024
View reviewed changes
Comment thread Lib/zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@jsirois

jsirois commented Apr 19, 2024

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jsirois added 2 commits April 19, 2024 09:57
@jsirois
Move the struct import to the top level.
923cdce
@jsirois
Fix whitespace to match local convention for import comments.
6fb7668
@Eclips4

Eclips4 commented Apr 19, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@itamaro

itamaro commented Apr 19, 2024

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jsirois

jsirois commented Apr 19, 2024

Copy link
Copy Markdown
Contributor Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@itamaro

itamaro commented Apr 19, 2024

Copy link
Copy Markdown
Contributor

@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

itamaro
itamaro reviewed Apr 19, 2024
View reviewed changes
Comment thread Lib/zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@jsirois
Add a test for large files and fix fallout in zipimporter.
8d55e2f
jsirois
jsirois commented Apr 20, 2024
View reviewed changes
Comment thread Lib/test/test_zipimport.py
self.doTest(".py", files, "f65536", comment=b"c" * ((1 << 16) - 1))

def testZip64LargeFile(self):
support.requires(

@jsirois jsirois Apr 20, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gpshead and encukou reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
jsirois
jsirois commented Apr 20, 2024
View reviewed changes
Comment thread 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",

@jsirois jsirois Apr 20, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

itamaro reacted with eyes emoji
All reactions
  • 👀 1 reaction

@itamaro itamaro Apr 21, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

another good catch! glad you added the test case!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jsirois jsirois mentioned this pull request Apr 20, 2024
Upgrade Python 3.13 to alpha 6. pex-tool/pex#2398
Merged
jsirois
jsirois commented Apr 20, 2024
View reviewed changes
Comment thread Lib/test/test_zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@jsirois
Merge branch 'main' into fix-issue-118107
86b23e3
itamaro
itamaro approved these changes Apr 21, 2024
View reviewed changes

@itamaro itamaro left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

thank you, this lgtm!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gpshead and encukou reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Comment thread 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",

@itamaro itamaro Apr 21, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

another good catch! glad you added the test case!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Eclips4
Eclips4 reviewed Apr 21, 2024
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2024-04-19-09-28-43.gh-issue-118107.Mdsr1J.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@jsirois @Eclips4
Update Misc/NEWS.d/next/Library/2024-04-19-09-28-43.gh-issue-118107.M…
13edef2
…dsr1J.rst

Co-authored-by: Kirill Podoprigora <[email protected]>
@jsirois
Merge branch 'main' into fix-issue-118107
d5df3fd
@gpshead gpshead self-assigned this Apr 22, 2024
jsirois and others added 4 commits April 24, 2024 12:29
@jsirois
Merge branch 'main' into fix-issue-118107
5d1f88d
@gpshead @jsirois
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.
@jsirois
Kill ZIP_DEFLATED sparse trick and document.
83bfe20
@jsirois
Merge remote-tracking branch 'refs/remotes/upstream/main' into fix-is…
b1bdd8f
…sue-118107
@jsirois

jsirois commented May 5, 2024

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou
encukou approved these changes May 6, 2024
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app 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
@jsirois
Fix 80 cols and make generated test data reproducible, with added tes…
00e30b2
…ts of test.
@jsirois
Merge remote-tracking branch 'upstream/main' into fix-issue-118107
3dc2325
@ambv
Merge branch 'main' into fix-issue-118107
801d0f4
@encukou encukou self-assigned this May 7, 2024
@encukou
Add test/zipimport_data to Makefile
9a15380
@encukou

encukou commented May 7, 2024

Copy link
Copy Markdown
Member

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

I've added zipimport_data to TESTSUBDIRS in Makefile.

ambv reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou
encukou merged commit 49258ef into python:main May 7, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label May 7, 2024
@jsirois
jsirois deleted the fix-issue-118107 branch May 7, 2024 14:00
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
@jsirois @gpshead
@Eclips4 @SonicField
pythongh-118107: Fix zipimporter ZIP64 handling. (pythonGH-118108)
1539e02
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] <[email protected]>
Co-authored-by: Kirill Podoprigora <[email protected]>
facebook-github-bot pushed a commit to facebookincubator/MetaPython that referenced this pull request Apr 5, 2025
@fried @facebook-github-bot
Fix zipimporter
d949e78
Summary:
My fix matches upstream

python/cpython#118108 (comment)

Reviewed By: itamaro

Differential Revision: D72491515

fbshipit-source-id: 8a469cb9c6612997dd42e8a349c947df32a6586a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead left review comments
@Eclips4 Eclips4 Eclips4 left review comments
@itamaro itamaro itamaro approved these changes
@encukou encukou encukou approved these changes
@brettcannon brettcannon Awaiting requested review from brettcannon brettcannon is a code owner
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

@encukou encukou

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@jsirois @Eclips4 @itamaro @encukou @gpshead @ambv
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.