@matheusbsbs / matheus-bagliano / commits / a90a089611

revision: free diff options

There is a todo comment in `release_revisions()` that mentions that we need to free the diff options, which was added via 54c8a7c379 (revisions API: add a TODO for diff_free(&revs->diffopt), 2022-04-14). Releasing the diff options wasn't quite feasible at that time because some call sites rely on its contents to remain even after the revisions have been released. In fact, there really only are a couple of callsites that misbehave here: - `cmd_shortlog()` releases the revisions, but continues to access its file pointer. - `do_diff_cache()` creates a shallow copy of `struct diff_options`, but does not set the `no_free` member. Consequently, we end up releasing resources of the caller-provided diff options. - `diff_free()` and friends do not play nice when being called multiple times as they don't unset data structures that they have just released. Fix all of those cases and enable the call to `diff_free()`, which plugs a bunch of memory leaks. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Jun 11, 2024 at 11:20 UTC a90a08961190dda2f664e102822fb6a7152e65d5
11 files changed +17 -7
builtin/shortlog.c
+1 -4
@@ -460,11 +460,8 @@ parse_done:
460 else
461 get_from_rev(&rev, &log);
462
463 - release_revisions(&rev);
464 -
463 shortlog_output(&log);
466 - if (log.file != stdout)
467 - fclose(log.file);
464 + release_revisions(&rev);
465 return 0;
466 }
467
diff-lib.c
+2
@@ -662,9 +662,11 @@ int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt)
662 repo_init_revisions(opt->repo, &revs, NULL);
663 copy_pathspec(&revs.prune_data, &opt->pathspec);
664 revs.diffopt = *opt;
665 + revs.diffopt.no_free = 1;
666
667 if (diff_cache(&revs, tree_oid, NULL, 1))
668 exit(128);
669 +
670 release_revisions(&revs);
671 return 0;
672 }
diff.c
+6 -2
@@ -6649,8 +6649,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
6649
6650 static void diff_free_file(struct diff_options *options)
6651 {
6652 - if (options->close_file)
6652 + if (options->close_file && options->file) {
6653 fclose(options->file);
6654 + options->file = NULL;
6655 + }
6656 }
6657
6658 static void diff_free_ignore_regex(struct diff_options *options)
@@ -6661,7 +6663,9 @@ static void diff_free_ignore_regex(struct diff_options *options)
6663 regfree(options->ignore_regex[i]);
6664 free(options->ignore_regex[i]);
6665 }
6664 - free(options->ignore_regex);
6666 +
6667 + FREE_AND_NULL(options->ignore_regex);
6668 + options->ignore_regex_nr = 0;
6669 }
6670
6671 void diff_free(struct diff_options *options)
revision.c
+1 -1
@@ -3191,7 +3191,7 @@ void release_revisions(struct rev_info *revs)
3191 release_revisions_mailmap(revs->mailmap);
3192 free_grep_patterns(&revs->grep_filter);
3193 graph_clear(revs->graph);
3194 - /* TODO (need to handle "no_free"): diff_free(&revs->diffopt) */
3194 + diff_free(&revs->diffopt);
3195 diff_free(&revs->pruning);
3196 reflog_walk_info_release(revs->reflog_info);
3197 release_revisions_topo_walk_info(revs->topo_walk_info);
t/t4208-log-magic-pathspec.sh
+1
@@ -5,6 +5,7 @@ test_description='magic pathspec tests using git-log'
5 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
6 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
7
8 +TEST_PASSES_SANITIZE_LEAK=true
9 . ./test-lib.sh
10
11 test_expect_success 'setup' '
t/t6000-rev-list-misc.sh
+1
@@ -5,6 +5,7 @@ test_description='miscellaneous rev-list tests'
5 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
6 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
7
8 +TEST_PASSES_SANITIZE_LEAK=true
9 . ./test-lib.sh
10
11 test_expect_success setup '
t/t6001-rev-list-graft.sh
+1
@@ -5,6 +5,7 @@ test_description='Revision traversal vs grafts and path limiter'
5 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
6 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
7
8 +TEST_PASSES_SANITIZE_LEAK=true
9 . ./test-lib.sh
10
11 test_expect_success setup '
t/t6013-rev-list-reverse-parents.sh
+1
@@ -5,6 +5,7 @@ test_description='--reverse combines with --parents'
5 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
6 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
7
8 +TEST_PASSES_SANITIZE_LEAK=true
9 . ./test-lib.sh
10
11
t/t6017-rev-list-stdin.sh
+1
@@ -8,6 +8,7 @@ test_description='log family learns --stdin'
8 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
9 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
10
11 +TEST_PASSES_SANITIZE_LEAK=true
12 . ./test-lib.sh
13
14 check () {
t/t9500-gitweb-standalone-no-errors.sh
+1
@@ -13,6 +13,7 @@ or warnings to log.'
13 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
14 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
15
16 +TEST_PASSES_SANITIZE_LEAK=true
17 . ./lib-gitweb.sh
18
19 # ----------------------------------------------------------------------
t/t9502-gitweb-standalone-parse-output.sh
+1
@@ -13,6 +13,7 @@ in the HTTP header or the actual script output.'
13 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
14 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
15
16 +TEST_PASSES_SANITIZE_LEAK=true
17 . ./lib-gitweb.sh
18
19 # ----------------------------------------------------------------------