Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

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

It probably would be better to raise an attributeError instead of a valueError here since you are trying to access an attribute a closed zipfile doesn’t have

This behavior simply resemblesopen() andwrite(), which raises aValueError in various cases. Furthermore there has been a change from raisingRuntimeError since Python 3.6:

Changed in version 3.6: Callingopen() on a closed ZipFile will raise aValueError. Previously, aRuntimeError was raised.

Changed in version 3.6: Callingwrite() on a ZipFile created with mode 'r' or a closed ZipFile will raise aValueError. Previously, aRuntimeError was raised.

Nicely inform@ubershmekel,@barneygale,@merwok, and@wimglenn about this PR. This should be more desirable and flexible than the previous PR, although cares must be taken as there might be a potential risk on the algorithm about reclaiming spaces.

The previous PR is kept open in case some folks are interested in it. Will close when either one is accepted.

Sorry! I didn’t type that. I stepped away from the laptop and my friend who has a bad sense of humor but knows his way around GitHub typed that. Deleting those comments. I just found out from these emails!

@danny0838 thank you for sticking with this issue! Would love to see removal support for zipfiles.

I'm a bit concerned aboutrepack being overly restrictive. Perhaps ZipFile could track the offsets of removed entries and use that information to allow for repack to handle arbitrary zips? In this scenario, repack would scan for local file header signatures, and check the offset against the central directory and list of deleted ZipInfos. If it is in neither, data is left as is and scanning continues. If it is in the deleted list, the file is dropped, if it is in the central directory, then it is kept.

@emmatyping

@danny0838 thank you for sticking with this issue! Would love to see removal support for zipfiles.

I'm a bit concerned aboutrepack being overly restrictive. Perhaps ZipFile could track the offsets of removed entries and use that information to allow for repack to handle arbitrary zips? In this scenario, repack would scan for local file header signatures, and check the offset against the central directory and list of deleted ZipInfos. If it is in neither, data is left as is and scanning continues. If it is in the deleted list, the file is dropped, if it is in the central directory, then it is kept.

Added support ofrepack(removed) to pass a sequence of removedZipInfos, and reclaims space only for corresponding local file headers. So one can do something like:

with zipfile.ZipFile('archive.zip', 'a') as zh:    zinfos = [zh.remove(n) for n in zh.namelist() if n.startswith('folder/')]    zh.repack(zinfos)

Though the result should not change in most cases, except for being more performant by eliminating the need to scan file entries in the bytes.

Is there still any problem about this PR?

Here are something that may need further consideration/discussion:

1. Shouldstrict_descriptor option forrepack() default to True or False?

Summary of trade-offs

Background

When a local file entry has the flag bit indicating usage of data descriptor:

1. This method first attempts to scan for a signed data descriptor.
2. If no valid one is found:
   1. For supported compression methods (ZIP_DEFLATED,ZIP_BZIP2, orZIP_ZSTANDARD), it decompresses the data to find its end offset.
   2. Otherwise it performs a byte-by-byte scan for an unsigned data descriptor.

This option only affects case2.2, which is used when neither signed descriptor nor decompression-based validation is applicable.

Performance comparison

Based on the benchmark (see tests intest_zipfile64.py):

• 8 GiBZIP_STORED file with signed data descriptor: ~56.7s
• 400 MiBZIP_STORED file with unsigned data descriptor: ~270s

The latter is over150× slower due to the byte-by-byte scanning for a valid data descriptor.

This may also raise a security concern sincestrict_descriptor=False may open a path for a DoS Attack (if an attacker crafts a ZIP file with offensive entries).

False-positive risk

It's not possible to guarantee the "real" file size of a local entry with a data descriptor without the information from the central directory.

If a local file entry spans 100 MiB, it's theoretically possible that multiple byte ranges (e.g., the first 20 MiB, 30 MiB, etc.) could each appear as valid data + data descriptor segments with differing CRCs and compressed sizes. (Currently, the algorithm validates only the compressed size. Checking for CRCs could reduce false positives but would significantly deteriate performance.) The byte-by-byte validation can increase the risk of a false positive compared to the signature or decompression based validation, which only checks for certain points and has more prerequisites.

A false positive should be unlikely to happen in practice. If it were to happen, a stale local file entry is stripped incompletely (e.g. a 30MiB entry be treated as 20MiB, leaving 10MiB random bytes over) and cause following entries not stripped (since the algorithm requires consecutive entries). However, the ZIP file remains uncorrupted.

Spec compliance

According to theZIP file format specification: Applications SHOULD write signed descriptors and SHOULD support both forms when reading.

Unsigned descriptors are thus consideredlegacy, but it is unclear whether they are still used widely.

strict_descriptor=True adheres less strictly to the spec, but doesnot violate it — because stripping is neither reading nor writing, and a suboptimal stripping does not corrupt the ZIP archive.

2. Should we also implementcopy() forZipFile?

Currently, copying an entry within a ZIP file is cumbersome due to the lack of support for simultaneous reading and writing. The implementer must either:

• Read the entire entry and write afterwards (which is memory-intensive and inefficient for large files), or
• Use a temporary file for buffered copying.

Both approaches are more complex and less performant, due to the need to decompress and recompress data.

If would be much more performant and friendly by implementing acopy() method, using the similar internal buffered copying technique that_ZipRepacker has used.

Additionally, this also opens the door to support an efficientmove() operation, composed ofcopy(),remove(), and optionallyrepack().

And an additional question: whether this should be included in the current PR, or proposed separately as a follow-up?

A higher level question: Why would we want to maintain advanced features that are not used by most people within the standard library at all? This code existing in the stdlib creates a maintenance burden and is the slowest possible way to get features and their resulting trail of bugfixes to people who might want them. All features have an ongoing cost.

Is there a realneed for these zipfile features to not simply be advanced ones available in a PyPI package?@jaraco FYI

I appreciate the enthusiasm for implementing something interesting. I'm not sure we actually want to maintain that within the CPython project though. Are there compelling use cases for these features to be part of Python rather than external?

(edit: posted this on the Issue, leaving here for posterity)

@gpshead Don't you think your question should be raised in the issue thread rather than here? 🫠

good point, moved, thanks!(where to have what discussions is an area where I consider github's UX to be... not great)