Skip to content

fix: consume the whole extended timestamp field in the central header - #1016

Merged
Its-Just-Nans merged 3 commits into
zip-rs:masterfrom
youdie006:central-timestamp-trailing-bytes
Sep 29, 2026
Merged

Its-Just-Nans merged 3 commits into
zip-rs:masterfrom
youdie006:central-timestamp-trailing-bytes

Conversation

@youdie006

Copy link
Copy Markdown
Contributor

#1002 split out parse_central_header, which reads the modification time only when the field is exactly 4 bytes after the flags and reads nothing otherwise. ExtraFields::parse walks one cursor across all fields, so a central 0x5455 field of length 9 or 13 leaves 8 or 12 bytes that are read as the next field's header. Such fields come from any writer that copies the local field into the central directory, including this crate:

let mut o = FullFileOptions::default();
o.add_extra_data(0x5455, &[0x03, 0x00, 0xF1, 0x53, 0x65, 0x01, 0xF1, 0x53, 0x65], false)?;
// start_file, write, finish, then ZipArchive::new on the result:
// master:   Err(invalid Zip archive: Extra field content truncated)
// b5634497~1 and this PR: opens, mod_time == Some(1700000000)

An archive written by Python's zipfile with the same field fails the same way. This reads the modification time when there are at least 4 bytes and skips the rest, as parse_local_header already does with the same comment. The regression is not in a release yet (latest is 9.0.0-pre3).

One behaviour to note: before #1002 a length-9 central field also reported accessed; after this it reports None, as #1002 intended for the central header.

Verification

Base c5e58bcc. New case in the existing test_extended_timestamp_in_central_header.

row extended_timestamp.rs md5 result
master 8ba15635 new case fails: mod_time is None
this PR ab90196a passes
without the skip add3d132 fails: cursor at 5, not 9
== instead of >= b2c57928 fails on mod_time
> instead of >= 32f50a48 fails #1002's own length-5 case

On stable: cargo fmt --all -- --check; cargo clippy --all-targets -- -D warnings with --no-default-features, default and --all-features; cargo test --all with each of the three (35 result blocks each, 0 failed); cargo doc --no-deps with default and --all-features. On 1.88 (MSRV): cargo build with --no-default-features and default. cargo doc --no-default-features reports two unresolved intra-doc links to CompressionMethod::Bzip2 and ::Deflated, identical on master. Not run: nightly, Windows and macOS, Miri, wasm and the fuzz job.

Written with AI assistance (Claude); the measurements above were run locally and I have reviewed the change.

parse_central_header read the modification time only when the field was
exactly 4 bytes after the flags, and read nothing otherwise. ExtraFields
parses every field from one cursor, so a central 0x5455 field of length 9 or
13 (a writer that copies the local field, which Python's zipfile and
FullFileOptions::add_extra_data both do) left 8 or 12 bytes that were then
read as the next field header, and ZipArchive::new failed with "Extra field
content truncated". Read the modification time when present and skip the
rest, as parse_local_header already does.
Copilot AI lite review requested due to automatic review settings September 27, 2026 15:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The parsing fix and regression coverage are complete with no unresolved blocking issues.

Review effort: Lite
Findings: None

What changed in this PR

Fixes central-directory extended timestamp parsing to consume the complete field while preserving the modification time.

Changes:

  • Reads timestamps from fields at least four bytes long.
  • Skips trailing bytes to preserve parser alignment.
  • Adds regression coverage for extended fields.
File Description
src/​extra_fields/​extended_timestamp.rs Fixes central-header parsing and adds regression tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Signed-off-by: n4n5 <git@n4n5.dev>
Copilot AI review requested due to automatic review settings September 28, 2026 13:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings September 28, 2026 13:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Its-Just-Nans
Its-Just-Nans added this pull request to the merge queue Sep 29, 2026
Merged via the queue into zip-rs:master with commit a4da533 Sep 29, 2026
134 checks passed
@Pr0methean Pr0methean mentioned this pull request Sep 29, 2026
@Its-Just-Nans

Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants