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

[dimaryaz]

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

[ghost]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

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.

[danifus]

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]

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]

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

[dimaryaz]

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]

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

[bedevere-app]

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

[bedevere-app]

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

[ZeroIntensity]

Congrats on your first contribution @dimaryaz

[dimaryaz]

Thanks everyone!