Am 03.04.24 um 02:47 schrieb Jeff King: > 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(). >From reading the code I assumed the static buffer is there to stay within some IMAP limit. RFC 9051 mentions the distinction between synchronizing and non-synchronizing literals. The latter have a maximum length of 4096 bytes. But those are transferred after the command, so have no relevance for the command buffer size. I see no other limits, and I don't see us respecting that non-synchronizing literals limit, either. I guess that means messages longer than 4096 bytes could be rejected by a conforming IMAP server? Hmm. > Likewise imap-send's nfvasprintf() is basically xstrfmt(), except it > takes a va_list. So it would have to be replaced by strbuf_vaddf(). Looking closer I notice that the result of the single nfvasprintf() call is fed into the 1024 bytes buffer. So we could replace it with strbuf_vaddf() or xstrvfmt() and still stay within that strange limit, as it's enforced later. Its own 8192 buffer shields us from huge allocations e.g. due to long usernames or paths, but we probably don't need this protection as such and "attack" would originate and be felt only locally. > 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. Oh, my build uses curl, so my earlier test run was even worth less than I thought. René
