From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "René Scharfe" <l.s.r@web.de>, "Git List" <git@vger.kernel.org>
Subject: Re: [PATCH] don't report vsnprintf(3) error as bug
Date: Tue, 23 Apr 2024 18:26:52 -0400 [thread overview]
Message-ID: <20240423222652.GD1172807@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqa5lm1pr5.fsf@gitster.g>
On Sun, Apr 21, 2024 at 12:26:22PM -0700, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
> > strbuf_addf() has been reporting a negative return value of vsnprintf(3)
> > as a bug since f141bd804d (Handle broken vsnprintf implementations in
> > strbuf, 2007-11-13). Other functions copied that behavior:
> >
> > 7b03c89ebd (add xsnprintf helper function, 2015-09-24)
> > 5ef264dbdb (strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`, 2019-02-25)
> > 8d25663d70 (mem-pool: add mem_pool_strfmt(), 2024-02-25)
> >
> > However, vsnprintf(3) can legitimately return a negative value if the
> > formatted output would be longer than INT_MAX. Stop accusing it of
> > being broken and just report the fact that formatting failed.
>
> """ ... function returns the number of characters that would have
> been written had n been sufficiently large, not counting the
> terminating null character, or a negative value if an encoding error
> occurred. Thus, the null-terminated output has been completely
> written if and only if the returned value is nonnegative and less
> than n.""" is what I read in some versions of ISO/IEC 9899. It is
> curious that it does not say anything about the consequence of a
> parameter error arising from int (the type snprintf family of
> functions returns) being narrower than size_t (the type of the
> parameter n), but your point still stands that vsnprintf() can
> legitimately fail, and it is not a programming error.
POSIX does say:
The snprintf() function shall fail if:
EOVERFLOW
The value of n is greater than {INT_MAX}.
But mostly the INT_MAX thing is simply the one thing we've seen in
practice. I wouldn't be surprised if there are other conditions that can
trigger an error return from vsnprintf. E.g., POSIX says:
If a conversion specification does not match one of the above forms,
the behavior is undefined.
Of course "undefined" is much worse than returning -1, but it seems like
a reasonable thing for an implementation to choose to do (either that or
just output the character literally).
We should be immune-ish to such an issue by virtue of -Wformat (we're
only allowed to pass literal strings, and they must all be understood by
the compiler). Of course that's platform-specific and we only check
-Werror on some platforms. So gcc allows "%b", but it may be meaningless
on AIX, and who knows what their libc will do. ;)
That case kind of _is_ a BUG() situation. But I don't think it's worth
trying to differentiate that. Switching all of these to die() makes the
most sense (i.e., I like René's patch).
-Peff
prev parent reply other threads:[~2024-04-23 22:26 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-21 12:40 [PATCH] don't report vsnprintf(3) error as bug René Scharfe
2024-04-21 19:26 ` Junio C Hamano
2024-04-23 22:26 ` Jeff King [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240423222652.GD1172807@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.