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>

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index d4daf31e2285e2414c77a8b1a9754b79d310e4db..5bde7c68c26aaaef06cd8b06474a4b0d3b600f2b 100644 (file)
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -460,11 +460,8 @@ parse_done:
        else
                get_from_rev(&rev, &log);
 
-       release_revisions(&rev);
-
        shortlog_output(&log);
-       if (log.file != stdout)
-               fclose(log.file);
+       release_revisions(&rev);
        return 0;
 }
 

diff --git a/diff-lib.c b/diff-lib.c
index 5a5a50c5a15a9e449aa0c41b4c0770330cc1aa19..1cbf03bf39085ee7d8070db907c1a470cf670612 100644 (file)
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -662,9 +662,11 @@ int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt)
        repo_init_revisions(opt->repo, &revs, NULL);
        copy_pathspec(&revs.prune_data, &opt->pathspec);
        revs.diffopt = *opt;
+       revs.diffopt.no_free = 1;
 
        if (diff_cache(&revs, tree_oid, NULL, 1))
                exit(128);
+
        release_revisions(&revs);
        return 0;
 }

diff --git a/diff.c b/diff.c
index e70301df76743ab196d8f046748d4cb1ca0a5434..53e2f5a42e14243b4cb0257ca82f18e1febc1e75 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -6649,8 +6649,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
 
 static void diff_free_file(struct diff_options *options)
 {
-       if (options->close_file)
+       if (options->close_file && options->file) {
                fclose(options->file);
+               options->file = NULL;
+       }
 }
 
 static void diff_free_ignore_regex(struct diff_options *options)
@@ -6661,7 +6663,9 @@ static void diff_free_ignore_regex(struct diff_options *options)
                regfree(options->ignore_regex[i]);
                free(options->ignore_regex[i]);
        }
-       free(options->ignore_regex);
+
+       FREE_AND_NULL(options->ignore_regex);
+       options->ignore_regex_nr = 0;
 }
 
 void diff_free(struct diff_options *options)

diff --git a/revision.c b/revision.c
index 82c0aadb42ce984bcae223fd569e85e7023968a3..99c75c939de3644146fde07101a3330d759ae4da 100644 (file)
--- a/revision.c
+++ b/revision.c
@@ -3191,7 +3191,7 @@ void release_revisions(struct rev_info *revs)
        release_revisions_mailmap(revs->mailmap);
        free_grep_patterns(&revs->grep_filter);
        graph_clear(revs->graph);
-       /* TODO (need to handle "no_free"): diff_free(&revs->diffopt) */
+       diff_free(&revs->diffopt);
        diff_free(&revs->pruning);
        reflog_walk_info_release(revs->reflog_info);
        release_revisions_topo_walk_info(revs->topo_walk_info);

diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh
index 806b2809d405f854f1d1f8b0d7156ce2c167e38a..2a46eb6bedbe283a08fd77917b7fb17da155b027 100755 (executable)
--- a/t/t4208-log-magic-pathspec.sh
+++ b/t/t4208-log-magic-pathspec.sh
@@ -5,6 +5,7 @@ test_description='magic pathspec tests using git-log'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '

diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 6289a2e8b03890ded5863f3472e469c28cb3034a..f6d17ee9025100ba565da16c1ef97c1f8068e25a 100755 (executable)
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -5,6 +5,7 @@ test_description='miscellaneous rev-list tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '

diff --git a/t/t6001-rev-list-graft.sh b/t/t6001-rev-list-graft.sh
index 73a2465aa0eca662e69f1efd49f285786f5bd9c1..3553bbbfe73bd085eed955939ed7b721aa1a4892 100755 (executable)
--- a/t/t6001-rev-list-graft.sh
+++ b/t/t6001-rev-list-graft.sh
@@ -5,6 +5,7 @@ test_description='Revision traversal vs grafts and path limiter'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '

diff --git a/t/t6013-rev-list-reverse-parents.sh b/t/t6013-rev-list-reverse-parents.sh
index 39793cbbd661afe7fe9b9bab2302b023e4ef1d0a..4128269c1d481bf4837e62fdbce0982196e16d33 100755 (executable)
--- a/t/t6013-rev-list-reverse-parents.sh
+++ b/t/t6013-rev-list-reverse-parents.sh
@@ -5,6 +5,7 @@ test_description='--reverse combines with --parents'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 

diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh
index 4821b90e7479ad8f4878ab4432ce0e9d2ce47de5..a0a40fe55cd8f78eaf2182c73754088fb38d4388 100755 (executable)
--- a/t/t6017-rev-list-stdin.sh
+++ b/t/t6017-rev-list-stdin.sh
@@ -8,6 +8,7 @@ test_description='log family learns --stdin'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check () {

diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index 7679780fb87b4f14e9c0f5a22708b1e8925a4d53..ccfa4153840acfdcf9e2002baa1bcf90a7ccd0a5 100755 (executable)
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -13,6 +13,7 @@ or warnings to log.'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-gitweb.sh
 
 # ----------------------------------------------------------------------

diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh
index 81d562555799bebe80b5cf36b1063e28d377ea8c..b41ea1933143814d895be93053382052637c34b4 100755 (executable)
--- a/t/t9502-gitweb-standalone-parse-output.sh
+++ b/t/t9502-gitweb-standalone-parse-output.sh
@@ -13,6 +13,7 @@ in the HTTP header or the actual script output.'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-gitweb.sh
 
 # ----------------------------------------------------------------------