Re: git segfault in tag verify (patch included)

From: Jeff King <peff@peff.net>
Date: Jul 16, 2019, 18:58 UTC
Message-ID: <20190716185835.GA24468@sigill.intra.peff.net>
In-Reply-To: <xmqqsgr53kov.fsf@gitster-ct.c.googlers.com>

On Tue, Jul 16, 2019 at 11:20:48AM -0700, Junio C Hamano wrote:

Show 25 quoted lines

> That is this thing.
> 
>         static void parse_gpg_output(struct signature_check *sigc)
>         {
>                 const char *buf = sigc->gpg_status;
>                 const char *line, *next;
>                 int i, j;
>                 int seen_exclusive_status = 0;
> 
>                 /* Iterate over all lines */
>                 for (line = buf; *line; line = strchrnul(line+1, '\n')) {
>                         while (*line == '\n')
>                                 line++;
>                         /* Skip lines that don't start with GNUPG status */
>                         if (!skip_prefix(line, "[GNUPG:] ", &line))
>                                 continue;
> 
> If the GPG output ends with a trailing blank line, we skip and get
> to the terminating NUL, then find that it does not begin with
> the "[GNUPG:] " prefix, and hit the continue.  We try to scan and
> look for LF (or stop at the end of the string) for the next round,
> starting at one past where we are, which is already the terminating
> NUL.  Ouch.
> 
> Good finding.

Yeah. The patch below looks fine, and I do not see any other out-of-bounds issues in the code (there is a similar "next + 1" in the inner loop, but it checks for an empty line right beforehand already).

I find these kind of "+1" pointer increments without constraint checking are error-prone. I suspect this whole loop could be a bit easier to follow by finding the next line boundary at the start of the loop, and jumping to it in the for-loop advancement. And then within the loop, we know the boundaries of the line (right now, for example, we issue a strchrnul() looking for a space that might unexpectedly cross line boundaries).

Something like:

  for (line = buf; *line; line = next) {
	next = strchrnul(line, '\n');

	... do stuff ...
	/* find a space within the line */
	space = memchr(line, ' ', next - line);
  }

or even:

  for (line = buf; *line; line += len) {
	size_t len = strchrnul(line, '\n') - line;
	...
	space = memchr(line, ' ', linelen);
  }

but it may not be worth the churn. It's just as likely to introduce a new bug. ;)

I've also found working with strbuf_getline() to be very pleasant for a lot of these cases (in which you get a real per-line NUL-terminated string), but of course it implies an extra copy.

-Peff