zipAbandonFileInZip reports success when nothing was truncated - #1413
Merged
Conversation
The three Win32 filefunc tables in minizip's iowin32.c set neither ztruncate64_file nor zflush_file, so a caller filling one from an uninitialized struct is left with a wild pointer. Implement both over SetFilePointerEx/SetEndOfFile and FlushFileBuffers. Where a backend has no truncate at all the rollback stays a rewind, but zipAbandonFileInZip() now returns ZIP_NOTRUNCATED rather than ZIP_OK, and the cache logs the incomplete rollback instead of continuing quietly. The cache-writefail self-test opened its injected ZIP through the 32-bit zipOpen2, where the entry is NULL, so it never truncated and could not see this regress; it now uses the table the cache itself opens with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
zip-abandon-notrunc's "the reference was not truncated away" check compared against the member's nominal size, which an un-truncated archive never reaches: zip.c flushes a stored member in 64KB units, so the case it named sailed through. Bound it on the 64KB backscan, the size that matters here. _chsize_s returns an errno rather than -1, so a failed Windows truncate was labelled "this backend has no truncate" instead of a failure. Collapse any non-zero backend return to -1 in call_ztruncate64. The cache's incomplete-rollback warning ran in no test, since cache-writefail failed outright on a table with no truncate. It now also drives one such table and asserts the entry drops, the mirror lives, and the warning is printed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
zipAbandonFileInZip()rolls a partial cache member back by rewinding to its local header and truncating there, and both ends of that were incomplete. The Win32 filefunc tables insrc/minizip/iowin32.cset neitherztruncate64_filenorzflush_file, leaving a caller who fills one from an uninitialized struct with a wild pointer; and where a backend carries no truncate at all,call_ztruncate64()degrades to a no-op that the rollback then reported asZIP_OK. The caller keeps an archive whose leftover tail hides the central directory from any reader once it outgrows the 64KB backscan, and hears nothing about it.call_ztruncate64()also collapses any non-zero backend return to a failure, since_chsize_s()reports one as an errno rather than -1, which would otherwise read as "this backend has no truncate".This implements
win32_truncate64_file_func(SetFilePointerExplusSetEndOfFile, putting the file position back where the caller had it, asftruncateleaves it) andwin32_flush_file_func(FlushFileBuffers), wires both into all four tables, and adds aZIP_NOTRUNCATEDreturn for a rollback that could only rewind. The no-op degradation stays, since failing a rollback the caller has already performed is worse, but it is no longer silent: the cache logs the incomplete rollback. Of the three callers onlycache_zip_write_failed()ignored the return.iowin32.cgains the.orig/.diffpair the other patched minizip files carry, which is most of the added line count.-#test=cache-writefail, the self-test that drives the production abandon path, was opening its injected ZIP through the 32-bitzipOpen2where the entry is NULL, so it made zero truncate calls and could not have seen any of this regress. It now opens through the table the cache itself uses (factored out ashts_zip_filefunc64()) behind a counting interposer, and asserts both that the table carries a truncate and that the abandoned member went through it. It also drives a table carrying no truncate at all, to hold the deliberate degradation: the entry drops, the mirror lives, and the warning is printed. Test 356 drives a new-#test=zip-abandon-notrunc, building the same archive with and without the entry and grading the return, the leftover tail, and whether Python'szipfilestill opens it.The Win32 half is latent and unexercised by Linux CI: nothing in the tree fills those tables today, the engine reaches minizip through
hts_zipOpen_utf8on the 64-bit fopen table, andiowin32.conly ships inEXTRA_DISTfor the Windows projects, so no leg here compiles it. Being_WIN32-only it moves no POSIX ABI. I did compile it locally against a stubwindows.hand ran a truncate through the sequence the abandon uses: the file shortens, the position ends up where the caller left it, and the next write lands at the rewound offset. DroppingSetEndOfFileand dropping the position restore each red that probe. It stays out of the tree, since nothing else here builds against a stub Win32 and an uncompiled probe rots unnoticed. What the suite covers on Linux is the reporting contract and the cache path, not the Win32 code.Closes #1402