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

["René Scharfe" <l.s.r@web.de>]

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é