Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()

[Jeff King <peff@peff.net>]

On Sun, Mar 31, 2024 at 08:53:07PM +0200, René Scharfe wrote:

> If len is INT_MAX in mem_pool_strvfmt(), then len + 1 overflows.
> Casting it to size_t would prevent that.  Use st_add() to go a step
> further and make the addition *obviously* safe.  The compiler can
> optimize the check away on platforms where SIZE_MAX > INT_MAX, i.e.
> basically everywhere.

Yeah, I think this is a good thing to do. I was confused at first why we
had an "int" at all, but it's the usual crappy snprintf interface.

Which, by the way...

> @@ -123,13 +124,14 @@ static char *mem_pool_strvfmt(struct mem_pool *pool, const char *fmt,
>  	if (len < 0)
>  		BUG("your vsnprintf is broken (returned %d)", len);

Not new in your patch, and I know this is copied from the strbuf code,
but I think a BUG() is probably the wrong thing. We added it long ago to
let us know about broken vsnprintf() implementations, but we'd have
flushed those out by now, as nothing in Git would work on such a
platform.

And meanwhile there are legitimate reasons for a non-broken vsnprintf()
to return -1: namely that it is the only useful thing they can do when
the requested string is larger than INT_MAX (e.g., "%s" on a string that
is over 2GB). This is sort of academic, of course. There's no useful
error to return here, and anybody who manages to shove 2GB into a place
where we expect a short string fully deserves to have their program
abort.

I don't have a good example of where you can trigger this (it used to be
easy with long attribute names, but these days we refuse to parse them).
But in general probably calling die() is more appropriate.

There's a similar call in vreportf() that tries to keep going, but it
ends up with lousy results. E.g., try:

  perl -le 'print "create refs/heads/", "a"x2147483648, "HEAD"' |
  git update-ref --stdin

which results in just "fatal: ", since formatting the error string
fails. Perhaps we should just print the unexpanded format string
("invalid ref format: %s" in this case). It's not great, but it's better
than nothing.

I guess I diverged quite far from reviewing your patch. ;) It obviously
looks fine, but the snprintf() int return value got me off on a tangent.

-Peff