[PATCH] pathspec: fix segfault in clear_pathspec

[PATCH] pathspec: fix segfault in clear_pathspec

[Brandon Williams]

In 'clear_pathspec()' the incorrect index parameter is used to bound an
inner-loop which is used to free a 'struct attr_match' value field.
Using the incorrect index parameter (in addition to being incorrect)
occasionally causes segmentation faults when attempting to free an
invalid pointer.  Fix this by using the correct index parameter 'i'.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 pathspec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pathspec.c b/pathspec.c
index 303efda83..69ef86b85 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -724,7 +724,7 @@ void clear_pathspec(struct pathspec *pathspec)
 		free(pathspec->items[i].match);
 		free(pathspec->items[i].original);
 
-		for (j = 0; j < pathspec->items[j].attr_match_nr; j++)
+		for (j = 0; j < pathspec->items[i].attr_match_nr; j++)
 			free(pathspec->items[i].attr_match[j].value);
 		free(pathspec->items[i].attr_match);
 
-- 
2.12.2.715.g7642488e1d-goog

Re: [PATCH] pathspec: fix segfault in clear_pathspec

[Stefan Beller]

On Fri, Apr 7, 2017 at 12:29 PM, Brandon Williams <bmwill@google.com> wrote:
> In 'clear_pathspec()' the incorrect index parameter is used to bound an
> inner-loop which is used to free a 'struct attr_match' value field.
> Using the incorrect index parameter (in addition to being incorrect)
> occasionally causes segmentation faults when attempting to free an
> invalid pointer.  Fix this by using the correct index parameter 'i'.

This was introduced via b0db704652 (pathspec: allow querying for
attributes, 2017-03-13), and it seems there was no other topics
in flight since then or at the time.  So the review failed to spot it
and not some other weird root cause.

Thanks,
Stefan

Re: [PATCH] pathspec: fix segfault in clear_pathspec

[Duy Nguyen]

On Sat, Apr 8, 2017 at 2:29 AM, Brandon Williams <bmwill@google.com> wrote:
> In 'clear_pathspec()' the incorrect index parameter is used to bound an
> inner-loop which is used to free a 'struct attr_match' value field.
> Using the incorrect index parameter (in addition to being incorrect)
> occasionally causes segmentation faults when attempting to free an
> invalid pointer.  Fix this by using the correct index parameter 'i'.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  pathspec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 303efda83..69ef86b85 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -724,7 +724,7 @@ void clear_pathspec(struct pathspec *pathspec)
>                 free(pathspec->items[i].match);
>                 free(pathspec->items[i].original);
>
> -               for (j = 0; j < pathspec->items[j].attr_match_nr; j++)
> +               for (j = 0; j < pathspec->items[i].attr_match_nr; j++)

Ouch. Perhaps this is a good time to rename 'j' to something better?
attr_idx or attr_index, maybe.

>                         free(pathspec->items[i].attr_match[j].value);
>                 free(pathspec->items[i].attr_match);
>
> --
> 2.12.2.715.g7642488e1d-goog
>



-- 
Duy