Skip to content

zipAbandonFileInZip reports success when nothing was truncated - #1413

Merged
xroche merged 2 commits into
masterfrom
fix-1402-iowin32-truncate
Aug 24, 2026
Merged

zipAbandonFileInZip reports success when nothing was truncated#1413
xroche merged 2 commits into
masterfrom
fix-1402-iowin32-truncate

Conversation

@xroche

@xroche xroche commented Aug 24, 2026

Copy link
Copy Markdown
Owner

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 in src/minizip/iowin32.c set neither ztruncate64_file nor zflush_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 as ZIP_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 (SetFilePointerEx plus SetEndOfFile, putting the file position back where the caller had it, as ftruncate leaves it) and win32_flush_file_func (FlushFileBuffers), wires both into all four tables, and adds a ZIP_NOTRUNCATED return 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 only cache_zip_write_failed() ignored the return. iowin32.c gains the .orig/.diff pair 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-bit zipOpen2 where 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 as hts_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's zipfile still 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_utf8 on the 64-bit fopen table, and iowin32.c only ships in EXTRA_DIST for the Windows projects, so no leg here compiles it. Being _WIN32-only it moves no POSIX ABI. I did compile it locally against a stub windows.h and 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. Dropping SetEndOfFile and 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

xroche and others added 2 commits August 24, 2026 22:20
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>
@xroche
xroche enabled auto-merge (squash) August 24, 2026 21:39
@xroche
xroche merged commit 5bf74ae into master Aug 24, 2026
44 checks passed
@xroche
xroche deleted the fix-1402-iowin32-truncate branch August 24, 2026 22:59
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.

The Win32 ioapi tables leave the new truncate and flush entries unset

1 participant