pathspec: fix segfault in clear_pathspec

PATCH In review Brandon Williams 3 participants 3 messages Apr 7, 2017 – Apr 10, 2017

Share & tools

Brandon Williams

author

2017-04-07 19:29 UTC #0 lore raw

PATCH 1/1 pathspec: fix segfault in clear_pathspec

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 <[email protected]>

---
 pathspec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

pathspec.c +1 -1

index 303efda83..69ef86b85 100644
@@ -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);

Signature · 1 line

2.12.2.715.g7642488e1d-goog

Stefan Beller

2017-04-07 19:40 UTC #1 lore raw

Quoted 5 lines · 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'.

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

Duy Nguyen

2017-04-10 12:05 UTC #2 lore raw

In reply to Brandon Williams

Quoted 16 lines · 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 <[email protected]>
---
 pathspec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
pathspec.c +0 -0
index 303efda83..69ef86b85 100644

pathspec.c void clear_pathspec(struct pathspec *pathspec) · line 727

Quoted 5 lines

pathspec.c +1 -1

@@ -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.

Quoted 6 lines

pathspec.c +0 -1

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

2.12.2.715.g7642488e1d-goog

Signature · 1 line

Duy