commit-graph: fix segfault on e.g. "git status"

When core.commitGraph=true is set, various common commands now consult
the commit graph. Because the commit-graph code is very trusting of
its input data, it's possibly to construct a graph that'll cause an
immediate segfault on e.g. "status" (and e.g. "log", "blame", ...). In
some other cases where git immediately exits with a cryptic error
about the graph being broken.

The root cause of this is that while the "commit-graph verify"
sub-command exhaustively verifies the graph, other users of the graph
simply trust the graph, and will e.g. deference data found at certain
offsets as pointers, causing segfaults.

This change does the bare minimum to ensure that we don't segfault in
the common fill_commit_in_graph() codepath called by
e.g. setup_revisions(), to do this instrument the "commit-graph
verify" tests to always check if "status" would subsequently
segfault. This fixes the following tests which would previously
segfault:

not ok 50 - detect low chunk count
not ok 51 - detect missing OID fanout chunk
not ok 52 - detect missing OID lookup chunk
not ok 53 - detect missing commit data chunk

Those happened because with the commit-graph enabled setup_revisions()
would eventually call fill_commit_in_graph(), where e.g.
g->chunk_commit_data is used early as an offset (and will be
0x0). With this change we get far enough to detect that the graph is
broken, and show an error instead. E.g.:

$ git status; echo $?
error: commit-graph is missing the Commit Data chunk
1

That also sucks, we should *warn* and not hard-fail "status" just
because the commit-graph is corrupt, but fixing is left to a follow-up
change.

A side-effect of changing the reporting from graph_report() to error()
is that we now have an "error: " prefix for these even for
"commit-graph verify". Pseudo-diff before/after:

$ git commit-graph verify
-commit-graph is missing the Commit Data chunk
+error: commit-graph is missing the Commit Data chunk

Changing that is OK. Various errors it emits now early on are prefixed
with "error: ", moving these over and changing the output doesn't
break anything.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/commit-graph.c b/commit-graph.c
index 47e9be0a3aad883c17221b11c972b8376a70a555..f8201d888b9a26299b3c7b0eb7cc48ad546dd7f2 100644 (file)
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -112,6 +112,36 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
        return ret;
 }
 
        return ret;
 }
 

+static int verify_commit_graph_lite(struct commit_graph *g)
+{
+       /*
+        * Basic validation shared between parse_commit_graph()
+        * which'll be called every time the graph is used, and the
+        * much more expensive verify_commit_graph() used by
+        * "commit-graph verify".
+        *
+        * There should only be very basic checks here to ensure that
+        * we don't e.g. segfault in fill_commit_in_graph(), but
+        * because this is a very hot codepath nothing that e.g. loops
+        * over g->num_commits, or runs a checksum on the commit-graph
+        * itself.
+        */
+       if (!g->chunk_oid_fanout) {
+               error("commit-graph is missing the OID Fanout chunk");
+               return 1;
+       }
+       if (!g->chunk_oid_lookup) {
+               error("commit-graph is missing the OID Lookup chunk");
+               return 1;
+       }
+       if (!g->chunk_commit_data) {
+               error("commit-graph is missing the Commit Data chunk");
+               return 1;
+       }
+
+       return 0;
+}
+

 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
                                        size_t graph_size)
 {
 struct commit_graph *parse_commit_graph(void *graph_map, int fd,
                                        size_t graph_size)
 {


@@ -233,6 +263,9 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
                last_chunk_offset = chunk_offset;
        }
 
                last_chunk_offset = chunk_offset;
        }
 

+       if (verify_commit_graph_lite(graph))
+               return NULL;
+

        return graph;
 }
 
        return graph;
 }
 


@@ -1089,15 +1122,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
                return 1;
        }
 
                return 1;
        }
 

-       verify_commit_graph_error = 0;
-
-       if (!g->chunk_oid_fanout)
-               graph_report("commit-graph is missing the OID Fanout chunk");
-       if (!g->chunk_oid_lookup)
-               graph_report("commit-graph is missing the OID Lookup chunk");
-       if (!g->chunk_commit_data)
-               graph_report("commit-graph is missing the Commit Data chunk");
-
+       verify_commit_graph_error = verify_commit_graph_lite(g);

        if (verify_commit_graph_error)
                return verify_commit_graph_error;
 
        if (verify_commit_graph_error)
                return verify_commit_graph_error;
 

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 733be2ed30c2ea1fdd3d920dec43a30227a98634..b3f3e515fc011cdc5c3582f7fae38682570b7068 100755 (executable)
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -376,7 +376,8 @@ corrupt_graph_verify() {
        grepstr=$1
        test_must_fail git commit-graph verify 2>test_err &&
        grep -v "^+" test_err >err &&
        grepstr=$1
        test_must_fail git commit-graph verify 2>test_err &&
        grep -v "^+" test_err >err &&

-       test_i18ngrep "$grepstr" err
+       test_i18ngrep "$grepstr" err &&
+       test_might_fail git status --short

 }
 
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
 }
 
 # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]