{"thread":{"id":"45633","subject":"[PATCH] pathspec: fix segfault in clear_pathspec","startedAt":"2017-04-07T19:29:29Z","lastAt":"2017-04-10T12:05:12Z","messageCount":3,"participants":["Brandon Williams","Stefan Beller","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"316377","messageId":"20170407192919.10209-1-bmwill@google.com","threadId":"45633","inReplyTo":null,"subject":"[PATCH] pathspec: fix segfault in clear_pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-07T19:29:19Z","receivedAt":"2017-04-07T19:29:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In 'clear_pathspec()' the incorrect index parameter is used to bound an\ninner-loop which is used to free a 'struct attr_match' value field.\nUsing the incorrect index parameter (in addition to being incorrect)\noccasionally causes segmentation faults when attempting to free an\ninvalid pointer.  Fix this by using the correct index parameter 'i'.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n pathspec.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 303efda83..69ef86b85 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -724,7 +724,7 @@ void clear_pathspec(struct pathspec *pathspec)\n \t\tfree(pathspec->items[i].match);\n \t\tfree(pathspec->items[i].original);\n \n-\t\tfor (j = 0; j < pathspec->items[j].attr_match_nr; j++)\n+\t\tfor (j = 0; j < pathspec->items[i].attr_match_nr; j++)\n \t\t\tfree(pathspec->items[i].attr_match[j].value);\n \t\tfree(pathspec->items[i].attr_match);\n \n-- \n2.12.2.715.g7642488e1d-goog\n\n"},{"id":"316378","messageId":"CAGZ79kao8R9ANDv94oovwB0QK4V_r2VmdCuuUxoEBt_LWFm70A@mail.gmail.com","threadId":"45633","inReplyTo":"20170407192919.10209-1-bmwill@google.com","subject":"Re: [PATCH] pathspec: fix segfault in clear_pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-07T19:39:57Z","receivedAt":"2017-04-07T19:40:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 7, 2017 at 12:29 PM, Brandon Williams <bmwill@google.com> wrote:\n> In 'clear_pathspec()' the incorrect index parameter is used to bound an\n> inner-loop which is used to free a 'struct attr_match' value field.\n> Using the incorrect index parameter (in addition to being incorrect)\n> occasionally causes segmentation faults when attempting to free an\n> invalid pointer.  Fix this by using the correct index parameter 'i'.\n\nThis was introduced via b0db704652 (pathspec: allow querying for\nattributes, 2017-03-13), and it seems there was no other topics\nin flight since then or at the time.  So the review failed to spot it\nand not some other weird root cause.\n\nThanks,\nStefan\n"},{"id":"316481","messageId":"CACsJy8D9oXDbOqEfPTuSdm0Phvwo6AxQh_Q5tG4k22_mFBT4OA@mail.gmail.com","threadId":"45633","inReplyTo":"20170407192919.10209-1-bmwill@google.com","subject":"Re: [PATCH] pathspec: fix segfault in clear_pathspec","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-10T12:04:34Z","receivedAt":"2017-04-10T12:05:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 8, 2017 at 2:29 AM, Brandon Williams <bmwill@google.com> wrote:\n> In 'clear_pathspec()' the incorrect index parameter is used to bound an\n> inner-loop which is used to free a 'struct attr_match' value field.\n> Using the incorrect index parameter (in addition to being incorrect)\n> occasionally causes segmentation faults when attempting to free an\n> invalid pointer.  Fix this by using the correct index parameter 'i'.\n>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n>  pathspec.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/pathspec.c b/pathspec.c\n> index 303efda83..69ef86b85 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -724,7 +724,7 @@ void clear_pathspec(struct pathspec *pathspec)\n>                 free(pathspec->items[i].match);\n>                 free(pathspec->items[i].original);\n>\n> -               for (j = 0; j < pathspec->items[j].attr_match_nr; j++)\n> +               for (j = 0; j < pathspec->items[i].attr_match_nr; j++)\n\nOuch. Perhaps this is a good time to rename 'j' to something better?\nattr_idx or attr_index, maybe.\n\n>                         free(pathspec->items[i].attr_match[j].value);\n>                 free(pathspec->items[i].attr_match);\n>\n> --\n> 2.12.2.715.g7642488e1d-goog\n>\n\n\n\n-- \nDuy\n"}]}