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

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

[Juan F. Codagnone]

If <msg> or <patch> files can't be opened, clear_mailinfo crash as
it follows NULL pointers.

Can be reproduced using `git mailinfo . .`

Signed-off-by: Juan F. Codagnone <jcodagnone@gmail.com>
---
 mailinfo.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

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]);
 	free(mi->p_hdr_data);
-	for (i = 0; mi->s_hdr_data[i]; i++)
-		strbuf_release(mi->s_hdr_data[i]);
+	if(mi->s_hdr_data != NULL)
+		for (i = 0; mi->s_hdr_data[i]; i++)
+			strbuf_release(mi->s_hdr_data[i]);
 	free(mi->s_hdr_data);
 
 	while (mi->content < mi->content_top) {
-- 
2.14.3

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

[Jeff King]

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. :)

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

[Juan F. Codagnone]

On Wed, Jan 24, 2018 at 1:02 AM, Jeff King <peff@peff.net> wrote:
>
> On Tue, Jan 23, 2018 at 11:54:17PM -0300, Juan F. Codagnone wrote:
...
>
>
> 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:


Thanks for the detailed review! I'm sorry about the style nits. I
focused on the tabs and braces. Next time I will take additional
attention.
I'll be resubmitting  the patch taking into account your remarks.

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

[Jeff King]

On Wed, Jan 24, 2018 at 01:51:31PM -0300, Juan F. Codagnone wrote:

> > As for the patch itself, it looks correct but I saw two style nits:
> 
> Thanks for the detailed review! I'm sorry about the style nits. I
> focused on the tabs and braces. Next time I will take additional
> attention.

No problem, and thank you for fixing the bug (also, I think this is your
first patch, so welcome to git. :) ).

> I'll be resubmitting  the patch taking into account your remarks.

It looks good to me.

-Peff

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

[Juan F. Codagnone]

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.

Signed-off-by: Juan F. Codagnone <jcodagnone@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
---
 mailinfo.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index a89db22ab..d04142ccc 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)
+		for (i = 0; mi->p_hdr_data[i]; i++)
+			strbuf_release(mi->p_hdr_data[i]);
 	free(mi->p_hdr_data);
-	for (i = 0; mi->s_hdr_data[i]; i++)
-		strbuf_release(mi->s_hdr_data[i]);
+	if (mi->s_hdr_data)
+		for (i = 0; mi->s_hdr_data[i]; i++)
+			strbuf_release(mi->s_hdr_data[i]);
 	free(mi->s_hdr_data);
 
 	while (mi->content < mi->content_top) {
-- 
2.14.3