From 130e1dd7b93d7496323b012cffa66088d725ea62 Mon Sep 17 00:00:00 2001 From: Vyron Vasileiadis Date: Thu, 27 Aug 2026 01:27:41 +0300 Subject: [PATCH 1/3] gh-156434: Restore entry offsets when `repack()` fails _ZipRepacker.repack() updates ZipInfo.header_offset for each member before moving that member's bytes, so a failing _copy_bytes() left the in-memory offsets describing a layout that was never written. remove() has already set _didModify, so a later close() committed a central directory built from those offsets, and the archive became unreadable even though the caller had handled the error. ZipFile.repack() now snapshots the offsets of the surviving and the removed entries and restores them if the repack raises. The copy only ever moves data to lower offsets, so a partial move never overwrites its own source and the archive falls back to the state of a remove() with no repack(). --- Lib/test/test_zipfile/test_core.py | 43 ++++++++++++++++++++++++++++++ Lib/zipfile/__init__.py | 6 +++++ 2 files changed, 49 insertions(+) diff --git a/Lib/test/test_zipfile/test_core.py b/Lib/test/test_zipfile/test_core.py index fdf2cd26f8c7c64..3eae62e09681d0f 100644 --- a/Lib/test/test_zipfile/test_core.py +++ b/Lib/test/test_zipfile/test_core.py @@ -2662,6 +2662,49 @@ def test_overlap_with_archive_comment(self): fz.seek(0) self.assertEqual(fz.read(), expected) + def test_repack_failed_write_restores_header_offsets(self): + # A failed repack() must leave the in-memory offsets describing the + # file that is actually on disk, so that a later close() does not + # commit a central directory pointing at data that was never written. + class FlakyBytesIO(io.BytesIO): + countdown = None + + def write(self, data): + if self.countdown is not None: + self.countdown -= 1 + if self.countdown < 0: + raise OSError(errno.ENOSPC, 'No space left on device') + return super().write(data) + + names = ['a.txt', 'b.txt', 'c.txt', 'd.txt'] + buf = io.BytesIO() + with zipfile.ZipFile(buf, 'w') as zh: + for name in names: + zh.writestr(name, name[0].upper().encode() * 5000) + + fz = FlakyBytesIO(buf.getvalue()) + with zipfile.ZipFile(fz, 'a') as zh: + zi = zh.remove('b.txt') + expected = [z.header_offset for z in zh.infolist()] + expected_removed = zi.header_offset + + fz.countdown = 1 + with self.assertRaises(OSError): + zh.repack([zi], chunk_size=4096) + + self.assertEqual([z.header_offset for z in zh.infolist()], expected) + self.assertEqual(zi.header_offset, expected_removed) + + # The archive is still usable once the write error clears. + fz.countdown = None + + fz.seek(0) + with zipfile.ZipFile(fz) as zh: + self.assertIsNone(zh.testzip()) + self.assertEqual(zh.namelist(), ['a.txt', 'c.txt', 'd.txt']) + for name in zh.namelist(): + self.assertEqual(zh.read(name), name[0].upper().encode() * 5000) + class ZipRepackerTests(unittest.TestCase): def _generate_local_file_entry(self, arcname, raw_bytes, compression=zipfile.ZIP_STORED, diff --git a/Lib/zipfile/__init__.py b/Lib/zipfile/__init__.py index 0accf324c90e3fd..468b847cada1c71 100644 --- a/Lib/zipfile/__init__.py +++ b/Lib/zipfile/__init__.py @@ -2434,12 +2434,18 @@ def repack(self, removed=None, *, strict_descriptor=True, ) self._writing = True + header_offsets = [(zinfo, zinfo.header_offset) + for zinfo in (*self.filelist, *(removed or ()))] try: repacker = _ZipRepacker( strict_descriptor=strict_descriptor, chunk_size=chunk_size, ) repacker.repack(self, removed) + except BaseException: + for zinfo, header_offset in header_offsets: + zinfo.header_offset = header_offset + raise finally: self._writing = False From cbcaea10cf5ff5e6da5b3238b3117e84eb9c6850 Mon Sep 17 00:00:00 2001 From: Vyron Vasileiadis Date: Fri, 28 Aug 2026 20:51:02 +0300 Subject: [PATCH 2/3] Address review: materialize removed so a one-shot iterable still repacks --- Lib/test/test_zipfile/test_core.py | 18 ++++++++++++++++++ Lib/zipfile/__init__.py | 5 +++++ 2 files changed, 23 insertions(+) diff --git a/Lib/test/test_zipfile/test_core.py b/Lib/test/test_zipfile/test_core.py index 3eae62e09681d0f..12c1977cff9d895 100644 --- a/Lib/test/test_zipfile/test_core.py +++ b/Lib/test/test_zipfile/test_core.py @@ -2662,6 +2662,24 @@ def test_overlap_with_archive_comment(self): fz.seek(0) self.assertEqual(fz.read(), expected) + def test_repack_removed_iterator(self): + # A one-shot iterable of removed entries must still be repacked. + buf = io.BytesIO() + with zipfile.ZipFile(buf, 'w') as zh: + for name in ['a.txt', 'b.txt', 'c.txt']: + zh.writestr(name, name[0].upper().encode() * 5000) + full_size = len(buf.getvalue()) + + fz = io.BytesIO(buf.getvalue()) + with zipfile.ZipFile(fz, 'a') as zh: + zh.repack(iter([zh.remove('b.txt')])) + + self.assertLess(len(fz.getvalue()), full_size - 5000) + fz.seek(0) + with zipfile.ZipFile(fz) as zh: + self.assertIsNone(zh.testzip()) + self.assertEqual(zh.namelist(), ['a.txt', 'c.txt']) + def test_repack_failed_write_restores_header_offsets(self): # A failed repack() must leave the in-memory offsets describing the # file that is actually on disk, so that a later close() does not diff --git a/Lib/zipfile/__init__.py b/Lib/zipfile/__init__.py index 468b847cada1c71..60db70777711ee5 100644 --- a/Lib/zipfile/__init__.py +++ b/Lib/zipfile/__init__.py @@ -2434,6 +2434,11 @@ def repack(self, removed=None, *, strict_descriptor=True, ) self._writing = True + # *removed* is read twice below, so a one-shot iterable must not + # be consumed by the first read. None (scan the archive) is not + # the same as an empty sequence, so keep it as is. + if removed is not None: + removed = list(removed) header_offsets = [(zinfo, zinfo.header_offset) for zinfo in (*self.filelist, *(removed or ()))] try: From 48679d27689bac015ea4002c6974b04c0f9e147d Mon Sep 17 00:00:00 2001 From: Vyron Vasileiadis Date: Tue, 3 Nov 2026 16:21:04 +0200 Subject: [PATCH 3/3] Address review: update offsets only after moving each entry Restoring every offset after a failed repack() is wrong once an entry has been moved over its old position: the restored offset then points at overwritten data. Instead, set each entry's header_offset only after its bytes have been copied, so entries that finished moving or were never touched stay readable. This also drops the list(removed) copy, which was only needed because the rollback read removed a second time. --- Lib/test/test_zipfile/test_core.py | 80 ++++++++++++------------------ Lib/zipfile/__init__.py | 23 +++------ 2 files changed, 38 insertions(+), 65 deletions(-) diff --git a/Lib/test/test_zipfile/test_core.py b/Lib/test/test_zipfile/test_core.py index 82b703030255e7c..0edfa138acb1f05 100644 --- a/Lib/test/test_zipfile/test_core.py +++ b/Lib/test/test_zipfile/test_core.py @@ -2664,28 +2664,10 @@ def test_overlap_with_archive_comment(self): fz.seek(0) self.assertEqual(fz.read(), expected) - def test_repack_removed_iterator(self): - # A one-shot iterable of removed entries must still be repacked. - buf = io.BytesIO() - with zipfile.ZipFile(buf, 'w') as zh: - for name in ['a.txt', 'b.txt', 'c.txt']: - zh.writestr(name, name[0].upper().encode() * 5000) - full_size = len(buf.getvalue()) - - fz = io.BytesIO(buf.getvalue()) - with zipfile.ZipFile(fz, 'a') as zh: - zh.repack(iter([zh.remove('b.txt')])) - - self.assertLess(len(fz.getvalue()), full_size - 5000) - fz.seek(0) - with zipfile.ZipFile(fz) as zh: - self.assertIsNone(zh.testzip()) - self.assertEqual(zh.namelist(), ['a.txt', 'c.txt']) - - def test_repack_failed_write_restores_header_offsets(self): - # A failed repack() must leave the in-memory offsets describing the - # file that is actually on disk, so that a later close() does not - # commit a central directory pointing at data that was never written. + def test_repack_write_error_keeps_entries_readable(self): + # close() after a failed repack() writes the in-memory offsets to + # the central directory, so an entry's offset must only change once + # its data has been moved. class FlakyBytesIO(io.BytesIO): countdown = None @@ -2696,34 +2678,34 @@ def write(self, data): raise OSError(errno.ENOSPC, 'No space left on device') return super().write(data) - names = ['a.txt', 'b.txt', 'c.txt', 'd.txt'] - buf = io.BytesIO() - with zipfile.ZipFile(buf, 'w') as zh: - for name in names: - zh.writestr(name, name[0].upper().encode() * 5000) - - fz = FlakyBytesIO(buf.getvalue()) - with zipfile.ZipFile(fz, 'a') as zh: - zi = zh.remove('b.txt') - expected = [z.header_offset for z in zh.infolist()] - expected_removed = zi.header_offset - - fz.countdown = 1 - with self.assertRaises(OSError): - zh.repack([zi], chunk_size=4096) - - self.assertEqual([z.header_offset for z in zh.infolist()], expected) - self.assertEqual(zi.header_offset, expected_removed) - - # The archive is still usable once the write error clears. - fz.countdown = None + for sizes, countdown, readable in [ + # The error hits b, the first entry to move. The gap left by a + # is larger than one chunk, so b's old data is still intact. + ([5000, 5000, 5000, 5000], 1, 'bcd'), + # b has been moved when the error hits c. c is not checked: + # the gap left by a is small, so its move overwrote its old data. + ([100, 10000, 10000, 100], 4, 'bd'), + ]: + with self.subTest(sizes=sizes): + contents = {name: randbytes(size) + for name, size in zip('abcd', sizes)} + buf = io.BytesIO() + with zipfile.ZipFile(buf, 'w') as zh: + for name, data in contents.items(): + zh.writestr(name, data) + + fz = FlakyBytesIO(buf.getvalue()) + with zipfile.ZipFile(fz, 'a') as zh: + zi = zh.remove('a') + fz.countdown = countdown + with self.assertRaises(OSError): + zh.repack([zi], chunk_size=4096) + fz.countdown = None - fz.seek(0) - with zipfile.ZipFile(fz) as zh: - self.assertIsNone(zh.testzip()) - self.assertEqual(zh.namelist(), ['a.txt', 'c.txt', 'd.txt']) - for name in zh.namelist(): - self.assertEqual(zh.read(name), name[0].upper().encode() * 5000) + fz.seek(0) + with zipfile.ZipFile(fz) as zh: + for name in readable: + self.assertEqual(zh.read(name), contents[name]) class ZipRepackerTests(unittest.TestCase): def _generate_local_file_entry(self, arcname, raw_bytes, diff --git a/Lib/zipfile/__init__.py b/Lib/zipfile/__init__.py index 3b141ddb0fe2869..151179fa6d15a3f 100644 --- a/Lib/zipfile/__init__.py +++ b/Lib/zipfile/__init__.py @@ -1581,17 +1581,18 @@ def repack(self, zfile, removed=None): entry_size = entry_size_list[i] used_entry_size = used_entry_size_list[i] - # update the header and move entry data to the new position + # move entry data to the new position, then update the header old_header_offset = zinfo.header_offset - zinfo.header_offset -= entry_offset + new_header_offset = old_header_offset - entry_offset if zinfo in removed_zinfos: self._copy_bytes( fp, old_header_offset + used_entry_size, - zinfo.header_offset, + new_header_offset, entry_size - used_entry_size, ) + zinfo.header_offset = new_header_offset # update entry_offset for subsequent files to follow entry_offset += used_entry_size @@ -1601,7 +1602,7 @@ def repack(self, zfile, removed=None): self._copy_bytes( fp, old_header_offset, - zinfo.header_offset, + new_header_offset, used_entry_size, ) @@ -1615,12 +1616,13 @@ def repack(self, zfile, removed=None): self._copy_bytes( fp, old_header_offset + used_entry_size + stale_entry_size, - zinfo.header_offset + used_entry_size, + new_header_offset + used_entry_size, entry_size - used_entry_size - stale_entry_size, ) # update entry_offset for subsequent files to follow entry_offset += stale_entry_size + zinfo.header_offset = new_header_offset # update state zfile.start_dir -= entry_offset @@ -2441,23 +2443,12 @@ def repack(self, removed=None, *, strict_descriptor=True, ) self._writing = True - # *removed* is read twice below, so a one-shot iterable must not - # be consumed by the first read. None (scan the archive) is not - # the same as an empty sequence, so keep it as is. - if removed is not None: - removed = list(removed) - header_offsets = [(zinfo, zinfo.header_offset) - for zinfo in (*self.filelist, *(removed or ()))] try: repacker = _ZipRepacker( strict_descriptor=strict_descriptor, chunk_size=chunk_size, ) repacker.repack(self, removed) - except BaseException: - for zinfo, header_offset in header_offsets: - zinfo.header_offset = header_offset - raise finally: self._writing = False