{"thread":{"id":"47675","subject":"[PATCH] mailinfo: avoid segfault when can't open files","startedAt":"2018-01-24T02:54:39Z","lastAt":"2018-01-24T18:06:44Z","messageCount":5,"participants":["Juan F. Codagnone","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"337201","messageId":"20180124025417.32497-1-jcodagnone@gmail.com","threadId":"47675","inReplyTo":null,"subject":"[PATCH] mailinfo: avoid segfault when can't open files","fromName":"Juan F. Codagnone","fromEmail":"jcodagnone@gmail.com","sentAt":"2018-01-24T02:54:17Z","receivedAt":"2018-01-24T02:54:39Z","isPatch":true,"sender":{"key":"jcodagnone@gmail.com","avatar":"https://avatars.githubusercontent.com/u/541876?v=4"},"body":"If or files can't be opened, clear_mailinfo crash as\nit follows NULL pointers.\n\nCan be reproduced using `git mailinfo . .`\n\nSigned-off-by: Juan F. Codagnone \n---\n mailinfo.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex a89db22ab..035abbbf5 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -1167,11 +1167,13 @@ void clear_mailinfo(struct mailinfo *mi)\n \tstrbuf_release(&mi->inbody_header_accum);\n \tfree(mi->message_id);\n \n-\tfor (i = 0; mi->p_hdr_data[i]; i++)\n-\t\tstrbuf_release(mi->p_hdr_data[i]);\n+\tif(mi->p_hdr_data != NULL)\n+\t\tfor (i = 0; mi->p_hdr_data[i]; i++)\n+\t\t\tstrbuf_release(mi->p_hdr_data[i]);\n \tfree(mi->p_hdr_data);\n-\tfor (i = 0; mi->s_hdr_data[i]; i++)\n-\t\tstrbuf_release(mi->s_hdr_data[i]);\n+\tif(mi->s_hdr_data != NULL)\n+\t\tfor (i = 0; mi->s_hdr_data[i]; i++)\n+\t\t\tstrbuf_release(mi->s_hdr_data[i]);\n \tfree(mi->s_hdr_data);\n \n \twhile (mi->content < mi->content_top) {\n-- \n2.14.3\n\n"},{"id":"337202","messageId":"20180124040232.GB1330@sigill.intra.peff.net","threadId":"47675","inReplyTo":"20180124025417.32497-1-jcodagnone@gmail.com","subject":"Re: [PATCH] mailinfo: avoid segfault when can't open files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-24T04:02:32Z","receivedAt":"2018-01-24T04:02:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 23, 2018 at 11:54:17PM -0300, Juan F. Codagnone wrote:\n\n> If or files can't be opened, clear_mailinfo crash as\n> it follows NULL pointers.\n> \n> Can be reproduced using `git mailinfo . .`\n\nThanks for finding this.\n\nLooking at the offending code and your solution, it looks like:\n\n 1. mailinfo() sometimes allocates mi->p_hdr_data and mi->s_hdr_data\n and sometimes not, depending on how far we get before seeing an\n error. The caller cannot know whether we did so or not based on\n seeing an error return, but most call clear_mailinfo() if it wants\n to avoid a leak.\n\n 2. There are two callers of mailinfo(). git-am simply dies on an\n error, and so is unaffected. But git-mailinfo unconditionally calls\n clear_mailinfo() before returning, regardless of the return code.\n\n 3. When we get to clear_mailinfo(), the arrays are either populated or\n are NULL. We know they're initialized to NULL because of\n setup_mailinfo(), which zeroes the whole struct.\n\nSo I think your fix does the right thing. I do think this is a pretty\nawkward interface, and it would be less error-prone if either[1]:\n\n a. we bumped the allocation of these arrays up in mailinfo() so\n that they were simply always initialized. This fixes the bug in\n clear_mailinfo(), but also any other function which looks at the\n mailinfo struct (though I don't think there are any such cases).\n\n b. we had mailinfo() clean up after itself on error, so that it was\n always in a de-initialized state.\n\nBut given the lack of callers, it may not be worth the effort. So I'm OK\nwith this solution. It may be worth giving an abbreviated version of the\nabove explanation in the commit message. Perhaps:\n\n If or files can't be opened, then mailinfo() returns an\n error before it even initializes mi->p_hdr_data or mi->s_hdr_data.\n When cmd_mailinfo() then calls clear_mailinfo(), we dereference the\n NULL pointers trying to free their contents.\n\nAs for the patch itself, it looks correct but I saw two style nits:\n\n> diff --git a/mailinfo.c b/mailinfo.c\n> index a89db22ab..035abbbf5 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -1167,11 +1167,13 @@ void clear_mailinfo(struct mailinfo *mi)\n> \tstrbuf_release(&mi->inbody_header_accum);\n> \tfree(mi->message_id);\n> \n> -\tfor (i = 0; mi->p_hdr_data[i]; i++)\n> -\t\tstrbuf_release(mi->p_hdr_data[i]);\n> +\tif(mi->p_hdr_data != NULL)\n> +\t\tfor (i = 0; mi->p_hdr_data[i]; i++)\n> +\t\t\tstrbuf_release(mi->p_hdr_data[i]);\n\nWe usually say \"if (\" with an extra space. And we generally just check\npointers for their truth value. So:\n\n if (mi->p_hdr_data) {\n\tfor (i = 0; ...)\n\n-Peff\n\n[1] Actually, it seems a little funny that we use xcalloc() here at all,\n since the size is determined by a compile-time constant. Why not\n just put an array directly into the struct and let it get zeroed\n with the rest of the struct? That sidesteps the question of whether\n we need to clear() after an error return, but it would fix this bug. :)\n"},{"id":"337269","messageId":"CALfSDjd0oZZmz1nKzasKu-Adn3LPS+AdLH4mYeR=Rx-PZpe4Jw@mail.gmail.com","threadId":"47675","inReplyTo":"20180124040232.GB1330@sigill.intra.peff.net","subject":"Re: [PATCH] mailinfo: avoid segfault when can't open files","fromName":"Juan F. Codagnone","fromEmail":"jcodagnone@gmail.com","sentAt":"2018-01-24T16:51:31Z","receivedAt":"2018-01-24T16:51:37Z","isPatch":true,"sender":{"key":"jcodagnone@gmail.com","avatar":"https://avatars.githubusercontent.com/u/541876?v=4"},"body":"On Wed, Jan 24, 2018 at 1:02 AM, Jeff King wrote:\n>\n> On Tue, Jan 23, 2018 at 11:54:17PM -0300, Juan F. Codagnone wrote:\n...\n>\n>\n> But given the lack of callers, it may not be worth the effort. So I'm OK\n> with this solution. It may be worth giving an abbreviated version of the\n> above explanation in the commit message. Perhaps:\n>\n> If or files can't be opened, then mailinfo() returns an\n> error before it even initializes mi->p_hdr_data or mi->s_hdr_data.\n> When cmd_mailinfo() then calls clear_mailinfo(), we dereference the\n> NULL pointers trying to free their contents.\n>\n> As for the patch itself, it looks correct but I saw two style nits:\n\n\nThanks for the detailed review! I'm sorry about the style nits. I\nfocused on the tabs and braces. Next time I will take additional\nattention.\nI'll be resubmitting the patch taking into account your remarks.\n"},{"id":"337270","messageId":"20180124165647.7386-1-jcodagnone@gmail.com","threadId":"47675","inReplyTo":"20180124040232.GB1330@sigill.intra.peff.net","subject":"[PATCH v1] mailinfo: avoid segfault when can't open files","fromName":"Juan F. Codagnone","fromEmail":"jcodagnone@gmail.com","sentAt":"2018-01-24T16:56:47Z","receivedAt":"2018-01-24T16:57:02Z","isPatch":true,"sender":{"key":"jcodagnone@gmail.com","avatar":"https://avatars.githubusercontent.com/u/541876?v=4"},"body":"If or files can't be opened, then mailinfo() returns an\nerror before it even initializes mi->p_hdr_data or mi->s_hdr_data.\nWhen cmd_mailinfo() then calls clear_mailinfo(), we dereference the\nNULL pointers trying to free their contents.\n\nSigned-off-by: Juan F. Codagnone \nReviewed-by: Jeff King \n---\n mailinfo.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex a89db22ab..d04142ccc 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -1167,11 +1167,13 @@ void clear_mailinfo(struct mailinfo *mi)\n \tstrbuf_release(&mi->inbody_header_accum);\n \tfree(mi->message_id);\n \n-\tfor (i = 0; mi->p_hdr_data[i]; i++)\n-\t\tstrbuf_release(mi->p_hdr_data[i]);\n+\tif (mi->p_hdr_data)\n+\t\tfor (i = 0; mi->p_hdr_data[i]; i++)\n+\t\t\tstrbuf_release(mi->p_hdr_data[i]);\n \tfree(mi->p_hdr_data);\n-\tfor (i = 0; mi->s_hdr_data[i]; i++)\n-\t\tstrbuf_release(mi->s_hdr_data[i]);\n+\tif (mi->s_hdr_data)\n+\t\tfor (i = 0; mi->s_hdr_data[i]; i++)\n+\t\t\tstrbuf_release(mi->s_hdr_data[i]);\n \tfree(mi->s_hdr_data);\n \n \twhile (mi->content < mi->content_top) {\n-- \n2.14.3\n\n"},{"id":"337274","messageId":"20180124180638.GA18703@sigill.intra.peff.net","threadId":"47675","inReplyTo":"CALfSDjd0oZZmz1nKzasKu-Adn3LPS+AdLH4mYeR=Rx-PZpe4Jw@mail.gmail.com","subject":"Re: [PATCH] mailinfo: avoid segfault when can't open files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-24T18:06:38Z","receivedAt":"2018-01-24T18:06:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 24, 2018 at 01:51:31PM -0300, Juan F. Codagnone wrote:\n\n> > As for the patch itself, it looks correct but I saw two style nits:\n> \n> Thanks for the detailed review! I'm sorry about the style nits. I\n> focused on the tabs and braces. Next time I will take additional\n> attention.\n\nNo problem, and thank you for fixing the bug (also, I think this is your\nfirst patch, so welcome to git. :) ).\n\n> I'll be resubmitting the patch taking into account your remarks.\n\nIt looks good to me.\n\n-Peff\n"}]}