@matheusbsbs / matheus-bagliano / commits / 2ac138d568

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>

Ævar Arnfjörð Bjarmason committed Mar 25, 2019 at 13:08 UTC 2ac138d568abc84e9447825a342365e0136180f8

2 files changed +36 -10

commit-graph.c

+34 -9

@@ -112,6 +112,36 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)

112 return ret;

113 }

114

115 +static int verify_commit_graph_lite(struct commit_graph *g)

116 +{

117 + /*

118 + * Basic validation shared between parse_commit_graph()

119 + * which'll be called every time the graph is used, and the

120 + * much more expensive verify_commit_graph() used by

121 + * "commit-graph verify".

122 + *

123 + * There should only be very basic checks here to ensure that

124 + * we don't e.g. segfault in fill_commit_in_graph(), but

125 + * because this is a very hot codepath nothing that e.g. loops

126 + * over g->num_commits, or runs a checksum on the commit-graph

127 + * itself.

128 + */

129 + if (!g->chunk_oid_fanout) {

130 + error("commit-graph is missing the OID Fanout chunk");

131 + return 1;

132 + }

133 + if (!g->chunk_oid_lookup) {

134 + error("commit-graph is missing the OID Lookup chunk");

135 + return 1;

136 + }

137 + if (!g->chunk_commit_data) {

138 + error("commit-graph is missing the Commit Data chunk");

139 + return 1;

140 + }

141 +

142 + return 0;

143 +}

144 +

145 struct commit_graph *parse_commit_graph(void *graph_map, int fd,

146 size_t graph_size)

147 {

@@ -233,6 +263,9 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,

263 last_chunk_offset = chunk_offset;

264 }

265

266 + if (verify_commit_graph_lite(graph))

267 + return NULL;

268 +

269 return graph;

270 }

271

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

1122 return 1;

1123 }

1124

1092 - verify_commit_graph_error = 0;

1093 -

1094 - if (!g->chunk_oid_fanout)

1095 - graph_report("commit-graph is missing the OID Fanout chunk");

1096 - if (!g->chunk_oid_lookup)

1097 - graph_report("commit-graph is missing the OID Lookup chunk");

1098 - if (!g->chunk_commit_data)

1099 - graph_report("commit-graph is missing the Commit Data chunk");

1100 -

1125 + verify_commit_graph_error = verify_commit_graph_lite(g);

1126 if (verify_commit_graph_error)

1127 return verify_commit_graph_error;

1128

t/t5318-commit-graph.sh

+2 -1

@@ -376,7 +376,8 @@ corrupt_graph_verify() {

376 grepstr=$1

377 test_must_fail git commit-graph verify 2>test_err &&

378 grep -v "^+" test_err >err &&

379 - test_i18ngrep "$grepstr" err

379 + test_i18ngrep "$grepstr" err &&

380 + test_might_fail git status --short

381 }

382

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