Skip to content

Update central directory bounds when replacing its stream - #1064

Merged
nmoinvaz merged 2 commits into
zlib-ng:developfrom
igooor-bb:fix-catalog-bounds
Sep 16, 2026
Merged

nmoinvaz merged 2 commits into
zlib-ng:developfrom
igooor-bb:fix-catalog-bounds

Conversation

@igooor-bb

@igooor-bb igooor-bb commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Hello! This PR addresses the stale central directory bounds discussed in #1063.

Problem

mz_zip_set_cd_stream replaces the stream and its start position but keeps the previous cd_size. With a larger replacement directory, sequential iteration can succeed while mz_zip_goto_entry() rejects valid offsets with MZ_PARAM_ERROR.

Fix

Update cd_size to stream_end - cd_start_pos, treating the replacement directory as extending to the stream's end.

The setter validates the bounds, restores the stream position and updates the ZIP fields only after success. It uses tell -> seek to end -> tell -> seek back because the stream API has no general length property. Adding one would require defining its semantics and implementing support across stream types, making this a broader change.

The original setter did not move the stream position, so the fix saves and restores it to avoid introducing an unexpected side effect for callers.

Validation

I kept two regression tests for replacing the directory at zero and nonzero offsets. They check that valid entries remain accessible, offsets beyond the new directory are rejected, and the stream position is preserved. The tests focus on the changed behavior rather than every possible input or error path.

All 69 stock CTest checks passed on macOS.

Closes #1063

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

Walkthrough

The PR updates mz_zip_set_cd_stream to validate stream positions, preserve the current stream position, propagate stream errors, reject invalid bounds, and compute cd_size. The API documentation now defines the central directory range.

Changes

Central directory stream handling

Layer / File(s) Summary
Stream validation and sizing
mz_zip.c, mz_zip.h
mz_zip_set_cd_stream validates cd_start_pos, determines the stream end, restores the saved position, propagates errors, rejects out-of-range positions, and stores cd_size. The API comment describes reading through the stream end.

Suggested reviewers: nmoinvaz

Priority: ➖ Normal

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: 🔵 Low · up to 6055a

The central-directory sizing fix is not protected against regressions when a replacement stream has a different size. Add the focused regression test before merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: recalculating central directory bounds when replacing its stream.
Description check ✅ Passed The description directly explains the stale central directory bounds, the fix, validation behavior, and test results.
Linked Issues check ✅ Passed The change satisfies #1063. mz_zip_set_cd_stream measures the replacement stream end, restores the original stream position, rejects invalid bounds, and sets zip->cd_size to `stream_end - cd_start…
Out of Scope Changes check ✅ Passed The changes stay within #1063. The added validation, position restoration, error handling, central-directory bound update, and API comment support safe replacement-stream handling. No unrelated source…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nmoinvaz

Copy link
Copy Markdown
Member

Remove or simplify unit tests, it is overkill.

@igooor-bb

Copy link
Copy Markdown
Contributor Author

@nmoinvaz I've reduced this to one test that still catches the original regression. Is this simple enough?

@nmoinvaz nmoinvaz added the improvement Change for the better label Sep 16, 2026
@nmoinvaz

nmoinvaz commented Sep 16, 2026

Copy link
Copy Markdown
Member

I don't want the unit test at all. I don't see it as having enough value. And it looks complicated, doesn't use a fixture, introduces a new test file with a name that I'm unsure about, etc. I just don't want to deal with going through all that.

@igooor-bb

Copy link
Copy Markdown
Contributor Author

Fair enough, I've removed the test

@nmoinvaz

Copy link
Copy Markdown
Member

Needs rebase thanks.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 27e1f43d-2f30-4bcb-8b4a-fefbb0de228a

📥 Commits

Reviewing files that changed from the base of the PR and between 3a65992 and 6055a9b.

📒 Files selected for processing (2)
  • mz_zip.c
  • mz_zip.h
🚧 Files skipped from review as they are similar to previous changes (1)
  • mz_zip.h

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

@zlib-ng zlib-ng deleted a comment from coderabbitai Bot Sep 16, 2026
@nmoinvaz
nmoinvaz merged commit a5e2eef into zlib-ng:develop Sep 16, 2026
34 checks passed
nmoinvaz pushed a commit that referenced this pull request Sep 16, 2026
Derive replacement central directory size from stream end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Change for the better

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mz_zip_set_cd_stream keeps stale central directory bounds

2 participants