Re: [PATCH] mailinfo: avoid segfault when can't open files

[Jeff King <peff@peff.net>]

On Tue, Jan 23, 2018 at 11:54:17PM -0300, Juan F. Codagnone wrote:

> If <msg> or <patch> files can't be opened, clear_mailinfo crash as
> it follows NULL pointers.
> 
> Can be reproduced using `git mailinfo . .`

Thanks for finding this.

Looking at the offending code and your solution, it looks like:

  1. mailinfo() sometimes allocates mi->p_hdr_data and mi->s_hdr_data
     and sometimes not, depending on how far we get before seeing an
     error. The caller cannot know whether we did so or not based on
     seeing an error return, but most call clear_mailinfo() if it wants
     to avoid a leak.

  2. There are two callers of mailinfo(). git-am simply dies on an
     error, and so is unaffected. But git-mailinfo unconditionally calls
     clear_mailinfo() before returning, regardless of the return code.

  3. When we get to clear_mailinfo(), the arrays are either populated or
     are NULL. We know they're initialized to NULL because of
     setup_mailinfo(), which zeroes the whole struct.

So I think your fix does the right thing. I do think this is a pretty
awkward interface, and it would be less error-prone if either[1]:

  a. we bumped the allocation of these arrays up in mailinfo() so
     that they were simply always initialized. This fixes the bug in
     clear_mailinfo(), but also any other function which looks at the
     mailinfo struct (though I don't think there are any such cases).

  b. we had mailinfo() clean up after itself on error, so that it was
     always in a de-initialized state.

But given the lack of callers, it may not be worth the effort. So I'm OK
with this solution. It may be worth giving an abbreviated version of the
above explanation in the commit message. Perhaps:

  If <msg> or <patch> files can't be opened, then mailinfo() returns an
  error before it even initializes mi->p_hdr_data or mi->s_hdr_data.
  When cmd_mailinfo() then calls clear_mailinfo(), we dereference the
  NULL pointers trying to free their contents.

As for the patch itself, it looks correct but I saw two style nits:

> diff --git a/mailinfo.c b/mailinfo.c
> index a89db22ab..035abbbf5 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -1167,11 +1167,13 @@ void clear_mailinfo(struct mailinfo *mi)
>  	strbuf_release(&mi->inbody_header_accum);
>  	free(mi->message_id);
>  
> -	for (i = 0; mi->p_hdr_data[i]; i++)
> -		strbuf_release(mi->p_hdr_data[i]);
> +	if(mi->p_hdr_data != NULL)
> +		for (i = 0; mi->p_hdr_data[i]; i++)
> +			strbuf_release(mi->p_hdr_data[i]);

We usually say "if (" with an extra space. And we generally just check
pointers for their truth value. So:

  if (mi->p_hdr_data) {
	for (i = 0; ...)

-Peff

[1] Actually, it seems a little funny that we use xcalloc() here at all,
    since the size is determined by a compile-time constant. Why not
    just put an array directly into the struct and let it get zeroed
    with the rest of the struct? That sidesteps the question of whether
    we need to clear() after an error return, but it would fix this bug. :)