Update central directory bounds when replacing its stream - #1064
Conversation
WalkthroughThe PR updates ChangesCentral directory stream handling
Suggested reviewers: Priority: ➖ Normal Change: Bug fix · Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to 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)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Remove or simplify unit tests, it is overkill. |
|
@nmoinvaz I've reduced this to one test that still catches the original regression. Is this simple enough? |
|
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. |
|
Fair enough, I've removed the test |
|
Needs rebase thanks. |
3a65992 to
4176030
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 27e1f43d-2f30-4bcb-8b4a-fefbb0de228a
📒 Files selected for processing (2)
mz_zip.cmz_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.
Derive replacement central directory size from stream end
Hello! This PR addresses the stale central directory bounds discussed in #1063.
Problem
mz_zip_set_cd_streamreplaces the stream and its start position but keeps the previouscd_size. With a larger replacement directory, sequential iteration can succeed whilemz_zip_goto_entry()rejects valid offsets withMZ_PARAM_ERROR.Fix
Update
cd_sizetostream_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 backbecause 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