format-patch: fix leak of empty header string

format-patch: fix leak of empty header string

The log_write_email_headers() function recently learned to return the
"extra_headers_p" variable to the caller as an allocated string. We
start by copying rev_info.extra_headers into a strbuf, and then detach
the strbuf at the end of the function. If there are no extra headers, we
leave the strbuf empty. Likewise, if there are no headers to return, we
pass back NULL.

This misses a corner case which can cause a leak. The "do we have any
headers to copy" check is done by looking for a NULL opt->extra_headers.
But the "do we have a non-empty string to return" check is done by
checking the length of the strbuf. That means if opt->extra_headers is
the empty string, we'll "copy" it into the strbuf, triggering an
allocation, but then leak the buffer when we return NULL from the
function.

We can solve this in one of two ways:

  1. Rather than checking headers->len at the end, we could check
     headers->alloc to see if we allocated anything. That retains the
     original behavior before the recent change, where an empty
     extra_headers string is "passed through" to the caller. In practice
     this doesn't matter, though (the code which eventually looks at the
     result treats NULL or the empty string the same).

  2. Only bother copying a non-empty string into the strbuf. This has
     the added bonus of avoiding a pointless allocation.

     Arguably strbuf_addstr() could do this optimization itself, though
     it may be slightly dangerous to do so (some existing callers may
     not get a fresh allocation when they expect to). In theory callers
     are all supposed to use strbuf_detach() in such a case, but there's
     no guarantee that this is the case.

This patch uses option 2. Without it, building with SANITIZE=leak shows
many errors in t4021 and elsewhere.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>