Alter ZipFile.data_offset calculation

[danifus]

Bug report

Bug description:

Zip implementations interpret the zip appnote with regards to offset fields in the central directory differently centDirOffset (offset of start of central directory with respect to the starting disk number) and relative offset of local header. Some implementations will store the number of bytes from the first local header (eg. 7zip - I tested by adding self extracting content with 7zz) while others will store the number of bytes from the start of the file (eg. infozip, python's zipfile which I didn't expect!). The two approaches only produce different results when data is prepended to an archive.

When prepended data is present, ZipFile.data_offset will contain different values for the different implementations.

Taking the example provided by @danny0838 in this comment: #134627 (comment)

with open('archive.zip', 'wb') as fh:
    fh.write(b'dummy')
    with zipfile.ZipFile(fh, 'a') as zh:
        zh.writestr('file', 'file content')

with zipfile.ZipFile('archive.zip') as zh:
    print(zh.data_offset)  # 0
    print(zh.infolist()[0].header_offset)  # 5
    print(zh.start_dir)  # 51

zh.data_offset should be 5

Related: #84481

CPython versions tested on:

3.14

Operating systems tested on:

macOS

Linked PRs

• gh-136170: Use earliest zinfo.header_offset as ZipFile.data_offset #136171
• gh-136170: Revert adding ZipFile.data_offset #136950
• [3.14] gh-136170: Revert adding ZipFile.data_offset (GH-136950) #136955

[danny0838]

Thank you. I was considering raising an issue for this.

I think we have to first determine what the main purpose (or expected use case) of data_offset is. Seeing the original issue #84481 the data_offset can be used to detect whether a prefixed (potentially malware) payload exists, in which case the zip file can have bytes before data_offset stripped safely when it's non-zero.

1. central directory offsets calculated from the first local header (can strip bytes 0..5)

   with open('archive.zip', 'wb') as fh:
       fh.write(b'dummy')
       with zipfile.ZipFile(fh, 'a') as zh:
           zh.writestr('file', 'file content')
   
   with zipfile.ZipFile('archive.zip') as zh:
       print(zh.data_offset)  # 5
       print(zh.filelist[0].header_offset)  # 5
       print(zh.start_dir)  # 51

2. central directory offsets calculated from the start of the file (cannot strip bytes 0..5)

   with open('archive.zip', 'wb') as fh:
       with zipfile.ZipFile(fh, 'a') as zh:
           fh.write(b'dummy')
           zh.start_dir = fh.tell()
           zh.writestr('file', 'file content')
   
   with zipfile.ZipFile('archive.zip') as zh:
       print(zh.data_offset)  # 0
       print(zh.filelist[0].header_offset)  # 5
       print(zh.start_dir)  # 51

The proposed PR 136171 will break the feature since case 2 cannot be stripped in the same way as case 1.

When stripping case 2, the resulting zip file can be read by zipfile, with a negative concat value. However, the support of other zip apps varies. For example, 7z supports it while Windows file explorer does not.

OTOH, I don't see there are too many potential useful case for the proposed data_offset, and would tend to simply remove it since the developer can simply calculate the value on the fly when it's really needed (like what zipfile.repack has done).

[danny0838]

Maybe we should ask @emmatyping, the original PR provider, about the intention of the implementation of this API.

[emmatyping]

Since 3.14rc1 is tomorrow and the interpretation of the attribute is unclear, I decided to simply revert the addition: #136950

[danny0838]

Should the original issue #84481 be reopened? [done]