mirror of
https://github.com/git/git.git
synced 2026-03-04 14:37:35 +01:00
024b4c9697
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>
54 lines
1.0 KiB
Plaintext
54 lines
1.0 KiB
Plaintext
@@
|
|
expression c;
|
|
@@
|
|
- &c->maybe_tree->object.oid
|
|
+ get_commit_tree_oid(c)
|
|
|
|
@@
|
|
expression c;
|
|
@@
|
|
- c->maybe_tree->object.oid.hash
|
|
+ get_commit_tree_oid(c)->hash
|
|
|
|
@@
|
|
identifier f !~ "^set_commit_tree$";
|
|
expression c;
|
|
expression s;
|
|
@@
|
|
f(...) {<...
|
|
- c->maybe_tree = s
|
|
+ set_commit_tree(c, s)
|
|
...>}
|
|
|
|
// These excluded functions must access c->maybe_tree directly.
|
|
// Note that if c->maybe_tree is written somewhere outside of these
|
|
// functions, then the recommended transformation will be bogus with
|
|
// 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, repo_parse_commit_no_graph };
|
|
expression c;
|
|
@@
|
|
f(...) {<...
|
|
- c->maybe_tree
|
|
+ repo_get_commit_tree(specify_the_right_repo_here, c)
|
|
...>}
|
|
|
|
@@
|
|
struct commit *c;
|
|
expression E;
|
|
@@
|
|
(
|
|
- c->generation = E;
|
|
+ commit_graph_data_at(c)->generation = E;
|
|
|
|
|
- c->graph_pos = E;
|
|
+ commit_graph_data_at(c)->graph_pos = E;
|
|
|
|
|
- c->generation
|
|
+ commit_graph_generation(c)
|
|
|
|
|
- c->graph_pos
|
|
+ commit_graph_position(c)
|
|
)
|