git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 03/17] commit: discard partial cache before (re-)reading it

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Nov 3, 2022, 17:06 UTC
Message-ID
<patch-03.17-c1003454939-20221103T164632Z-avarab@gmail.com>
In-Reply-To
<cover-00.17-00000000000-20221103T164632Z-avarab@gmail.com>

The read_cache() in prepare_to_commit() would end up clobbering the pointer we had for a previously populated "the_index.cache_tree" in the very common case of "git commit" stressed by e.g. the tests being changed here.

We'd populate "the_index.cache_tree" by calling "update_main_cache_tree" in prepare_index(), but would not end up with a "fully prepared" index. What constitutes an existing index is clearly overly fuzzy, here we'll check "active_nr" (aka "the_index.cache_nr"), but our "the_index.cache_tree" might have been malloc()'d already.

Thus the code added in 11c8a74a64a (commit: write cache-tree data when writing index anyway, 2011-12-06) would end up allocating the "cache_tree", and would interact here with code added in 7168624c353 (Do not generate full commit log message if it is not going to be used, 2007-11-28). The result was a very common memory leak.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit.c                        | 7 +++++--
 t/t0068-for-each-repo.sh                | 1 +
 t/t0070-fundamental.sh                  | 1 +
 t/t1404-update-ref-errors.sh            | 2 ++
 t/t1409-avoid-packing-refs.sh           | 1 +
 t/t1413-reflog-detach.sh                | 1 +
 t/t1501-work-tree.sh                    | 2 ++
 t/t2025-checkout-no-overlay.sh          | 1 +
 t/t3009-ls-files-others-nonsubmodule.sh | 1 +
 t/t3010-ls-files-killed-modified.sh     | 2 ++
 t/t4045-diff-relative.sh                | 2 ++
 t/t4111-apply-subdir.sh                 | 1 +
 t/t4135-apply-weird-filenames.sh        | 1 +
 t/t4213-log-tabexpand.sh                | 1 +
 t/t5618-alternate-refs.sh               | 2 ++
 t/t6301-for-each-ref-errors.sh          | 1 +
 t/t7520-ignored-hook-warning.sh         | 1 +
 t/t7614-merge-signoff.sh                | 1 +
 t/t9003-help-autocorrect.sh             | 2 ++
 19 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index e22bdf23f5f..c291199b704 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -987,8 +987,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		struct object_id oid;
 		const char *parent = "HEAD";
 
-		if (!active_nr && read_cache() < 0)
-			die(_("Cannot read index"));
+		if (!active_nr) {
+			discard_cache();
+			if (read_cache() < 0)
+				die(_("Cannot read index"));
+		}
 
 		if (amend)
 			parent = "HEAD^1";
diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh
index 4675e852517..9f63f612425 100755
--- a/t/t0068-for-each-repo.sh
+++ b/t/t0068-for-each-repo.sh
@@ -2,6 +2,7 @@
 
 test_description='git for-each-repo builtin'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'run based on configured value' '
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 8d59905ef09..574de341980 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -6,6 +6,7 @@ test_description='check that the most basic functions work
 Verify wrappers and compatibility functions.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'character classes (isspace, isalpha etc.)' '
diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 13c2b43bbae..b5606d93b52 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='Test git update-ref error handling'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Create some references, perhaps run pack-refs --all, then try to
diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index be12fb63506..f23c0152a85 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -2,6 +2,7 @@
 
 test_description='avoid rewriting packed-refs unnecessarily'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Add an identifying mark to the packed-refs file header line. This
