[PATCH 2/3] commit: make `repo_parse_commit_no_graph()` more robust
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Feb 16, 2026, 15:38 UTC
- Message-ID
- <20260216-b4-pks-receive-pack-optimize-shallow-v1-2-e98886daff2b@pks.im>
- In-Reply-To
- <20260216-b4-pks-receive-pack-optimize-shallow-v1-0-e98886daff2b@pks.im>
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 --amendTo 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> --- commit.h | 14 ++++++++++++-- contrib/coccinelle/commit.cocci | 2 +- 2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/commit.h b/commit.h index 1635de418b..f2f39e1a89 100644 --- 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 c5284604c5..42725161e9 100644 --- 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(...) {<...
-- 2.53.0.352.gd1286b26eb.dirty