Merge branch 'ab/commit-graph-fixes'

Code cleanup with more careful error checking before using data
read from the commit-graph file.

* ab/commit-graph-fixes:
  commit-graph: improve & i18n error messages
  commit-graph write: don't die if the existing graph is corrupt
  commit-graph verify: detect inability to read the graph
  commit-graph: don't pass filename to load_commit_graph_one_fd_st()
  commit-graph: don't early exit(1) on e.g. "git status"
  commit-graph: fix segfault on e.g. "git status"
  commit-graph tests: test a graph that's too small
  commit-graph tests: split up corrupt_graph_and_verify()
This commit is contained in:
Junio C Hamano 2019-04-25 16:41:15 +09:00
commit a5e4be2f68
5 changed files with 153 additions and 54 deletions

View file

@ -42,6 +42,9 @@ static int graph_verify(int argc, const char **argv)
{
struct commit_graph *graph = NULL;
char *graph_name;
int open_ok;
int fd;
struct stat st;
static struct option builtin_commit_graph_verify_options[] = {
OPT_STRING(0, "object-dir", &opts.obj_dir,
@ -58,11 +61,16 @@ static int graph_verify(int argc, const char **argv)
opts.obj_dir = get_object_directory();
graph_name = get_commit_graph_filename(opts.obj_dir);
graph = load_commit_graph_one(graph_name);
open_ok = open_commit_graph(graph_name, &fd, &st);
if (!open_ok && errno == ENOENT)
return 0;
if (!open_ok)
die_errno(_("Could not open commit-graph '%s'"), graph_name);
graph = load_commit_graph_one_fd_st(fd, &st);
FREE_AND_NULL(graph_name);
if (!graph)
return 0;
return 1;
UNLEAK(graph);
return verify_commit_graph(the_repository, graph);
@ -72,6 +80,9 @@ static int graph_read(int argc, const char **argv)
{
struct commit_graph *graph = NULL;
char *graph_name;
int open_ok;
int fd;
struct stat st;
static struct option builtin_commit_graph_read_options[] = {
OPT_STRING(0, "object-dir", &opts.obj_dir,
@ -88,10 +99,14 @@ static int graph_read(int argc, const char **argv)
opts.obj_dir = get_object_directory();
graph_name = get_commit_graph_filename(opts.obj_dir);
graph = load_commit_graph_one(graph_name);
open_ok = open_commit_graph(graph_name, &fd, &st);
if (!open_ok)
die_errno(_("Could not open commit-graph '%s'"), graph_name);
graph = load_commit_graph_one_fd_st(fd, &st);
if (!graph)
die("graph file %s does not exist", graph_name);
return 1;
FREE_AND_NULL(graph_name);

View file

@ -80,25 +80,30 @@ static int commit_graph_compatible(struct repository *r)
return 1;
}
struct commit_graph *load_commit_graph_one(const char *graph_file)
int open_commit_graph(const char *graph_file, int *fd, struct stat *st)
{
*fd = git_open(graph_file);
if (*fd < 0)
return 0;
if (fstat(*fd, st)) {
close(*fd);
return 0;
}
return 1;
}
struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st)
{
void *graph_map;
size_t graph_size;
struct stat st;
struct commit_graph *ret;
int fd = git_open(graph_file);
if (fd < 0)
return NULL;
if (fstat(fd, &st)) {
close(fd);
return NULL;
}
graph_size = xsize_t(st.st_size);
graph_size = xsize_t(st->st_size);
if (graph_size < GRAPH_MIN_SIZE) {
close(fd);
die(_("graph file %s is too small"), graph_file);
error(_("commit-graph file is too small"));
return NULL;
}
graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
ret = parse_commit_graph(graph_map, fd, graph_size);
@ -106,12 +111,41 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
if (!ret) {
munmap(graph_map, graph_size);
close(fd);
exit(1);
}
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)
{
@ -133,21 +167,21 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
graph_signature = get_be32(data);
if (graph_signature != GRAPH_SIGNATURE) {
error(_("graph signature %X does not match signature %X"),
error(_("commit-graph signature %X does not match signature %X"),
graph_signature, GRAPH_SIGNATURE);
return NULL;
}
graph_version = *(unsigned char*)(data + 4);
if (graph_version != GRAPH_VERSION) {
error(_("graph version %X does not match version %X"),
error(_("commit-graph version %X does not match version %X"),
graph_version, GRAPH_VERSION);
return NULL;
}
hash_version = *(unsigned char*)(data + 5);
if (hash_version != oid_version()) {
error(_("hash version %X does not match version %X"),
error(_("commit-graph hash version %X does not match version %X"),
hash_version, oid_version());
return NULL;
}
@ -170,7 +204,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
if (data + graph_size - chunk_lookup <
GRAPH_CHUNKLOOKUP_WIDTH) {
error(_("chunk lookup table entry missing; graph file may be incomplete"));
error(_("commit-graph chunk lookup table entry missing; file may be incomplete"));
free(graph);
return NULL;
}
@ -181,7 +215,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
chunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;
if (chunk_offset > graph_size - the_hash_algo->rawsz) {
error(_("improper chunk offset %08x%08x"), (uint32_t)(chunk_offset >> 32),
error(_("commit-graph improper chunk offset %08x%08x"), (uint32_t)(chunk_offset >> 32),
(uint32_t)chunk_offset);
free(graph);
return NULL;
@ -218,7 +252,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
}
if (chunk_repeated) {
error(_("chunk id %08x appears multiple times"), chunk_id);
error(_("commit-graph chunk id %08x appears multiple times"), chunk_id);
free(graph);
return NULL;
}
@ -233,9 +267,25 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
last_chunk_offset = chunk_offset;
}
if (verify_commit_graph_lite(graph))
return NULL;
return graph;
}
static struct commit_graph *load_commit_graph_one(const char *graph_file)
{
struct stat st;
int fd;
int open_ok = open_commit_graph(graph_file, &fd, &st);
if (!open_ok)
return NULL;
return load_commit_graph_one_fd_st(fd, &st);
}
static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
{
char *graph_name;
@ -261,6 +311,10 @@ static int prepare_commit_graph(struct repository *r)
struct object_directory *odb;
int config_value;
if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))
die("dying as requested by the '%s' variable on commit-graph load!",
GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);
if (r->objects->commit_graph_attempted)
return !!r->objects->commit_graph;
r->objects->commit_graph_attempted = 1;
@ -525,7 +579,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
uint32_t packedDate[2];
display_progress(progress, ++*progress_cnt);
parse_commit(*list);
parse_commit_no_graph(*list);
hashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);
parent = (*list)->parents;
@ -722,7 +776,7 @@ static void close_reachable(struct packed_oid_list *oids, int report_progress)
display_progress(progress, i + 1);
commit = lookup_commit(the_repository, &oids->list[i]);
if (commit && !parse_commit(commit))
if (commit && !parse_commit_no_graph(commit))
add_missing_parents(oids, commit);
}
stop_progress(&progress);
@ -971,7 +1025,7 @@ void write_commit_graph(const char *obj_dir,
continue;
commits.list[commits.nr] = lookup_commit(the_repository, &oids.list[i]);
parse_commit(commits.list[commits.nr]);
parse_commit_no_graph(commits.list[commits.nr]);
for (parent = commits.list[commits.nr]->parents;
parent; parent = parent->next)
@ -1089,15 +1143,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
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;
@ -1116,7 +1162,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
if (i && oidcmp(&prev_oid, &cur_oid) >= 0)
graph_report("commit-graph has incorrect OID order: %s then %s",
graph_report(_("commit-graph has incorrect OID order: %s then %s"),
oid_to_hex(&prev_oid),
oid_to_hex(&cur_oid));
@ -1126,14 +1172,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
uint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);
if (i != fanout_value)
graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",
graph_report(_("commit-graph has incorrect fanout value: fanout[%d] = %u != %u"),
cur_fanout_pos, fanout_value, i);
cur_fanout_pos++;
}
graph_commit = lookup_commit(r, &cur_oid);
if (!parse_commit_in_graph_one(r, g, graph_commit))
graph_report("failed to parse %s from commit-graph",
graph_report(_("failed to parse commit %s from commit-graph"),
oid_to_hex(&cur_oid));
}
@ -1141,7 +1187,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
uint32_t fanout_value = get_be32(g->chunk_oid_fanout + cur_fanout_pos);
if (g->num_commits != fanout_value)
graph_report("commit-graph has incorrect fanout value: fanout[%d] = %u != %u",
graph_report(_("commit-graph has incorrect fanout value: fanout[%d] = %u != %u"),
cur_fanout_pos, fanout_value, i);
cur_fanout_pos++;
@ -1163,14 +1209,14 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
graph_commit = lookup_commit(r, &cur_oid);
odb_commit = (struct commit *)create_object(r, cur_oid.hash, alloc_commit_node(r));
if (parse_commit_internal(odb_commit, 0, 0)) {
graph_report("failed to parse %s from object database",
graph_report(_("failed to parse commit %s from object database for commit-graph"),
oid_to_hex(&cur_oid));
continue;
}
if (!oideq(&get_commit_tree_in_graph_one(r, g, graph_commit)->object.oid,
get_commit_tree_oid(odb_commit)))
graph_report("root tree OID for commit %s in commit-graph is %s != %s",
graph_report(_("root tree OID for commit %s in commit-graph is %s != %s"),
oid_to_hex(&cur_oid),
oid_to_hex(get_commit_tree_oid(graph_commit)),
oid_to_hex(get_commit_tree_oid(odb_commit)));
@ -1180,13 +1226,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
while (graph_parents) {
if (odb_parents == NULL) {
graph_report("commit-graph parent list for commit %s is too long",
graph_report(_("commit-graph parent list for commit %s is too long"),
oid_to_hex(&cur_oid));
break;
}
if (!oideq(&graph_parents->item->object.oid, &odb_parents->item->object.oid))
graph_report("commit-graph parent for %s is %s != %s",
graph_report(_("commit-graph parent for %s is %s != %s"),
oid_to_hex(&cur_oid),
oid_to_hex(&graph_parents->item->object.oid),
oid_to_hex(&odb_parents->item->object.oid));
@ -1199,16 +1245,16 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
}
if (odb_parents != NULL)
graph_report("commit-graph parent list for commit %s terminates early",
graph_report(_("commit-graph parent list for commit %s terminates early"),
oid_to_hex(&cur_oid));
if (!graph_commit->generation) {
if (generation_zero == GENERATION_NUMBER_EXISTS)
graph_report("commit-graph has generation number zero for commit %s, but non-zero elsewhere",
graph_report(_("commit-graph has generation number zero for commit %s, but non-zero elsewhere"),
oid_to_hex(&cur_oid));
generation_zero = GENERATION_ZERO_EXISTS;
} else if (generation_zero == GENERATION_ZERO_EXISTS)
graph_report("commit-graph has non-zero generation number for commit %s, but zero elsewhere",
graph_report(_("commit-graph has non-zero generation number for commit %s, but zero elsewhere"),
oid_to_hex(&cur_oid));
if (generation_zero == GENERATION_ZERO_EXISTS)
@ -1223,13 +1269,13 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
max_generation--;
if (graph_commit->generation != max_generation + 1)
graph_report("commit-graph generation for commit %s is %u != %u",
graph_report(_("commit-graph generation for commit %s is %u != %u"),
oid_to_hex(&cur_oid),
graph_commit->generation,
max_generation + 1);
if (graph_commit->date != odb_commit->date)
graph_report("commit date for commit %s in commit-graph is %"PRItime" != %"PRItime,
graph_report(_("commit date for commit %s in commit-graph is %"PRItime" != %"PRItime),
oid_to_hex(&cur_oid),
graph_commit->date,
odb_commit->date);

View file

@ -7,10 +7,12 @@
#include "cache.h"
#define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH"
#define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD"
struct commit;
char *get_commit_graph_filename(const char *obj_dir);
int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
/*
* Given a commit struct, try to fill the commit struct info, including:
@ -52,7 +54,7 @@ struct commit_graph {
const unsigned char *chunk_extra_edges;
};
struct commit_graph *load_commit_graph_one(const char *graph_file);
struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st);
struct commit_graph *parse_commit_graph(void *graph_map, int fd,
size_t graph_size);

View file

@ -89,6 +89,12 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)
{
return repo_parse_commit_gently(r, item, 0);
}
static inline int parse_commit_no_graph(struct commit *commit)
{
return repo_parse_commit_internal(the_repository, commit, 0, 0);
}
#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
#define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use)
#define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet)

View file

@ -366,6 +366,26 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \
GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))
GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
corrupt_graph_setup() {
cd "$TRASH_DIRECTORY/full" &&
test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
cp $objdir/info/commit-graph commit-graph-backup
}
corrupt_graph_verify() {
grepstr=$1
test_must_fail git commit-graph verify 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "$grepstr" err &&
if test "$2" != "no-copy"
then
cp $objdir/info/commit-graph commit-graph-pre-write-test
fi &&
git status --short &&
GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&
git commit-graph verify
}
# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
# Manipulates the commit-graph file at the position
# by inserting the data, optionally zeroing the file
@ -376,19 +396,28 @@ corrupt_graph_and_verify() {
pos=$1
data="${2:-\0}"
grepstr=$3
cd "$TRASH_DIRECTORY/full" &&
corrupt_graph_setup &&
orig_size=$(wc -c < $objdir/info/commit-graph) &&
zero_pos=${4:-${orig_size}} &&
test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
cp $objdir/info/commit-graph commit-graph-backup &&
printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" if=/dev/null &&
generate_zero_bytes $(($orig_size - $zero_pos)) >>"$objdir/info/commit-graph" &&
test_must_fail git commit-graph verify 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "$grepstr" err
corrupt_graph_verify "$grepstr"
}
test_expect_success POSIXPERM,SANITY 'detect permission problem' '
corrupt_graph_setup &&
chmod 000 $objdir/info/commit-graph &&
corrupt_graph_verify "Could not open" "no-copy"
'
test_expect_success 'detect too small' '
corrupt_graph_setup &&
echo "a small graph" >$objdir/info/commit-graph &&
corrupt_graph_verify "too small"
'
test_expect_success 'detect bad signature' '
corrupt_graph_and_verify 0 "\0" \
"graph signature"
@ -499,6 +528,7 @@ test_expect_success 'git fsck (checks commit-graph)' '
git fsck &&
corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
"incorrect checksum" &&
cp commit-graph-pre-write-test $objdir/info/commit-graph &&
test_must_fail git fsck
'