Repository navigation
BUG: Bound the formatted write in znzprintf, and end the va_list on failure - #110
Open
hjmjohnson wants to merge 4 commits into
Open
hjmjohnson wants to merge 4 commits into
hjmjohnson wants to merge 4 commits into
Conversation
hjmjohnson
marked this pull request as ready for review
September 24, 2026 12:17
hjmjohnson
force-pushed
the
pr/fix-vsprintf-bounded
branch
from
October 6, 2026 14:35
68a4bf1 to
bf4f0f6
Compare
znzprintf() built its output with vsprintf(), which has no bound; the buffer was sized strlen(format) + 1000000 under a comment reading "overkill I hope". A single %s argument longer than a megabyte writes past the end of the allocation. vsnprintf() with the size already being computed turns that overflow into a reported truncation. The same block also returned on calloc failure without calling va_end(), leaving the va_list unterminated. The znzlib.h prototype gains __attribute__((format(printf, 2, 3))) on GCC and Clang, through a ZNZ_PRINTF_ATTR macro, so callers' format strings are checked and the vsnprintf() on the caller's format passes -Wformat-nonliteral. Callers may see new -Wformat diagnostics; nothing else changes for them. This is the only unbounded formatted write in the tree; there is no sprintf() and no gets() anywhere.
The preceding commit bounds the write but still returned the gzprintf
count after a truncation, so a caller could not tell. It also
returned 0 for an allocation failure, and the function returns 0 for a
NULL stream. The printf family reports failure with a negative value;
0 is a successful empty write, so it cannot carry either meaning.
All three now return -1, and a truncated result is not written at all:
a partial record reported as a whole one is worse than nothing.
The gz branch also passed the formatted text back through
gzprintf("%s"), which formats into zlib's own buffer (8192 bytes by
default). For any longer output zlib writes nothing and returns 0, so
every gz write from 8192 bytes up to the 1000000 byte bound failed
silently. The text is already formatted, so it is written with
gzwrite(), and a short count returns -1.
znzprintf sits inside COMPILE_NIFTIUNUSED_CODE in both znzlib.c and
znzlib.h and is absent from libznz.a in a default build, so this is a
latent defect in code no released configuration compiles.
znzflush() and znzeof() are defined inside COMPILE_NIFTIUNUSED_CODE and declared nowhere, unlike znzgets(), znzputc(), and znzgetc() beside them. No build compiles that block today, so the omission was invisible; the znzprintf test that follows compiles it, and -Werror=missing-declarations would then reject both definitions. They are declared where the rest of the block is declared, so the warning set the project already requires stays clean when the block is built.
znzprintf has no caller in this tree and is compiled only under COMPILE_NIFTIUNUSED_CODE, so nothing reached it. The test builds its own copy of znzlib.c with that definition and checks: - a NULL stream returns a negative value; - a 100000 byte gz write, past zlib's 8192 byte gzprintf buffer, returns its length and lands in the file; - a 2 MB argument, past the 1000000 byte bound, returns a negative value and writes nothing. Without the bound the 2 MB write overflows the allocation; with gzprintf in place of gzwrite the 100000 byte write returns 0 and writes nothing. The file content is pinned too, so an over-broad fix that refuses everything does not pass. ZNZPRINTF_BOUND in the test mirrors the bound in znzlib.c, and a comment there points back here.
hjmjohnson
force-pushed
the
pr/fix-vsprintf-bounded
branch
from
November 6, 2026 16:55
bf4f0f6 to
d64146e
Compare
Member
Author
|
@greptileai review |
Collaborator
|
Wait, indeed |
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.
Re-submission of #41, reverted from
masteron 2026-09-24 so it can bereviewed before merging. Review fixes since then: the gz branch now writes
with
gzwrite(), sincegzprintf()silently wrote nothing for output of8192 bytes or more, and the test covers that path and the NULL stream.
Base:
master. Independent: nothing has to land before it.Commits
(also adds
__attribute__((format(printf, 2, 3)))to the znzprintfprototype on GCC and Clang, through a
ZNZ_PRINTF_ATTRmacro)failure, truncation, and a short
gzwrite()all return -1)Changes from the original #41
gzprintf("%s").zlib formats into its own 8192 byte buffer and returns 0 without writing
anything for longer output, so every gz write from 8192 bytes up to the
1000000 byte bound failed silently. It now uses
gzwrite()and returns -1on a short count.
-Werror=missing-declarations.truncation case. Each fails when its fix is reverted.
one line each.
Ordering for all the re-submitted work is tracked in #84.