From: Jeff King <peff@peff.net>
Date: Tue, 28 Feb 2017 18:17:19 GMT
Subject: Re: format-patch subject-prefix gets truncated when using the --numbered flag
Message-ID: <20170228181719.t67abnnnsdcx2xfd@sigill.intra.peff.net>
In-Reply-To: <1488297556.2955.11.camel@enea.com>

On Tue, Feb 28, 2017 at 03:59:16PM +0000, Adrian Dudau wrote:

> This is happening on the latest master branch, so I dug through the
> code and tracked the issue to this piece of code in log-tree.c:
> 
>         if (opt->total > 0) {
>                 static char buffer[64];
>                 snprintf(buffer, sizeof(buffer),
>                          "Subject: [%s%s%0*d/%d] ",
>                          opt->subject_prefix,
>                          *opt->subject_prefix ? " " : "",
>                          digits_in_number(opt->total),
>                          opt->nr, opt->total);
>                 subject = buffer;
>         } else if (opt->total == 0 && opt->subject_prefix && *opt-
> >subject_prefix) {
>                 static char buffer[256];
>                 snprintf(buffer, sizeof(buffer),
>                          "Subject: [%s] ",
>                          opt->subject_prefix);
>                 subject = buffer;
>         } else {
>                 subject = "Subject: ";
>         }
> 
> Apparently the size of the "buffer" var is different in the two
> situations. Anybody knows if this is by design or just an old
> oversight?

I think it's just an old oversight. There are some other fixed-size
buffers later, too, which could similarly truncate.

I think these should all be "static strbuf", and entering the function
they should get strbuf_reset(), followed by a strbuf_addf(). The static
strbufs will be the owners of the allocated heap memory, and it will get
reused from call to call.

That stops the immediate problem. As a function interface, it's pretty
ugly. It would probably make more sense for the caller to pass in a
strbuf rather than have us pass out pointers to static storage. For the
call in make_cover_letter(), that would be fine. For show_log(), it's
less clear. That's called for every commit in "git log", which might be
a little sensitive to allocations.

The only persistent storage it has is via the rev_info. Perhaps it could
hold some scratch buffers for the traversal (though I don't think we
currently do any resource-freeing when done with a rev_info, so it
effectively becomes a leak).

-Peff