gh-127847: Fix position in the special-cased zipfile seek by dimaryaz · Pull Request #127856 · python/cpython

dimaryaz · 2024-12-12T07:24:30Z

Issue: Data corruption when reading from two files in parallel in an uncompressed .zip #127847

ghost · 2024-12-12T07:24:33Z

All commit authors signed the Contributor License Agreement.

bedevere-app · 2024-12-12T07:24:37Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

ZeroIntensity

This is a great first contribution, thanks :)

I have a few nitpicks, mostly related to some high-level triage tidbits.

Misc/NEWS.d/next/Library/2024-12-12-07-27-51.gh-issue-127847.ksfNKM.rst

Lib/test/test_zipfile/test_core.py

danifus · 2024-12-13T01:30:52Z

Nice tests and the bug report was great.

The fix I suggested was incomplete (I forgot _fileobj was an instance of _SharedFile until after I responded to you). The fix needs to go up a level of abstraction :p

_SharedFile should manage the position within the zip entry for us but it's seek implementation isn't resuming from its current position (like it does in read):

cpython/Lib/zipfile/__init__.py

Lines 816 to 835 in 8bbd379

    
           def seek(self, offset, whence=0): 
        
               with self._lock: 
        
                   if self._writing(): 
        
                       raise ValueError("Can't reposition in the ZIP file while " 
        
                               "there is an open writing handle on it. " 
        
                               "Close the writing handle before trying to read.") 
        
                   self._file.seek(offset, whence) 
        
                   self._pos = self._file.tell() 
        
                   return self._pos 
        
           def read(self, n=-1): 
        
               with self._lock: 
        
                   if self._writing(): 
        
                       raise ValueError("Can't read from the ZIP file while there " 
        
                               "is an open writing handle on it. " 
        
                               "Close the writing handle before trying to read.") 
        
                   self._file.seek(self._pos) 
        
                   data = self._file.read(n) 
        
                   self._pos = self._file.tell() 
        
                   return data

seek should include something like self._file.seek(self._pos) like read does. This was hidden before the special cased ZIP_STORED seek as ZipExtFile.seek would use read to advance to the position if required (or seek back to the start of the file and read from there).

dimaryaz · 2024-12-13T06:04:15Z

Ah, got it.

I used an if/else, just to avoid doing two seeks unnecessarily - do you think that's better?

(Though an extra seek worked fine, too, so I can change it to that.)

danifus · 2024-12-14T11:16:34Z

I like the single seek you've done. Nice work!

(The CI tool failed for macos-13 but that looks unrelated to these changes. The failing test was: test.test_multiprocessing_forkserver.test_processes. I'm not sure if it needs to be rerun before it can be merged?)

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

dimaryaz · 2024-12-14T18:47:56Z

Just rebased, and everything passed now.

ZeroIntensity

LGTM, thanks for the fix :)

jaraco

LGTM. Thanks for the clean contribution and others for the thorough review.

miss-islington-app · 2024-12-24T15:56:46Z

Thanks @dimaryaz for the PR, and @jaraco for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

…onGH-127856) --------- (cherry picked from commit 7ed6c5c) Co-authored-by: Dima Ryazanov <dima@bucket.xxx> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>

bedevere-app · 2024-12-24T15:57:00Z

GH-128225 is a backport of this pull request to the 3.13 branch.

bedevere-app · 2024-12-24T15:57:04Z

GH-128226 is a backport of this pull request to the 3.12 branch.

…127856) (#128226) gh-127847: Fix position in the special-cased zipfile seek (GH-127856) --------- (cherry picked from commit 7ed6c5c) Co-authored-by: Dima Ryazanov <dima@bucket.xxx> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>

…127856) (#128225) gh-127847: Fix position in the special-cased zipfile seek (GH-127856) --------- (cherry picked from commit 7ed6c5c) Co-authored-by: Dima Ryazanov <dima@bucket.xxx> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>

ZeroIntensity · 2024-12-24T16:49:36Z

Congrats on your first contribution @dimaryaz

dimaryaz · 2024-12-24T17:18:38Z

Thanks everyone!

…on#127856) --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>

bedevere-app bot mentioned this pull request Dec 12, 2024

Data corruption when reading from two files in parallel in an uncompressed .zip #127847

Closed

bedevere-app bot added the awaiting review label Dec 12, 2024

ZeroIntensity reviewed Dec 12, 2024

View reviewed changes

Misc/NEWS.d/next/Library/2024-12-12-07-27-51.gh-issue-127847.ksfNKM.rst Outdated Show resolved Hide resolved

Lib/test/test_zipfile/test_core.py Outdated Show resolved Hide resolved

Lib/test/test_zipfile/test_core.py Outdated Show resolved Hide resolved

ZeroIntensity added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Dec 12, 2024

ZeroIntensity requested a review from jaraco December 12, 2024 13:39

dimaryaz requested a review from ZeroIntensity December 12, 2024 21:47

dimaryaz force-pushed the zipfile_position branch from b52d390 to 57af21a Compare December 12, 2024 22:12

danifus approved these changes Dec 14, 2024

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Dec 14, 2024

dimaryaz and others added 6 commits December 14, 2024 08:33

pythongh-127847: Fix position in the special-cased zipfile seek

8135237

📜🤖 Added by blurb_it.

3f8837a

move the blurb to the correct directory

fecb892

Apply suggestions from code review

bc2bd58

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

add more asserts

2835a4a

move the fix into _SharedFile

4cfd007

dimaryaz force-pushed the zipfile_position branch from 613945d to 4cfd007 Compare December 14, 2024 16:33

ZeroIntensity approved these changes Dec 15, 2024

View reviewed changes

danifus mentioned this pull request Dec 22, 2024

random access uncompressed unencrypted ZipExtFile #128131

Closed

jaraco approved these changes Dec 24, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Dec 24, 2024

Merge branch 'main' into zipfile_position

1b97b59

jaraco enabled auto-merge (squash) December 24, 2024 15:34

jaraco merged commit 7ed6c5c into python:main Dec 24, 2024

bedevere-app bot removed the awaiting merge label Dec 24, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Dec 24, 2024

bedevere-app bot removed the needs backport to 3.12 only security fixes label Dec 24, 2024

dimaryaz deleted the zipfile_position branch December 24, 2024 17:18

danifus mentioned this pull request May 20, 2025

zipfile may raise EOFError too early #101911

Closed

Uh oh!

Conversation

dimaryaz commented Dec 12, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

bedevere-app bot commented Dec 12, 2024

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danifus commented Dec 13, 2024

Uh oh!

dimaryaz commented Dec 13, 2024

Uh oh!

danifus commented Dec 14, 2024

Uh oh!

dimaryaz commented Dec 14, 2024

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

jaraco left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-app bot commented Dec 24, 2024

Uh oh!

bedevere-app bot commented Dec 24, 2024

Uh oh!

bedevere-app bot commented Dec 24, 2024

Uh oh!

ZeroIntensity commented Dec 24, 2024

Uh oh!

dimaryaz commented Dec 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dimaryaz commented Dec 12, 2024 •

edited by bedevere-app bot

Loading

ghost commented Dec 12, 2024 •

edited by ghost

Loading