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
