Zipfile lib overwrites the extra field during closing when the archive size is more then ZIP64_LIMIT

[shaanbhaya]

BPO	44067
Nosy	@Yhg1s, @serhiy-storchaka
Files	zip_bug.zip

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2021-05-07.08:18:55.383>
labels = ['type-bug', '3.8', '3.9', '3.10', '3.11', '3.7', 'library']
title = 'Zipfile lib overwrites the extra field during closing when the archive size is more then ZIP64_LIMIT'
updated_at = <Date 2021-11-07.00:34:59.007>
user = 'https://bugs.python.org/shaanbhaya'

bugs.python.org fields:

activity = <Date 2021-11-07.00:34:59.007>
actor = 'anadius'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2021-05-07.08:18:55.383>
creator = 'shaanbhaya'
dependencies = []
files = ['50025']
hgrepos = []
issue_num = 44067
keywords = []
message_count = 3.0
messages = ['393178', '393179', '405760']
nosy_count = 5.0
nosy_names = ['twouters', 'alanmcintyre', 'serhiy.storchaka', 'shaanbhaya', 'anadius']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue44067'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11']

Linked PRs

• gh-88233: zipfile: handle extras after a zip64 extra #96161
• gh-88233: zipfile: refactor _strip_extra #102083
• gh-88233: zipfile: refactor _strip_extra #102084
• [3.11] gh-88233: zipfile: handle extras after a zip64 extra (GH-96161) #102086
• [3.10] gh-88233: zipfile: handle extras after a zip64 extra (GH-96161) #102087

[shaanbhaya]

The current zipFile implementation supports the allowZip64,which can make large zip files.
There is a bug in the current implementation of close method, where the extra field is overwritten.

To reproduce it:

1. Create a directory with more then 4 GB of data(spread over many files).
2. Make the zip of those files using the any rar achiever which adds NTFS metadata (mtime, atime, and ctime) of files in the zip.
3. Append a new file to the zip using the zip library .

When I open the zip again, the files processed after the ZIP64_LIMIT is reached will have their extra fields overwritten.

I have attached the zip that contained the python used to add the new file and the images of zip archive before adding new files and after.

[shaanbhaya]

The issue stems from the following code inside the
def _write_end_record(self): method ,where the extra fields are trimmed .

if zinfo.header_offset > ZIP64_LIMIT:
    extra.append(zinfo.header_offset)
    header_offset = 0xffffffff
else:
    header_offset = zinfo.header_offset

extra_data = zinfo.extra
min_version = 0
if extra:
    # Append a ZIP64 field to the extra's
    extra_data = _strip_extra(extra_data, (1,))
    extra_data = struct.pack(
        '<HH' + 'Q'*len(extra),
        1, 8*len(extra), *extra) + extra_data

min_version = ZIP64_VERSION

[anadius]

I was looking at zipfile._strip_extra trying to figure out how it works. It doesn't. It skips extra headers after the last one that matches. That's what causes this issue.

Here's a fixed version:

def _strip_extra(extra, xids):
    # Remove Extra Fields with specified IDs.
    unpack = _EXTRA_FIELD_STRUCT.unpack
    modified = False
    buffer = []
    start = i = 0
    while i + 4 <= len(extra):
        xid, xlen = unpack(extra[i : i + 4])
        j = i + 4 + xlen
        if xid in xids:
            if i != start:
                buffer.append(extra[start : i])
            start = j
            modified = True
        i = j
    if i != start:
        buffer.append(extra[start : i])
    if not modified:
        return extra
    return b''.join(buffer)

Or this one, easier to understand:

def _strip_extra(extra, xids):
    # Remove Extra Fields with specified IDs.
    unpack = _EXTRA_FIELD_STRUCT.unpack
    modified = False
    buffer = []
    i = 0
    while i + 4 <= len(extra):
        xid, xlen = unpack(extra[i : i + 4])
        j = i + 4 + xlen
        if xid in xids:
            modified = True
        else:
            buffer.append(extra[i : j])
        i = j
    if not modified:
        return extra
    return b''.join(buffer)

Not sure which one is better.

[thatch]

I found the same bug while working on another issue. I prefer the easier-to-understand one.

[thatch]

I ended up not going with the easier-to-understand version, because it failed to pass through 1-3 bytes of garbage after the last extra, which we pass through in the "not modified" case already.

[jaraco]

I ended up not going with the easier-to-understand version, because it failed to pass through 1-3 bytes of garbage after the last extra, which we pass through in the "not modified" case already.

In an attempt to make the code easier to comprehend and less prone to error, I drafted #102084.