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

From: Jeff King <peff@peff.net>
Date: Apr 3, 2024, 00:47 UTC
Message-ID: <20240403004751.GA892394@coredump.intra.peff.net>
In-Reply-To: <f9ad9f41-5b9b-474e-9818-f91fc937daae@web.de>

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

Show 11 quoted lines

> 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