Re: [PATCH] imap-send: use xsnprintf to format command

[Jeff King <peff@peff.net>]

On Tue, Apr 02, 2024 at 04:51:05PM +0200, René Scharfe wrote:

> nfsnprintf() wraps vsnprintf(3) and reports attempts to use too small a
> buffer using BUG(), just like xsnprintf().
> 
> It has an extra check that makes sure the buffer size (converted to int)
> is positive.  vsnprintf(3) is supposed to handle a buffer size of zero
> or bigger than INT_MAX just fine, so this extra comparison doesn't make
> us any safer.  If a platform has a broken implementation, we'd need to
> work around it in our compat code.
> 
> Call xsnprintf() instead to reduce code duplication and make the caller
> slightly more readable by using this more common helper.

I think this is an improvement, and since the original also called BUG()
we are not making anything worse. But I think the original was actually
in error to do so, since it depends on user data. E.g., setting
imap.user to the output of $(perl -e 'print "a" x 1024"') results in:

  BUG: imap-send.c:511: buffer too small. Please report a bug.
  error: git-imap-send died of signal 6

Now obviously that's a dumb username, but this is the buffer used for
all IMAP commands. So conceivably long folder names, etc, do the same
thing. It's probably not a big deal in practice, but conceptually this
probably ought to be xstrfmt() and not xsnprintf().

Likewise imap-send's nfvasprintf() is basically xstrfmt(), except it
takes a va_list. So it would have to be replaced by strbuf_vaddf().

I wouldn't be surprised if there are other opportunities for string
cleanup, but I generally hoped that if we waited long enough imap-send
would just go away. ;) Either because we could get rid of the tool
entirely (though from a recent-ish search, there did not seem to be a
lot of good other tools) or because we'd just drop the old code and rely
on curl to do the heavy lifting.

All that said, I think your patch makes the world slightly better, so
I'm not opposed in the meantime.

-Peff