[PATCH 7/6] format-patch: fix leak of empty header string

On Tue, Mar 19, 2024 at 08:25:55PM -0400, Jeff King wrote:

These patches introduce a small leak into format-patch. I didn't notice before because the "leaks" CI jobs were broken due to sanitizer problems in the base image (which now seem fixed?).

Here's a fix that can go on top of jk/pretty-subject-cleanup. That topic is not in 'next' yet, so I could also re-roll. The issue was subtle enough that a separate commit is not such a bad thing, but I'm happy to squash it in if we'd prefer.

-- >8 --
Subject: [PATCH] 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>
---
 log-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)