Repository navigation
ZipFile.close() writes wrong offsets after a failed repack() #156434
Description
Bug description:
_ZipRepacker.repack() updates ZipInfo.header_offset for each member before moving that member's bytes, so a failing _copy_bytes() leaves the in-memory offsets describing a layout that was never written. remove() has already set _didModify, so a later normal close() commits a central directory built from those offsets: the caller handles the OSError, closes cleanly, and gets an archive that zipfile itself cannot read.
import io, zipfile
class Flaky(io.BytesIO):
countdown = None
def write(self, b):
if self.countdown is not None:
self.countdown -= 1
if self.countdown < 0:
raise OSError(28, 'No space left on device')
return super().write(b)
buf = io.BytesIO()
with zipfile.ZipFile(buf, 'w') as zf:
for c in 'abcd':
zf.writestr(c + '.txt', c.upper().encode() * 5000)
flaky = Flaky(buf.getvalue())
with zipfile.ZipFile(flaky, 'a') as zf:
flaky.countdown = 1
try:
zf.repack([zf.remove('b.txt')], chunk_size=4096)
except OSError as exc:
print('repack raised:', exc)
flaky.countdown = None # space freed; the caller closes normally
with zipfile.ZipFile(flaky) as zf:
print('testzip:', zf.testzip())repack raised: [Errno 28] No space left on device
testzip: c.txt
Expected: after the caller has handled the OSError, close() should either leave a readable archive or raise, rather than committing offsets that no write produced.
remove() and repack() are new in 3.16, so no released version is affected.
CPython versions tested on:
CPython main branch
Operating systems tested on:
Linux
Activity
danny0838 commentedon Oct 12, 2026
Do you have any real-world scenario of a failed repack in mind? I wonder if the
No space left on deviceerror is possible since the repack is shrinking a file.Strictly speaking, a repack could fail after some entries have been moved but before the remaining entries have been processed. In that case, restoring the original header offsets alone would not necessarily restore the archive to a consistent state.
Also,
repacksets_didModifyafter moving all entries, and it would not be set if an exception is raised during moving physical entries. (But may be set beforehand byremove)More generally, in-place archive modifications are not inherently atomic. This applies even to a simple
ZipFile.write(). If we want to guarantee that a failed modification leaves the original archive intact, we would need a transactional approach, such as writing to a temporary file and replacing the original only after the operation succeeds. However, this would probably be out of scope for thezipfilelibrary.fedonman commentedon Nov 3, 2026
Thanks, you're right that restoring the offsets is wrong once an entry has been moved: if the move overlapped its old position, the restored offset points at overwritten data. I've changed the PR to update each entry's offset only after its bytes are moved instead, so entries that finished moving or were never touched stay readable and only the one being moved can be lost. I agree that making it atomic is out of scope.
The case I had in mind is Ctrl-C or an I/O error during a long repack inside a
withblock:__exit__callsclose(), which writes the central directory becauseremove()already set_didModify.danny0838 commentedon Nov 4, 2026
Thank you. The revised version seems to be a nice way to reduce damage for an incomplete repack.
It seems that we should also set
_didModifybefore moving any entry, otherwise a standalone repack that fails in middle won't update the central directory.