diff --git a/t/t1413-reflog-detach.sh b/t/t1413-reflog-detach.sh
index 934688a1ee8..d2a4822d46f 100755
--- a/t/t1413-reflog-detach.sh
+++ b/t/t1413-reflog-detach.sh
@@ -4,6 +4,7 @@ test_description='Test reflog interaction with detached HEAD'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 reset_state () {
diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
index b75558040ff..ae6528aecea 100755
--- a/t/t1501-work-tree.sh
+++ b/t/t1501-work-tree.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test separate work tree'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t2025-checkout-no-overlay.sh b/t/t2025-checkout-no-overlay.sh
index 8f13341cf8e..3832c3de813 100755
--- a/t/t2025-checkout-no-overlay.sh
+++ b/t/t2025-checkout-no-overlay.sh
@@ -2,6 +2,7 @@
 
 test_description='checkout --no-overlay <tree-ish> -- <pathspec>'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3009-ls-files-others-nonsubmodule.sh b/t/t3009-ls-files-others-nonsubmodule.sh
index 963f3462b75..14218b34243 100755
--- a/t/t3009-ls-files-others-nonsubmodule.sh
+++ b/t/t3009-ls-files-others-nonsubmodule.sh
@@ -18,6 +18,7 @@ This test runs git ls-files --others with the following working tree:
       git repository with a commit and an untracked file
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup: directories' '
diff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh
index 580e158f991..054178703d7 100755
--- a/t/t3010-ls-files-killed-modified.sh
+++ b/t/t3010-ls-files-killed-modified.sh
@@ -41,6 +41,8 @@ Also for modification test, the cache and working tree have:
 We should report path0, path1, path2/file2, path3/file3, path7 and path8
 modified without reporting path9 and path10.  submod1 is also modified.
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'git update-index --add to add various paths.' '
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index fab351b48a1..198dfc91906 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='diff --relative tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4111-apply-subdir.sh b/t/t4111-apply-subdir.sh
index 1618a6dbc7c..e9a87d761d1 100755
--- a/t/t4111-apply-subdir.sh
+++ b/t/t4111-apply-subdir.sh
@@ -2,6 +2,7 @@
 
 test_description='patching from inconvenient places'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4135-apply-weird-filenames.sh b/t/t4135-apply-weird-filenames.sh
index 6bc3fb97a75..d3502c6fddf 100755
--- a/t/t4135-apply-weird-filenames.sh
+++ b/t/t4135-apply-weird-filenames.sh
@@ -2,6 +2,7 @@
 
 test_description='git apply with weird postimage filenames'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4213-log-tabexpand.sh b/t/t4213-log-tabexpand.sh
index 53a4af32449..590fce95e90 100755
--- a/t/t4213-log-tabexpand.sh
+++ b/t/t4213-log-tabexpand.sh
@@ -2,6 +2,7 @@
 
 test_description='log/show --expand-tabs'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 HT="	"
diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
index 3353216f09e..f905db0a3fd 100755
--- a/t/t5618-alternate-refs.sh
+++ b/t/t5618-alternate-refs.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test handling of --alternate-refs traversal'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Avoid test_commit because we want a specific and known set of refs:
diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
index 40edf9dab53..bfda1f46ad2 100755
--- a/t/t6301-for-each-ref-errors.sh
+++ b/t/t6301-for-each-ref-errors.sh
@@ -2,6 +2,7 @@
 
 test_description='for-each-ref errors for broken refs'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 ZEROS=$ZERO_OID
diff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh
index dc57526e6f1..184b2589893 100755
--- a/t/t7520-ignored-hook-warning.sh
+++ b/t/t7520-ignored-hook-warning.sh
@@ -2,6 +2,7 @@
 
 test_description='ignored hook warning'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t7614-merge-signoff.sh b/t/t7614-merge-signoff.sh
index fee258d4f0c..cf96a35e8e7 100755
--- a/t/t7614-merge-signoff.sh
+++ b/t/t7614-merge-signoff.sh
@@ -8,6 +8,7 @@ This test runs git merge --signoff and makes sure that it works.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Setup test files
diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh
index f00deaf3815..4b9cb4c942f 100755
--- a/t/t9003-help-autocorrect.sh
+++ b/t/t9003-help-autocorrect.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='help.autocorrect finding a match'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '

-- 
2.38.0.1451.g86b35f4140a
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 15 of 45 in “leak fixes: use existing constructors & other trivia”
  1. 00/17 leak fixes: use existing constructors & other triviaÆvar Arnfjörð Bjarmason, Nov 3, 2022
  2. 02/17 {reset,merge}: call discard_index() before returningÆvar Arnfjörð Bjarmason, Nov 3, 2022
  3. 01/17 tests: mark tests as passing with SANITIZE=leakÆvar Arnfjörð Bjarmason, Nov 3, 2022
  4. 04/17 read-cache.c: clear and free "sparse_checkout_patterns"Ævar Arnfjörð Bjarmason, Nov 3, 2022
  5. 05/17 dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache"Ævar Arnfjörð Bjarmason, Nov 3, 2022
  6. 06/17 built-ins & libs & helpers: add/move destructors, fix leaksÆvar Arnfjörð Bjarmason, Nov 3, 2022
  7. 07/17 unpack-file: fix ancient leak in create_temp_file()Ævar Arnfjörð Bjarmason, Nov 3, 2022
  8. 09/17 ls-files: fix a --with-tree memory leakÆvar Arnfjörð Bjarmason, Nov 3, 2022
  9. 10/17 sequencer.c: fix "opts->strategy" leak in read_strategy_opts()Ævar Arnfjörð Bjarmason, Nov 3, 2022
  10. Phillip WoodNov 4, 2022
  11. Taylor BlauNov 4, 2022
  12. Ævar Arnfjörð BjarmasonNov 5, 2022
  13. Phillip WoodNov 8, 2022
  14. Ævar Arnfjörð BjarmasonNov 8, 2022
  15. 03/17 commit: discard partial cache before (re-)reading itÆvar Arnfjörð Bjarmason, Nov 3, 2022
  16. 08/17 revision API: call graph_clear() in release_revisions()Ævar Arnfjörð Bjarmason, Nov 3, 2022
  17. 12/17 sequencer.c: fix a pick_commits() leakÆvar Arnfjörð Bjarmason, Nov 3, 2022
  18. 11/17 connected.c: free the "struct packed_git"Ævar Arnfjörð Bjarmason, Nov 3, 2022
  19. 13/17 rebase: don't leak on "--abort"Ævar Arnfjörð Bjarmason, Nov 3, 2022
  20. Phillip WoodNov 4, 2022
  21. Ævar Arnfjörð BjarmasonNov 5, 2022
  22. 16/17 revert: fix parse_options_concat() leakÆvar Arnfjörð Bjarmason, Nov 3, 2022
  23. 14/17 sequencer.c: fix sequencer_continue() leakÆvar Arnfjörð Bjarmason, Nov 3, 2022
  24. 15/17 cherry-pick: free "struct replay_opts" membersÆvar Arnfjörð Bjarmason, Nov 3, 2022
  25. 17/17 built-ins: use free() not UNLEAK() if trivial, rm dead codeÆvar Arnfjörð Bjarmason, Nov 3, 2022
  26. Phillip WoodNov 4, 2022
  27. Ævar Arnfjörð BjarmasonNov 5, 2022
  28. Phillip WoodNov 7, 2022
  29. 00/15 leak fixes: use existing constructors & other triviaÆvar Arnfjörð Bjarmason, Nov 8, 2022
  30. 01/15 tests: mark tests as passing with SANITIZE=leakÆvar Arnfjörð Bjarmason, Nov 8, 2022
  31. 02/15 {reset,merge}: call discard_index() before returningÆvar Arnfjörð Bjarmason, Nov 8, 2022
  32. 04/15 read-cache.c: clear and free "sparse_checkout_patterns"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  33. 03/15 commit: discard partial cache before (re-)reading itÆvar Arnfjörð Bjarmason, Nov 8, 2022
  34. 05/15 dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  35. 06/15 built-ins & libs & helpers: add/move destructors, fix leaksÆvar Arnfjörð Bjarmason, Nov 8, 2022
  36. 08/15 revision API: call graph_clear() in release_revisions()Ævar Arnfjörð Bjarmason, Nov 8, 2022
  37. 07/15 unpack-file: fix ancient leak in create_temp_file()Ævar Arnfjörð Bjarmason, Nov 8, 2022
  38. 09/15 ls-files: fix a --with-tree memory leakÆvar Arnfjörð Bjarmason, Nov 8, 2022
  39. 10/15 sequencer.c: fix "opts->strategy" leak in read_strategy_opts()Ævar Arnfjörð Bjarmason, Nov 8, 2022
  40. 11/15 connected.c: free the "struct packed_git"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  41. 13/15 cherry-pick: free "struct replay_opts" membersÆvar Arnfjörð Bjarmason, Nov 8, 2022
  42. 12/15 rebase: don't leak on "--abort"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  43. 15/15 built-ins: use free() not UNLEAK() if trivial, rm dead codeÆvar Arnfjörð Bjarmason, Nov 8, 2022
  44. 14/15 revert: fix parse_options_concat() leakÆvar Arnfjörð Bjarmason, Nov 8, 2022
  45. Taylor BlauNov 8, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.