commit: make `repo_parse_commit_no_graph()` more robust

In the next commit we will start to parse more commits via the
commit-graph. This change will lead to a segfault though because we try
to access the tree of a commit via `repo_get_commit_tree()`, but:

  - The commit has been parsed via the commit-graph, and thus its
    `maybe_tree` field is not yet populated.

  - We cannot use the commit-graph to populate the commit's tree because
    we're in the process of writing the commit-graph.

The consequence is that we'll get a `NULL` pointer for the tree in
`write_graph_chunk_data()`.

In theory we are already mindful of this situation, as we explicitly use
`repo_parse_commit_no_graph()` to parse the commit without the help of
the commit-graph. But that doesn't do the trick as the commit is already
marked as parsed, so the function will not re-populate it. And as the
commit-graph has been closed, neither will `get_commit_tree_oid()` be
able to load the tree for us.

It seems like this issue can only be hit under artificial circumstances:
the error was hit via `git_test_write_commit_graph_or_die()`, which is
run by git-commit(1) and git-merge(1) in case `GIT_TEST_COMMIT_GRAPH=1`:

  $ GIT_TEST_COMMIT_GRAPH=1 meson test t7507-commit-verbose \
      --test-args=-ix -i
  ...
  ++ git -c commit.verbose=true commit --amend
  hint: Waiting for your editor to close the file...
  ./test-lib.sh: line 1012: 55895 Segmentation fault         (core dumped) git -c commit.verbose=true commit --amend

To the best of my knowledge, this is the only case where we end up
writing a commit-graph in the same process that might have already
consulted the commit-graph to look up arbitrary objects. But regardless
of that, this feels like a bigger accident that is just waiting to
happen.

Make the code more robust by extending `repo_parse_commit_no_graph()` to
unparse a commit first in case we detect it's coming from a graph. This
ensures that we will re-read the object without it, and thus we will
populate `maybe_tree` properly.

This fix shouldn't have any performance consequences: the function is
only ever called in the "commit-graph.c" code, and we'll only re-parse
the commit at most once.

Add an exclusion to our Coccinelle rules so that it doesn't complain
about us accessing `maybe_tree` directly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/commit.h b/commit.h
index 79a761c37df0236b386b18a69e874793de3f1c3c..05165a48a52bcf474b72a904a32cbf1bc835945a 100644 (file)
--- a/commit.h
+++ b/commit.h
@@ -103,16 +103,26 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)
        return repo_parse_commit_gently(r, item, 0);
 }
 
+void unparse_commit(struct repository *r, const struct object_id *oid);
+
 static inline int repo_parse_commit_no_graph(struct repository *r,
                                             struct commit *commit)
 {
+       /*
+        * When the commit has been parsed but its tree wasn't populated then
+        * this is an indicator that it has been parsed via the commit-graph.
+        * We cannot read the tree via the commit-graph, as we're explicitly
+        * told not to use it. We thus have to first un-parse the object so
+        * that we can re-parse it without the graph.
+        */
+       if (commit->object.parsed && !commit->maybe_tree)
+               unparse_commit(r, &commit->object.oid);
+
        return repo_parse_commit_internal(r, commit, 0, 0);
 }
 
 void parse_commit_or_die(struct commit *item);
 
-void unparse_commit(struct repository *r, const struct object_id *oid);
-
 struct buffer_slab;
 struct buffer_slab *allocate_commit_buffer_slab(void);
 void free_commit_buffer_slab(struct buffer_slab *bs);

diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci
index c5284604c566bc78334c29662f35636b59385535..42725161e9f660d25c12001b932939f9b462892b 100644 (file)
--- a/contrib/coccinelle/commit.cocci
+++ b/contrib/coccinelle/commit.cocci
@@ -26,7 +26,7 @@ expression s;
 // repo_get_commit_tree() on the LHS.
 @@
 identifier f != { repo_get_commit_tree, get_commit_tree_in_graph_one,
-                 load_tree_for_commit, set_commit_tree };
+                 load_tree_for_commit, set_commit_tree, repo_parse_commit_no_graph };
 expression c;
 @@
   f(...) {<...