Re: [PATCH 2/3] commit: make `repo_parse_commit_no_graph()` more robust

[Justin Tobler <jltobler@gmail.com>]

On 26/02/16 04:38PM, Patrick Steinhardt wrote:
> 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()`.

IIUC, when a commit has been parsed via the commit graph, if the commit
graph is closed, there is no longer a way for commit object tree to be
read. This results `repo_get_commit_tree()` always returning NULL in
such scenarios.

> 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.

And `repo_parse_commit_no_graph()` doesn't work because the commit has
already been marked as parsed.

> 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.

So I assume we end up closing the commit-graph when writing a new one.
If we need to read the trees of commits parsed via commit-graph, this
will trigger a segfault since the commit tree will always be NULL.

> 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.

Hmm, I wonder if this is conceptually the correct place to address this
problem. Naively, I would expect `repo_get_commit_tree()` to always be
capable of providing the commit tree. I guess the problem though is that
this would require `repo_get_commit_tree()` to detect this scenario and
reparse the object itself. Maybe we could at least have
`repo_get_commit_tree()` BUG() in this scenario though?

> 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);

This unparses commits that were read via commit-graph so they can be
reparsed normally. Looks good.

-Justin