From 79366add74529359dfb57a387090e9c5f9c74282 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 14:39:31 +0100 Subject: bisect: fix leaking good/bad terms when reading multipe times Even though `read_bisect_terms()` is declared as assigning string constants, it in fact assigns allocated strings to the `read_bad` and `read_good` out parameters. The only callers of this function assign the result to global variables and thus don't have to free them in order to be leak-free. But that changes when executing the function multiple times because we'd then overwrite the previous value and thus make it unreachable. Fix the function signature and free the previous values. This leak is exposed by t0630, but plugging it does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bisect.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'bisect.c') diff --git a/bisect.c b/bisect.c index 4713226fad..9c52241050 100644 --- a/bisect.c +++ b/bisect.c @@ -28,8 +28,8 @@ static struct oid_array skipped_revs; static struct object_id *current_bad_oid; -static const char *term_bad; -static const char *term_good; +static char *term_bad; +static char *term_good; /* Remember to update object flag allocation in object.h */ #define COUNTED (1u<<16) @@ -985,7 +985,7 @@ static void show_commit(struct commit *commit) * We read them and store them to adapt the messages accordingly. * Default is bad/good. */ -void read_bisect_terms(const char **read_bad, const char **read_good) +void read_bisect_terms(char **read_bad, char **read_good) { struct strbuf str = STRBUF_INIT; const char *filename = git_path_bisect_terms(); @@ -993,16 +993,20 @@ void read_bisect_terms(const char **read_bad, const char **read_good) if (!fp) { if (errno == ENOENT) { - *read_bad = "bad"; - *read_good = "good"; + free(*read_bad); + *read_bad = xstrdup("bad"); + free(*read_good); + *read_good = xstrdup("good"); return; } else { die_errno(_("could not read file '%s'"), filename); } } else { strbuf_getline_lf(&str, fp); + free(*read_bad); *read_bad = strbuf_detach(&str, NULL); strbuf_getline_lf(&str, fp); + free(*read_good); *read_good = strbuf_detach(&str, NULL); } strbuf_release(&str); -- cgit v1.2.3 From 96ab0e7b8b586322c1da6bbcd40773a54472679b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 14:39:32 +0100 Subject: bisect: fix leaking string in `handle_bad_merge_base()` When handling a bad merge base we print an error, which includes the set of good revisions joined by spaces. This string is allocated, but never freed. Fix this memory leak. Note that the local `bad_hex` varible also looks like a string that we should free. But in fact, `oid_to_hex()` returns an address to a static variable even though it is declared to return a non-constant string. The function signature is thus quite misleading and really should be fixed, but doing so is outside of the scope of this patch series. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bisect.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'bisect.c') diff --git a/bisect.c b/bisect.c index 9c52241050..e9e603877e 100644 --- a/bisect.c +++ b/bisect.c @@ -801,6 +801,8 @@ static enum bisect_error handle_bad_merge_base(void) "between %s and [%s].\n"), bad_hex, term_bad, term_good, bad_hex, good_hex); } + + free(good_hex); return BISECT_MERGE_BASE_CHECK; } -- cgit v1.2.3 From a13d4a19d2b260e27b262292f07f5f315f04e07d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 14:39:33 +0100 Subject: bisect: fix leaking `current_bad_oid` When reading bisect refs we read the reference mapping to the "bad" term into the global `current_bad_oid` variable. This is an allocated string, but because it is global we never have to free it. This changes though when `register_ref()` is being called multiple times, at which point we'll overwrite the previous pointer and thus make it unreachable. Fix this issue by freeing the previous value. This leak is exposed by t6030, but plugging it does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bisect.c | 1 + 1 file changed, 1 insertion(+) (limited to 'bisect.c') diff --git a/bisect.c b/bisect.c index e9e603877e..6dbc22f84f 100644 --- a/bisect.c +++ b/bisect.c @@ -456,6 +456,7 @@ static int register_ref(const char *refname, const char *referent UNUSED, const strbuf_addstr(&good_prefix, "-"); if (!strcmp(refname, term_bad)) { + free(current_bad_oid); current_bad_oid = xmalloc(sizeof(*current_bad_oid)); oidcpy(current_bad_oid, oid); } else if (starts_with(refname, good_prefix.buf)) { -- cgit v1.2.3 From cfb8a0da55fec9619e4e5b1e9b211ef85e3c9cb3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 14:39:34 +0100 Subject: bisect: fix multiple leaks in `bisect_next_all()` There are multiple leaks in `bisect_next_all()`. For one we don't free the `tried` commit list. Second, one of the branches uses a direct return instead of jumping to the cleanup code. Fix these by freeing the commit list and converting the return to a goto. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bisect.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'bisect.c') diff --git a/bisect.c b/bisect.c index 6dbc22f84f..04e9a63f11 100644 --- a/bisect.c +++ b/bisect.c @@ -1031,7 +1031,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) { struct strvec rev_argv = STRVEC_INIT; struct rev_info revs = REV_INFO_INIT; - struct commit_list *tried; + struct commit_list *tried = NULL; int reaches = 0, all = 0, nr, steps; enum bisect_error res = BISECT_OK; struct object_id *bisect_rev; @@ -1098,7 +1098,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) if (oideq(bisect_rev, current_bad_oid)) { res = error_if_skipped_commits(tried, current_bad_oid); if (res) - return res; + goto cleanup; printf("%s is the first %s commit\n", oid_to_hex(bisect_rev), term_bad); @@ -1132,6 +1132,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) res = bisect_checkout(bisect_rev, no_checkout); cleanup: + free_commit_list(tried); release_revisions(&revs); strvec_clear(&rev_argv); return res; -- cgit v1.2.3 From 2b7706aae5b76653bdcb0787a5276a9a53460037 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 14:39:35 +0100 Subject: bisect: fix leaking commit list items in `check_merge_base()` While we free the result commit list at the end of `check_merge_base()`, we forget to free any items that we have already iterated over. Fix this by using a separate variable to iterate through them. This leak is exposed by t6030, but plugging it does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bisect.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'bisect.c') diff --git a/bisect.c b/bisect.c index 04e9a63f11..12efcff2e3 100644 --- a/bisect.c +++ b/bisect.c @@ -851,8 +851,8 @@ static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int rev + 1, &result) < 0) exit(128); - for (; result; result = result->next) { - const struct object_id *mb = &result->item->object.oid; + for (struct commit_list *l = result; l; l = l->next) { + const struct object_id *mb = &l->item->object.oid; if (oideq(mb, current_bad_oid)) { res = handle_bad_merge_base(); break; -- cgit v1.2.3 From c1e98f90103e8d98ef441ce8f609cf3bc8fa538b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 20 Nov 2024 14:39:36 +0100 Subject: bisect: fix various cases where we leak commit list items There are various cases where we leak commit list items because we evict items from the list, but don't free them. Plug those. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bisect.c | 30 ++++++++++++++++++++++-------- t/t6030-bisect-porcelain.sh | 1 + 2 files changed, 23 insertions(+), 8 deletions(-) (limited to 'bisect.c') diff --git a/bisect.c b/bisect.c index 12efcff2e3..f6fa5c235f 100644 --- a/bisect.c +++ b/bisect.c @@ -442,9 +442,12 @@ void find_bisection(struct commit_list **commit_list, int *reaches, best->next = NULL; } *reaches = weight(best); + } else { + free_commit_list(*commit_list); } - free(weights); *commit_list = best; + + free(weights); clear_commit_weight(&commit_weight); } @@ -557,8 +560,11 @@ struct commit_list *filter_skipped(struct commit_list *list, tried = &list->next; } else { if (!show_all) { - if (!skipped_first || !*skipped_first) + if (!skipped_first || !*skipped_first) { + free_commit_list(next); + free_commit_list(filtered); return list; + } } else if (skipped_first && !*skipped_first) { /* This means we know it's not skipped */ *skipped_first = -1; @@ -614,7 +620,7 @@ static int sqrti(int val) static struct commit_list *skip_away(struct commit_list *list, int count) { - struct commit_list *cur, *previous; + struct commit_list *cur, *previous, *result = list; int prn, index, i; prn = get_prn(count); @@ -626,15 +632,23 @@ static struct commit_list *skip_away(struct commit_list *list, int count) for (i = 0; cur; cur = cur->next, i++) { if (i == index) { if (!oideq(&cur->item->object.oid, current_bad_oid)) - return cur; - if (previous) - return previous; - return list; + result = cur; + else if (previous) + result = previous; + else + result = list; + break; } previous = cur; } - return list; + for (cur = list; cur != result; ) { + struct commit_list *next = cur->next; + free(cur); + cur = next; + } + + return result; } static struct commit_list *managed_skipped(struct commit_list *list, diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index cdc0270640..310affadeb 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -9,6 +9,7 @@ exec