mailinfo: avoid segfault when can't open files

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

Can be reproduced using git mailinfo . .

---
 mailinfo.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

index a89db22ab..035abbbf5 100644
@@ -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

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:

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

...

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.

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

It looks good to me.

-Peff

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.

---
 mailinfo.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

index a89db22ab..d04142ccc 100644
@@ -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