Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-118107: Fix zipimporter ZIP64 handling.#118108

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.

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

Already on GitHub?Sign in to your account

Merged
encukou merged 15 commits intopython:mainfromjsirois:fix-issue-118107
May 7, 2024

Conversation

jsirois
Copy link
Contributor

@jsiroisjsirois commentedApr 19, 2024•
edited
Loading

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

The code that handles too large files and offsets was missing an import.
@ghost
Copy link

ghost commentedApr 19, 2024•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

Copy link
Contributor

@itamaroitamaro left a 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?

gpshead reacted with thumbs up emoji
@jsirois
Copy link
ContributorAuthor

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 ifcat 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
Copy link
Member

Eclips4 commentedApr 19, 2024•
edited
Loading

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 ifcat 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 likeskip_no_disk_space in oursupport package (It's already present inLib/test/test_largefile.py). Maybe it's a good reason to add it to thesupport? If so, it's should be done in a separate PR.

@itamaro
Copy link
Contributor

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

@jsirois
Copy link
ContributorAuthor

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
Copy link
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

@@ -810,6 +812,27 @@ def testZip64CruftAndComment(self):
files = self.getZip64Files()
self.doTest(".py", files, "f65536", comment=b"c" * ((1 << 16) - 1))

def testZip64LargeFile(self):
support.requires(
Copy link
ContributorAuthor

@jsiroisjsiroisApr 20, 2024•
edited
Loading

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 fromLib/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): largefileUsing random seed: 33315497130:00:00 load avg: 3.80 Run 1 test sequentially0:00:00 load avg: 3.80 [1/1] test_zipimporttestZip64LargeFile (test.test_zipimport.CompressedZipImportTestCase.testZip64LargeFile) ... oktestZip64LargeFile (test.test_zipimport.UncompressedZipImportTestCase.testZip64LargeFile) ... ok----------------------------------------------------------------------Ran 2 tests in 42.905sOKtest_zipimport passed in 43.0 sec== Tests result: SUCCESS ==1 test OK.Total duration: 43.0 secTotal tests: run=2 (filtered)Total test files: run=1/1 (filtered)Result: SUCCESS

gpshead and encukou reacted with thumbs up emoji
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",
Copy link
ContributorAuthor

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_cacheKeyError: '/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_directoryAttributeError: 'tuple' object has no attribute 'pop'

itamaro reacted with eyes emoji
Copy link
Contributor

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!

Copy link
Contributor

@itamaroitamaro left a 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!

gpshead and encukou reacted with thumbs up emoji
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",
Copy link
Contributor

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!

…dsr1J.rstCo-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
@gpsheadgpshead self-assigned thisApr 22, 2024
@jsirois
Copy link
ContributorAuthor

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

Copy link
Member

@encukouencukou left a 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.

@encukouencukou self-assigned thisMay 7, 2024
@encukou
Copy link
Member

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

I've addedzipimport_data toTESTSUBDIRS in Makefile.

ambv reacted with thumbs up emoji

@encukouencukou merged commit49258ef intopython:mainMay 7, 2024
33 checks passed
@jsiroisjsirois deleted the fix-issue-118107 branchMay 7, 2024 14:00
SonicField pushed a commit to SonicField/cpython that referenced this pull requestMay 8, 2024
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>
facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this pull requestApr 5, 2025
Summary:My fix matches upstreampython/cpython#118108 (comment)Reviewed By: itamaroDifferential Revision: D72491515fbshipit-source-id: 8a469cb9c6612997dd42e8a349c947df32a6586a
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@gpsheadgpsheadgpshead left review comments

@Eclips4Eclips4Eclips4 left review comments

@itamaroitamaroitamaro approved these changes

@encukouencukouencukou approved these changes

@brettcannonbrettcannonAwaiting requested review from brettcannonbrettcannon is a code owner

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@ncoghlanncoghlanAwaiting requested review from ncoghlanncoghlan is a code owner

@warsawwarsawAwaiting requested review from warsawwarsaw is a code owner

Assignees

@gpsheadgpshead

@encukouencukou

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@jsirois@Eclips4@itamaro@encukou@gpshead@ambv
[8]ページ先頭
©2009-2025 Movatter.jp