{"thread":{"id":"49952","subject":"[PATCH 0/2] Add commit-graph fuzzer and fix buffer overflow","startedAt":"2018-12-05T22:32:56Z","lastAt":"2019-02-20T16:50:55Z","messageCount":50,"participants":["Josh Steadmon","Ævar Arnfjörð Bjarmason","Junio C Hamano","Derrick Stolee","Jeff King","SZEDER Gábor","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"364652","messageId":"cover.1544048946.git.steadmon@google.com","threadId":"49952","inReplyTo":null,"subject":"[PATCH 0/2] Add commit-graph fuzzer and fix buffer overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-05T22:32:49Z","receivedAt":"2018-12-05T22:32:56Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Add a new fuzz test for the commit graph and fix a buffer read-overflow\nthat it discovered.\n\nJosh Steadmon (2):\n commit-graph, fuzz: Add fuzzer for commit-graph\n commit-graph: fix buffer read-overflow\n\n .gitignore | 1 +\n Makefile | 1 +\n commit-graph.c | 76 +++++++++++++++++++++++++++++++++------------\n fuzz-commit-graph.c | 18 +++++++++++\n 4 files changed, 77 insertions(+), 19 deletions(-)\n create mode 100644 fuzz-commit-graph.c\n\n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"364653","messageId":"53e62baaa8769bf8e90991e32e0d123cc6629559.1544048946.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544048946.git.steadmon@google.com","subject":"[PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-05T22:32:50Z","receivedAt":"2018-12-05T22:32:59Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Breaks load_commit_graph_one() into a new function,\nparse_commit_graph(). The latter function operates on arbitrary buffers,\nwhich makes it suitable as a fuzzing target.\n\nAdds fuzz-commit-graph.c, which provides a fuzzing entry point\ncompatible with libFuzzer (and possibly other fuzzing engines).\n\nSigned-off-by: Josh Steadmon \n---\n .gitignore | 1 +\n Makefile | 1 +\n commit-graph.c | 63 +++++++++++++++++++++++++++++++++------------\n fuzz-commit-graph.c | 18 +++++++++++++\n 4 files changed, 66 insertions(+), 17 deletions(-)\n create mode 100644 fuzz-commit-graph.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 0d77ea5894..8bcf153ed9 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -1,3 +1,4 @@\n+/fuzz-commit-graph\n /fuzz_corpora\n /fuzz-pack-headers\n /fuzz-pack-idx\ndiff --git a/Makefile b/Makefile\nindex 1a44c811aa..6b72f37c29 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \\\n \n ETAGS_TARGET = TAGS\n \n+FUZZ_OBJS += fuzz-commit-graph.o\n FUZZ_OBJS += fuzz-pack-headers.o\n FUZZ_OBJS += fuzz-pack-idx.o\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 40c855f185..0755359b1a 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -46,6 +46,10 @@\n #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \\\n \t\t\t+ GRAPH_FANOUT_SIZE + GRAPH_OID_LEN)\n \n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size);\n+\n+\n char *get_commit_graph_filename(const char *obj_dir)\n {\n \treturn xstrfmt(\"%s/info/commit-graph\", obj_dir);\n@@ -84,16 +88,10 @@ static int commit_graph_compatible(struct repository *r)\n struct commit_graph *load_commit_graph_one(const char *graph_file)\n {\n \tvoid *graph_map;\n-\tconst unsigned char *data, *chunk_lookup;\n \tsize_t graph_size;\n \tstruct stat st;\n-\tuint32_t i;\n-\tstruct commit_graph *graph;\n+\tstruct commit_graph *ret;\n \tint fd = git_open(graph_file);\n-\tuint64_t last_chunk_offset;\n-\tuint32_t last_chunk_id;\n-\tuint32_t graph_signature;\n-\tunsigned char graph_version, hash_version;\n \n \tif (fd < 0)\n \t\treturn NULL;\n@@ -108,27 +106,61 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t\tdie(_(\"graph file %s is too small\"), graph_file);\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tret = parse_commit_graph(graph_map, fd, graph_size);\n+\n+\tif (ret == NULL) {\n+\t\tmunmap(graph_map, graph_size);\n+\t\tclose(fd);\n+\t\texit(1);\n+\t}\n+\n+\treturn ret;\n+}\n+\n+/*\n+ * This function is intended to be used only from load_commit_graph_one() or in\n+ * fuzz tests.\n+ */\n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size)\n+{\n+\tconst unsigned char *data, *chunk_lookup;\n+\tuint32_t i;\n+\tstruct commit_graph *graph;\n+\tuint64_t last_chunk_offset;\n+\tuint32_t last_chunk_id;\n+\tuint32_t graph_signature;\n+\tunsigned char graph_version, hash_version;\n+\n+\t/*\n+\t * This should already be checked in load_commit_graph_one, but we still\n+\t * need a check here for when we're calling parse_commit_graph directly\n+\t * from fuzz tests. We can omit the error message in that case.\n+\t */\n+\tif (graph_size < GRAPH_MIN_SIZE)\n+\t\treturn NULL;\n+\n \tdata = (const unsigned char *)graph_map;\n \n \tgraph_signature = get_be32(data);\n \tif (graph_signature != GRAPH_SIGNATURE) {\n \t\terror(_(\"graph signature %X does not match signature %X\"),\n \t\t graph_signature, GRAPH_SIGNATURE);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \tgraph_version = *(unsigned char*)(data + 4);\n \tif (graph_version != GRAPH_VERSION) {\n \t\terror(_(\"graph version %X does not match version %X\"),\n \t\t graph_version, GRAPH_VERSION);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \thash_version = *(unsigned char*)(data + 5);\n \tif (hash_version != GRAPH_OID_VERSION) {\n \t\terror(_(\"hash version %X does not match version %X\"),\n \t\t hash_version, GRAPH_OID_VERSION);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \tgraph = alloc_commit_graph();\n@@ -152,7 +184,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\n \t\t\terror(_(\"improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n \t\t\t (uint32_t)chunk_offset);\n-\t\t\tgoto cleanup_fail;\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n \t\t}\n \n \t\tswitch (chunk_id) {\n@@ -187,7 +220,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \n \t\tif (chunk_repeated) {\n \t\t\terror(_(\"chunk id %08x appears multiple times\"), chunk_id);\n-\t\t\tgoto cleanup_fail;\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n \t\t}\n \n \t\tif (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)\n@@ -201,11 +235,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t}\n \n \treturn graph;\n-\n-cleanup_fail:\n-\tmunmap(graph_map, graph_size);\n-\tclose(fd);\n-\texit(1);\n }\n \n static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nnew file mode 100644\nindex 0000000000..420851d0d2\n--- /dev/null\n+++ b/fuzz-commit-graph.c\n@@ -0,0 +1,18 @@\n+#include \"object-store.h\"\n+#include \"commit-graph.h\"\n+\n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size);\n+\n+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n+\n+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n+{\n+\tstruct commit_graph *g;\n+\n+\tg = parse_commit_graph((void *) data, -1, size);\n+\tif (g)\n+\t\tfree(g);\n+\n+\treturn 0;\n+}\n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"364654","messageId":"ad2e761f4438ac80e947be0f6831fb6467eb4396.1544048946.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544048946.git.steadmon@google.com","subject":"[PATCH 2/2] commit-graph: fix buffer read-overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-05T22:32:51Z","receivedAt":"2018-12-05T22:33:01Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"fuzz-commit-graph identified a case where Git will read past the end of\na buffer containing a commit graph if the graph's header has an\nincorrect chunk count. A simple bounds check in parse_commit_graph()\nprevents this.\n\nSigned-off-by: Josh Steadmon \nHelped-by: Derrick Stolee \n---\n commit-graph.c | 13 +++++++++++--\n 1 file changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 0755359b1a..fee171a5f3 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -175,10 +175,19 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \tlast_chunk_offset = 8;\n \tchunk_lookup = data + 8;\n \tfor (i = 0; i < graph->num_chunks; i++) {\n-\t\tuint32_t chunk_id = get_be32(chunk_lookup + 0);\n-\t\tuint64_t chunk_offset = get_be64(chunk_lookup + 4);\n+\t\tuint32_t chunk_id;\n+\t\tuint64_t chunk_offset;\n \t\tint chunk_repeated = 0;\n \n+\t\tif (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH > data + graph_size) {\n+\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\tchunk_id = get_be32(chunk_lookup + 0);\n+\t\tchunk_offset = get_be64(chunk_lookup + 4);\n+\n \t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n \n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"364657","messageId":"874lbrzj2d.fsf@evledraar.gmail.com","threadId":"49952","inReplyTo":"53e62baaa8769bf8e90991e32e0d123cc6629559.1544048946.git.steadmon@google.com","subject":"Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-05T22:48:26Z","receivedAt":"2018-12-05T22:48:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 05 2018, Josh Steadmon wrote:\n\n> Breaks load_commit_graph_one() into a new function,\n> parse_commit_graph(). The latter function operates on arbitrary buffers,\n> which makes it suitable as a fuzzing target.\n>\n> Adds fuzz-commit-graph.c, which provides a fuzzing entry point\n> compatible with libFuzzer (and possibly other fuzzing engines).\n>\n> Signed-off-by: Josh Steadmon \n> ---\n> .gitignore | 1 +\n> Makefile | 1 +\n> commit-graph.c | 63 +++++++++++++++++++++++++++++++++------------\n> fuzz-commit-graph.c | 18 +++++++++++++\n> 4 files changed, 66 insertions(+), 17 deletions(-)\n> create mode 100644 fuzz-commit-graph.c\n>\n> diff --git a/.gitignore b/.gitignore\n> index 0d77ea5894..8bcf153ed9 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -1,3 +1,4 @@\n> +/fuzz-commit-graph\n> /fuzz_corpora\n> /fuzz-pack-headers\n> /fuzz-pack-idx\n> diff --git a/Makefile b/Makefile\n> index 1a44c811aa..6b72f37c29 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \\\n>\n> ETAGS_TARGET = TAGS\n>\n> +FUZZ_OBJS += fuzz-commit-graph.o\n> FUZZ_OBJS += fuzz-pack-headers.o\n> FUZZ_OBJS += fuzz-pack-idx.o\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 40c855f185..0755359b1a 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -46,6 +46,10 @@\n> #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \\\n> \t\t\t+ GRAPH_FANOUT_SIZE + GRAPH_OID_LEN)\n>\n> +struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> +\t\t\t\t\tsize_t graph_size);\n> +\n> +\n> char *get_commit_graph_filename(const char *obj_dir)\n> {\n> \treturn xstrfmt(\"%s/info/commit-graph\", obj_dir);\n> @@ -84,16 +88,10 @@ static int commit_graph_compatible(struct repository *r)\n> struct commit_graph *load_commit_graph_one(const char *graph_file)\n> {\n> \tvoid *graph_map;\n> -\tconst unsigned char *data, *chunk_lookup;\n> \tsize_t graph_size;\n> \tstruct stat st;\n> -\tuint32_t i;\n> -\tstruct commit_graph *graph;\n> +\tstruct commit_graph *ret;\n> \tint fd = git_open(graph_file);\n> -\tuint64_t last_chunk_offset;\n> -\tuint32_t last_chunk_id;\n> -\tuint32_t graph_signature;\n> -\tunsigned char graph_version, hash_version;\n>\n> \tif (fd < 0)\n> \t\treturn NULL;\n> @@ -108,27 +106,61 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n> \t\tdie(_(\"graph file %s is too small\"), graph_file);\n> \t}\n> \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\tret = parse_commit_graph(graph_map, fd, graph_size);\n> +\n> +\tif (ret == NULL) {\n\nCode in git usually uses just !ret.\n\n> +\t\tmunmap(graph_map, graph_size);\n> +\t\tclose(fd);\n> +\t\texit(1);\n\nOuch, exit(1) from load_commit_graph_one()? Can't we return NULL here\ninstead? Nasty to exit from a library routine, but then I see later...\n\n> @@ -201,11 +235,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n> \t}\n>\n> \treturn graph;\n> -\n> -cleanup_fail:\n> -\tmunmap(graph_map, graph_size);\n> -\tclose(fd);\n> -\texit(1);\n> }\n\n... ah, I see this is where you got the exit(1) from. So it was there\nalready.\n\n> static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n> diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\n> new file mode 100644\n> index 0000000000..420851d0d2\n> --- /dev/null\n> +++ b/fuzz-commit-graph.c\n> @@ -0,0 +1,18 @@\n> +#include \"object-store.h\"\n> +#include \"commit-graph.h\"\n> +\n> +struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> +\t\t\t\t\tsize_t graph_size);\n> +\n> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n> +\n> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n> +{\n> +\tstruct commit_graph *g;\n> +\n> +\tg = parse_commit_graph((void *) data, -1, size);\n> +\tif (g)\n> +\t\tfree(g);\n> +\n> +\treturn 0;\n> +}\n\nI hadn't looked at this before, but see your 5e47215080 (\"fuzz: add\nbasic fuzz testing target.\", 2018-10-12) for some prior art.\n\nThere's instructions there for a very long \"make\" invocation. Would be\nnice if this were friendlier and we could just do \"make test-fuzz\" or\nsomething...\n"},{"id":"364665","messageId":"20181206010018.GE9703@google.com","threadId":"49952","inReplyTo":"874lbrzj2d.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-06T01:00:18Z","receivedAt":"2018-12-06T01:00:28Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.12.05 23:48, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Dec 05 2018, Josh Steadmon wrote:\n> \n> > Breaks load_commit_graph_one() into a new function,\n> > parse_commit_graph(). The latter function operates on arbitrary buffers,\n> > which makes it suitable as a fuzzing target.\n> >\n> > Adds fuzz-commit-graph.c, which provides a fuzzing entry point\n> > compatible with libFuzzer (and possibly other fuzzing engines).\n> >\n> > Signed-off-by: Josh Steadmon \n> > ---\n> > .gitignore | 1 +\n> > Makefile | 1 +\n> > commit-graph.c | 63 +++++++++++++++++++++++++++++++++------------\n> > fuzz-commit-graph.c | 18 +++++++++++++\n> > 4 files changed, 66 insertions(+), 17 deletions(-)\n> > create mode 100644 fuzz-commit-graph.c\n> >\n> > diff --git a/.gitignore b/.gitignore\n> > index 0d77ea5894..8bcf153ed9 100644\n> > --- a/.gitignore\n> > +++ b/.gitignore\n> > @@ -1,3 +1,4 @@\n> > +/fuzz-commit-graph\n> > /fuzz_corpora\n> > /fuzz-pack-headers\n> > /fuzz-pack-idx\n> > diff --git a/Makefile b/Makefile\n> > index 1a44c811aa..6b72f37c29 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \\\n> >\n> > ETAGS_TARGET = TAGS\n> >\n> > +FUZZ_OBJS += fuzz-commit-graph.o\n> > FUZZ_OBJS += fuzz-pack-headers.o\n> > FUZZ_OBJS += fuzz-pack-idx.o\n> >\n> > diff --git a/commit-graph.c b/commit-graph.c\n> > index 40c855f185..0755359b1a 100644\n> > --- a/commit-graph.c\n> > +++ b/commit-graph.c\n> > @@ -46,6 +46,10 @@\n> > #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \\\n> > \t\t\t+ GRAPH_FANOUT_SIZE + GRAPH_OID_LEN)\n> >\n> > +struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> > +\t\t\t\t\tsize_t graph_size);\n> > +\n> > +\n> > char *get_commit_graph_filename(const char *obj_dir)\n> > {\n> > \treturn xstrfmt(\"%s/info/commit-graph\", obj_dir);\n> > @@ -84,16 +88,10 @@ static int commit_graph_compatible(struct repository *r)\n> > struct commit_graph *load_commit_graph_one(const char *graph_file)\n> > {\n> > \tvoid *graph_map;\n> > -\tconst unsigned char *data, *chunk_lookup;\n> > \tsize_t graph_size;\n> > \tstruct stat st;\n> > -\tuint32_t i;\n> > -\tstruct commit_graph *graph;\n> > +\tstruct commit_graph *ret;\n> > \tint fd = git_open(graph_file);\n> > -\tuint64_t last_chunk_offset;\n> > -\tuint32_t last_chunk_id;\n> > -\tuint32_t graph_signature;\n> > -\tunsigned char graph_version, hash_version;\n> >\n> > \tif (fd < 0)\n> > \t\treturn NULL;\n> > @@ -108,27 +106,61 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n> > \t\tdie(_(\"graph file %s is too small\"), graph_file);\n> > \t}\n> > \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n> > +\tret = parse_commit_graph(graph_map, fd, graph_size);\n> > +\n> > +\tif (ret == NULL) {\n> \n> Code in git usually uses just !ret.\n\nWill fix in V2, thanks.\n\n\n> > +\t\tmunmap(graph_map, graph_size);\n> > +\t\tclose(fd);\n> > +\t\texit(1);\n> \n> Ouch, exit(1) from load_commit_graph_one()? Can't we return NULL here\n> instead? Nasty to exit from a library routine, but then I see later...\n> \n> > @@ -201,11 +235,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n> > \t}\n> >\n> > \treturn graph;\n> > -\n> > -cleanup_fail:\n> > -\tmunmap(graph_map, graph_size);\n> > -\tclose(fd);\n> > -\texit(1);\n> > }\n> \n> ... ah, I see this is where you got the exit(1) from. So it was there\n> already.\n> \n> > static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n> > diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\n> > new file mode 100644\n> > index 0000000000..420851d0d2\n> > --- /dev/null\n> > +++ b/fuzz-commit-graph.c\n> > @@ -0,0 +1,18 @@\n> > +#include \"object-store.h\"\n> > +#include \"commit-graph.h\"\n> > +\n> > +struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> > +\t\t\t\t\tsize_t graph_size);\n> > +\n> > +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n> > +\n> > +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n> > +{\n> > +\tstruct commit_graph *g;\n> > +\n> > +\tg = parse_commit_graph((void *) data, -1, size);\n> > +\tif (g)\n> > +\t\tfree(g);\n> > +\n> > +\treturn 0;\n> > +}\n> \n> I hadn't looked at this before, but see your 5e47215080 (\"fuzz: add\n> basic fuzz testing target.\", 2018-10-12) for some prior art.\n> \n> There's instructions there for a very long \"make\" invocation. Would be\n> nice if this were friendlier and we could just do \"make test-fuzz\" or\n> something...\n\nYeah, the problem is that there are too many combinations of fuzzing\nengine, sanitizer, and compiler to make any reasonable default here.\nEven if you just stick with libFuzzer, address sanitizer, and clang, the\nflags change radically depending on which version of clang you're using.\n"},{"id":"364668","messageId":"xmqqmupja19b.fsf@gitster-ct.c.googlers.com","threadId":"49952","inReplyTo":"53e62baaa8769bf8e90991e32e0d123cc6629559.1544048946.git.steadmon@google.com","subject":"Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-06T01:32:16Z","receivedAt":"2018-12-06T01:32:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> @@ -108,27 +106,61 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n> \t\tdie(_(\"graph file %s is too small\"), graph_file);\n> \t}\n> \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\tret = parse_commit_graph(graph_map, fd, graph_size);\n\nOK, assuming that the new helper returns NULL from all places in the\noriginal that would have jumped to the cleanup-fail label, this would\ndo the same thing (it may not be the right thing to exit, but that\nis OK for the purpose of this change).\n\n> +\tif (ret == NULL) {\n> +\t\tmunmap(graph_map, graph_size);\n> +\t\tclose(fd);\n> +\t\texit(1);\n> +\t}\n> +\n> +\treturn ret;\n> +}\n> +\n> +/*\n> + * This function is intended to be used only from load_commit_graph_one() or in\n> + * fuzz tests.\n> + */\n\nHmph, is that necessary to say? \n\n\"Right now, it has only these two callers\" is sometimes handy for\nthose without good static analysis tools, like \"git grep\" ;-), but I\ndo not think of a reason why a new caller we'll add in the future,\nwho happens to have the data of the graph file in-core, should not\nbe allowed to call this function.\n\n\n> +struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> +\t\t\t\t\tsize_t graph_size)\n> +{\n> +\tconst unsigned char *data, *chunk_lookup;\n> +\tuint32_t i;\n> +\tstruct commit_graph *graph;\n> +\tuint64_t last_chunk_offset;\n> +\tuint32_t last_chunk_id;\n> +\tuint32_t graph_signature;\n> +\tunsigned char graph_version, hash_version;\n> +\n> +\t/*\n> +\t * This should already be checked in load_commit_graph_one, but we still\n> +\t * need a check here for when we're calling parse_commit_graph directly\n> +\t * from fuzz tests. We can omit the error message in that case.\n> +\t */\n\nIn the same spirit, I am dubious of the longer-term value of this\ncomment. As an explanation of why this conversion is correct in the\nproposed log message for this change, it perfectly makes sense,\nthough.\n\n> +\tif (graph_size < GRAPH_MIN_SIZE)\n> +\t\treturn NULL;\n> +\n\nThe load_commit_graph_one() grabbed graph_map out of xmmap() so it\nis guaranteed to be non-NULL, but we need to check graph_map != NULL\nwhen we're calling this directly from the fuzz tests, exactly in the\nsame spirit that we check graph_size above. Besides, these are to\nmake sure future callers won't misuse the API.\n\n> \tdata = (const unsigned char *)graph_map;\n\nAnd the reset of the function is the same as the original modulo\njumping to the cleanup_fail label has been replaced with returning\nNULL.\n\nLooks good.\n\n> ...\n> -\n> -cleanup_fail:\n> -\tmunmap(graph_map, graph_size);\n> -\tclose(fd);\n> -\texit(1);\n> }\n> \n> static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n> diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\n> new file mode 100644\n> index 0000000000..420851d0d2\n> --- /dev/null\n> +++ b/fuzz-commit-graph.c\n> @@ -0,0 +1,18 @@\n> +#include \"object-store.h\"\n> +#include \"commit-graph.h\"\n> +\n> +struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> +\t\t\t\t\tsize_t graph_size);\n> +\n> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n> +\n> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n> +{\n> +\tstruct commit_graph *g;\n> +\n> +\tg = parse_commit_graph((void *) data, -1, size);\n> +\tif (g)\n> +\t\tfree(g);\n> +\n> +\treturn 0;\n> +}\n"},{"id":"364669","messageId":"xmqqin07a0ul.fsf@gitster-ct.c.googlers.com","threadId":"49952","inReplyTo":"xmqqmupja19b.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-06T01:41:06Z","receivedAt":"2018-12-06T01:41:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +\tif (graph_size < GRAPH_MIN_SIZE)\n>> +\t\treturn NULL;\n>> +\n>\n> The load_commit_graph_one() grabbed graph_map out of xmmap() so it\n> is guaranteed to be non-NULL, but we need to check graph_map != NULL\n> when we're calling this directly from the fuzz tests, exactly in the\n> same spirit that we check graph_size above. Besides, these are to\n> make sure future callers won't misuse the API.\n\nInsert \"Please check graph_map != NULL here, too.\" before the above\nparagraph.\n\n>> \tdata = (const unsigned char *)graph_map;\n>\n> And the reset of the function is the same as the original modulo\n> jumping to the cleanup_fail label has been replaced with returning\n> NULL.\n>\n> Looks good.\n\nOf course, s/reset/rest/ is what I meant.\n"},{"id":"364670","messageId":"xmqqa7lj9s8e.fsf@gitster-ct.c.googlers.com","threadId":"49952","inReplyTo":"53e62baaa8769bf8e90991e32e0d123cc6629559.1544048946.git.steadmon@google.com","subject":"Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-06T04:47:13Z","receivedAt":"2018-12-06T04:47:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\n> new file mode 100644\n> index 0000000000..420851d0d2\n> --- /dev/null\n> +++ b/fuzz-commit-graph.c\n> @@ -0,0 +1,18 @@\n> +#include \"object-store.h\"\n> +#include \"commit-graph.h\"\n> +\n> +struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> +\t\t\t\t\tsize_t graph_size);\n> +\n> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n> +\n> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n> +{\n> +\tstruct commit_graph *g;\n> +\n> +\tg = parse_commit_graph((void *) data, -1, size);\n> +\tif (g)\n> +\t\tfree(g);\n\nAs it is perfectly OK to free(NULL), please lose \"if (g)\" and a\nlevel of indentation; otherwise, \"make coccicheck\" would complain.\n\nThanks.\n\n> +\treturn 0;\n> +}\n"},{"id":"364685","messageId":"077fed80-8d8c-1ed9-b2b6-18b86887482f@gmail.com","threadId":"49952","inReplyTo":"ad2e761f4438ac80e947be0f6831fb6467eb4396.1544048946.git.steadmon@google.com","subject":"Re: [PATCH 2/2] commit-graph: fix buffer read-overflow","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-12-06T13:11:28Z","receivedAt":"2018-12-06T13:11:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/5/2018 5:32 PM, Josh Steadmon wrote:\n> \n> +\t\tif (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH > data + graph_size) {\n> +\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n> +\t\t\tfree(graph);\n> +\t\t\treturn NULL;\n> +\t\t}\n\nSomething I forgot earlier: there are several tests in \nt5318-commit-graph.sh that use 'git commit-graph verify' to ensure we \nhit these error conditions on a corrupted commit-graph file. Could you \ntry adding a test there that looks for this error message?\n\nThanks,\n-Stolee\n"},{"id":"364703","messageId":"cover.1544127439.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544048946.git.steadmon@google.com","subject":"[PATCH v2 0/3] Add commit-graph fuzzer and fix buffer overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-06T20:20:52Z","receivedAt":"2018-12-06T20:21:00Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Add a new fuzz test for the commit graph and fix a buffer read-overflow\nthat it discovered. Additionally, fix the Makefile instructions for\nbuilding fuzzers.\n\nChanges since V1:\n * Moved the parse_commit_graph() declaration to the header file, since\n we don't mind if others use it.\n * Moved some unnecessary comments into commit messages.\n * Fixed some style issues.\n * Added a test case for detecting commit graphs with missing chunk\n lookup entries.\n * Ævar's comments on the Makefile made me realize the fuzzer build\n instructions were using the wrong variable. Added a new commit to\n fix this.\n\nJosh Steadmon (3):\n commit-graph, fuzz: Add fuzzer for commit-graph\n commit-graph: fix buffer read-overflow\n Makefile: correct example fuzz build\n\n .gitignore | 1 +\n Makefile | 3 +-\n commit-graph.c | 67 +++++++++++++++++++++++++++++------------\n commit-graph.h | 3 ++\n fuzz-commit-graph.c | 16 ++++++++++\n t/t5318-commit-graph.sh | 28 +++++++++++++++++\n 6 files changed, 98 insertions(+), 20 deletions(-)\n create mode 100644 fuzz-commit-graph.c\n\nRange-diff against v1:\n1: 53e62baaa8 ! 1: 0b57ecbe1b commit-graph, fuzz: Add fuzzer for commit-graph\n @@ -4,7 +4,9 @@\n \n Breaks load_commit_graph_one() into a new function,\n parse_commit_graph(). The latter function operates on arbitrary buffers,\n - which makes it suitable as a fuzzing target.\n + which makes it suitable as a fuzzing target. Since parse_commit_graph()\n + is only called by load_commit_graph_one() (and the fuzzer described\n + below), we omit error messages that would be duplicated by the caller.\n \n Adds fuzz-commit-graph.c, which provides a fuzzing entry point\n compatible with libFuzzer (and possibly other fuzzing engines).\n @@ -35,17 +37,6 @@\n diff --git a/commit-graph.c b/commit-graph.c\n --- a/commit-graph.c\n +++ b/commit-graph.c\n -@@\n - #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \\\n - \t\t\t+ GRAPH_FANOUT_SIZE + GRAPH_OID_LEN)\n - \n -+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n -+\t\t\t\t\tsize_t graph_size);\n -+\n -+\n - char *get_commit_graph_filename(const char *obj_dir)\n - {\n - \treturn xstrfmt(\"%s/info/commit-graph\", obj_dir);\n @@\n struct commit_graph *load_commit_graph_one(const char *graph_file)\n {\n @@ -70,7 +61,7 @@\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n +\tret = parse_commit_graph(graph_map, fd, graph_size);\n +\n -+\tif (ret == NULL) {\n ++\tif (!ret) {\n +\t\tmunmap(graph_map, graph_size);\n +\t\tclose(fd);\n +\t\texit(1);\n @@ -79,10 +70,6 @@\n +\treturn ret;\n +}\n +\n -+/*\n -+ * This function is intended to be used only from load_commit_graph_one() or in\n -+ * fuzz tests.\n -+ */\n +struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n +\t\t\t\t\tsize_t graph_size)\n +{\n @@ -94,11 +81,9 @@\n +\tuint32_t graph_signature;\n +\tunsigned char graph_version, hash_version;\n +\n -+\t/*\n -+\t * This should already be checked in load_commit_graph_one, but we still\n -+\t * need a check here for when we're calling parse_commit_graph directly\n -+\t * from fuzz tests. We can omit the error message in that case.\n -+\t */\n ++\tif (!graph_map)\n ++\t\treturn NULL;\n ++\n +\tif (graph_size < GRAPH_MIN_SIZE)\n +\t\treturn NULL;\n +\n @@ -162,12 +147,25 @@\n \n static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n \n + diff --git a/commit-graph.h b/commit-graph.h\n + --- a/commit-graph.h\n + +++ b/commit-graph.h\n +@@\n + \n + struct commit_graph *load_commit_graph_one(const char *graph_file);\n + \n ++struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n ++\t\t\t\t\tsize_t graph_size);\n ++\n + /*\n + * Return 1 if and only if the repository has a commit-graph\n + * file and generation numbers are computed in that file.\n +\n diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\n new file mode 100644\n --- /dev/null\n +++ b/fuzz-commit-graph.c\n @@\n -+#include \"object-store.h\"\n +#include \"commit-graph.h\"\n +\n +struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n @@ -179,9 +177,8 @@\n +{\n +\tstruct commit_graph *g;\n +\n -+\tg = parse_commit_graph((void *) data, -1, size);\n -+\tif (g)\n -+\t\tfree(g);\n ++\tg = parse_commit_graph((void *)data, -1, size);\n ++\tfree(g);\n +\n +\treturn 0;\n +}\n2: ad2e761f44 ! 2: af45c2337f commit-graph: fix buffer read-overflow\n @@ -22,7 +22,8 @@\n +\t\tuint64_t chunk_offset;\n \t\tint chunk_repeated = 0;\n \n -+\t\tif (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH > data + graph_size) {\n ++\t\tif (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH >\n ++\t\t data + graph_size) {\n +\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n +\t\t\tfree(graph);\n +\t\t\treturn NULL;\n @@ -34,3 +35,49 @@\n \t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n \n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\n +\n + diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n + --- a/t/t5318-commit-graph.sh\n + +++ b/t/t5318-commit-graph.sh\n +@@\n + \ttest_i18ngrep \"$grepstr\" err\n + }\n + \n ++\n ++# usage: corrupt_and_zero_graph_then_verify \n ++# Manipulates the commit-graph file at by inserting the data,\n ++# then zeros the file starting at . Finally, runs\n ++# 'git commit-graph verify' and places the output in the file 'err'. Tests 'err'\n ++# for the given string.\n ++corrupt_and_zero_graph_then_verify() {\n ++\tcorrupt_pos=$1\n ++\tdata=\"${2:-\\0}\"\n ++\tzero_pos=$3\n ++\tgrepstr=$4\n ++\torig_size=$(stat --format=%s $objdir/info/commit-graph)\n ++\tcd \"$TRASH_DIRECTORY/full\" &&\n ++\ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n ++\tcp $objdir/info/commit-graph commit-graph-backup &&\n ++\tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$corrupt_pos\" conv=notrunc &&\n ++\ttruncate --size=$zero_pos $objdir/info/commit-graph &&\n ++\ttruncate --size=$orig_size $objdir/info/commit-graph &&\n ++\ttest_must_fail git commit-graph verify 2>test_err &&\n ++\tgrep -v \"^+\" test_err >err &&\n ++\ttest_i18ngrep \"$grepstr\" err\n ++}\n ++\n + test_expect_success 'detect bad signature' '\n + \tcorrupt_graph_and_verify 0 \"\\0\" \\\n + \t\t\"graph signature\"\n +@@\n + \t\t\"incorrect checksum\"\n + '\n + \n ++test_expect_success 'detect truncated graph' '\n ++\tcorrupt_and_zero_graph_then_verify $GRAPH_BYTE_CHUNK_COUNT \"\\xff\" \\\n ++\t\t$GRAPH_CHUNK_LOOKUP_OFFSET \"chunk lookup table entry missing\"\n ++'\n ++\n + test_expect_success 'git fsck (checks commit-graph)' '\n + \tcd \"$TRASH_DIRECTORY/full\" &&\n + \tgit fsck &&\n-: ---------- > 3: 7519fc76df Makefile: correct example fuzz build\n-- \n2.20.0.rc2.10.g7519fc76df\n\n"},{"id":"364704","messageId":"0b57ecbe1b6c72d95311ba1bb30ea1ed9fd7e3e5.1544127439.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544127439.git.steadmon@google.com","subject":"[PATCH v2 1/3] commit-graph, fuzz: Add fuzzer for commit-graph","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-06T20:20:53Z","receivedAt":"2018-12-06T20:21:03Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Breaks load_commit_graph_one() into a new function,\nparse_commit_graph(). The latter function operates on arbitrary buffers,\nwhich makes it suitable as a fuzzing target. Since parse_commit_graph()\nis only called by load_commit_graph_one() (and the fuzzer described\nbelow), we omit error messages that would be duplicated by the caller.\n\nAdds fuzz-commit-graph.c, which provides a fuzzing entry point\ncompatible with libFuzzer (and possibly other fuzzing engines).\n\nSigned-off-by: Josh Steadmon \n---\n .gitignore | 1 +\n Makefile | 1 +\n commit-graph.c | 53 ++++++++++++++++++++++++++++++---------------\n commit-graph.h | 3 +++\n fuzz-commit-graph.c | 16 ++++++++++++++\n 5 files changed, 57 insertions(+), 17 deletions(-)\n create mode 100644 fuzz-commit-graph.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 0d77ea5894..8bcf153ed9 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -1,3 +1,4 @@\n+/fuzz-commit-graph\n /fuzz_corpora\n /fuzz-pack-headers\n /fuzz-pack-idx\ndiff --git a/Makefile b/Makefile\nindex 1a44c811aa..6b72f37c29 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \\\n \n ETAGS_TARGET = TAGS\n \n+FUZZ_OBJS += fuzz-commit-graph.o\n FUZZ_OBJS += fuzz-pack-headers.o\n FUZZ_OBJS += fuzz-pack-idx.o\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 40c855f185..07dd410f3c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -84,16 +84,10 @@ static int commit_graph_compatible(struct repository *r)\n struct commit_graph *load_commit_graph_one(const char *graph_file)\n {\n \tvoid *graph_map;\n-\tconst unsigned char *data, *chunk_lookup;\n \tsize_t graph_size;\n \tstruct stat st;\n-\tuint32_t i;\n-\tstruct commit_graph *graph;\n+\tstruct commit_graph *ret;\n \tint fd = git_open(graph_file);\n-\tuint64_t last_chunk_offset;\n-\tuint32_t last_chunk_id;\n-\tuint32_t graph_signature;\n-\tunsigned char graph_version, hash_version;\n \n \tif (fd < 0)\n \t\treturn NULL;\n@@ -108,27 +102,55 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t\tdie(_(\"graph file %s is too small\"), graph_file);\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tret = parse_commit_graph(graph_map, fd, graph_size);\n+\n+\tif (!ret) {\n+\t\tmunmap(graph_map, graph_size);\n+\t\tclose(fd);\n+\t\texit(1);\n+\t}\n+\n+\treturn ret;\n+}\n+\n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size)\n+{\n+\tconst unsigned char *data, *chunk_lookup;\n+\tuint32_t i;\n+\tstruct commit_graph *graph;\n+\tuint64_t last_chunk_offset;\n+\tuint32_t last_chunk_id;\n+\tuint32_t graph_signature;\n+\tunsigned char graph_version, hash_version;\n+\n+\tif (!graph_map)\n+\t\treturn NULL;\n+\n+\tif (graph_size < GRAPH_MIN_SIZE)\n+\t\treturn NULL;\n+\n \tdata = (const unsigned char *)graph_map;\n \n \tgraph_signature = get_be32(data);\n \tif (graph_signature != GRAPH_SIGNATURE) {\n \t\terror(_(\"graph signature %X does not match signature %X\"),\n \t\t graph_signature, GRAPH_SIGNATURE);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \tgraph_version = *(unsigned char*)(data + 4);\n \tif (graph_version != GRAPH_VERSION) {\n \t\terror(_(\"graph version %X does not match version %X\"),\n \t\t graph_version, GRAPH_VERSION);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \thash_version = *(unsigned char*)(data + 5);\n \tif (hash_version != GRAPH_OID_VERSION) {\n \t\terror(_(\"hash version %X does not match version %X\"),\n \t\t hash_version, GRAPH_OID_VERSION);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \tgraph = alloc_commit_graph();\n@@ -152,7 +174,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\n \t\t\terror(_(\"improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n \t\t\t (uint32_t)chunk_offset);\n-\t\t\tgoto cleanup_fail;\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n \t\t}\n \n \t\tswitch (chunk_id) {\n@@ -187,7 +210,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \n \t\tif (chunk_repeated) {\n \t\t\terror(_(\"chunk id %08x appears multiple times\"), chunk_id);\n-\t\t\tgoto cleanup_fail;\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n \t\t}\n \n \t\tif (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)\n@@ -201,11 +225,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t}\n \n \treturn graph;\n-\n-cleanup_fail:\n-\tmunmap(graph_map, graph_size);\n-\tclose(fd);\n-\texit(1);\n }\n \n static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 9db40b4d3a..813e7c19f1 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -54,6 +54,9 @@ struct commit_graph {\n \n struct commit_graph *load_commit_graph_one(const char *graph_file);\n \n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size);\n+\n /*\n * Return 1 if and only if the repository has a commit-graph\n * file and generation numbers are computed in that file.\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nnew file mode 100644\nindex 0000000000..cf790c9d04\n--- /dev/null\n+++ b/fuzz-commit-graph.c\n@@ -0,0 +1,16 @@\n+#include \"commit-graph.h\"\n+\n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size);\n+\n+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n+\n+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n+{\n+\tstruct commit_graph *g;\n+\n+\tg = parse_commit_graph((void *)data, -1, size);\n+\tfree(g);\n+\n+\treturn 0;\n+}\n-- \n2.20.0.rc2.10.g7519fc76df\n\n"},{"id":"364705","messageId":"af45c2337fbe2a59ac95aff3ce90a69d8c30416f.1544127439.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544127439.git.steadmon@google.com","subject":"[PATCH v2 2/3] commit-graph: fix buffer read-overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-06T20:20:54Z","receivedAt":"2018-12-06T20:21:06Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"fuzz-commit-graph identified a case where Git will read past the end of\na buffer containing a commit graph if the graph's header has an\nincorrect chunk count. A simple bounds check in parse_commit_graph()\nprevents this.\n\nSigned-off-by: Josh Steadmon \n---\n commit-graph.c | 14 ++++++++++++--\n t/t5318-commit-graph.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 40 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 07dd410f3c..224a5f161e 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \tlast_chunk_offset = 8;\n \tchunk_lookup = data + 8;\n \tfor (i = 0; i < graph->num_chunks; i++) {\n-\t\tuint32_t chunk_id = get_be32(chunk_lookup + 0);\n-\t\tuint64_t chunk_offset = get_be64(chunk_lookup + 4);\n+\t\tuint32_t chunk_id;\n+\t\tuint64_t chunk_offset;\n \t\tint chunk_repeated = 0;\n \n+\t\tif (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH >\n+\t\t data + graph_size) {\n+\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\tchunk_id = get_be32(chunk_lookup + 0);\n+\t\tchunk_offset = get_be64(chunk_lookup + 4);\n+\n \t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n \n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 5fe21db99f..2503cb0345 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -384,6 +384,29 @@ corrupt_graph_and_verify() {\n \ttest_i18ngrep \"$grepstr\" err\n }\n \n+\n+# usage: corrupt_and_zero_graph_then_verify \n+# Manipulates the commit-graph file at by inserting the data,\n+# then zeros the file starting at . Finally, runs\n+# 'git commit-graph verify' and places the output in the file 'err'. Tests 'err'\n+# for the given string.\n+corrupt_and_zero_graph_then_verify() {\n+\tcorrupt_pos=$1\n+\tdata=\"${2:-\\0}\"\n+\tzero_pos=$3\n+\tgrepstr=$4\n+\torig_size=$(stat --format=%s $objdir/info/commit-graph)\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n+\tcp $objdir/info/commit-graph commit-graph-backup &&\n+\tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$corrupt_pos\" conv=notrunc &&\n+\ttruncate --size=$zero_pos $objdir/info/commit-graph &&\n+\ttruncate --size=$orig_size $objdir/info/commit-graph &&\n+\ttest_must_fail git commit-graph verify 2>test_err &&\n+\tgrep -v \"^+\" test_err >err &&\n+\ttest_i18ngrep \"$grepstr\" err\n+}\n+\n test_expect_success 'detect bad signature' '\n \tcorrupt_graph_and_verify 0 \"\\0\" \\\n \t\t\"graph signature\"\n@@ -484,6 +507,11 @@ test_expect_success 'detect invalid checksum hash' '\n \t\t\"incorrect checksum\"\n '\n \n+test_expect_success 'detect truncated graph' '\n+\tcorrupt_and_zero_graph_then_verify $GRAPH_BYTE_CHUNK_COUNT \"\\xff\" \\\n+\t\t$GRAPH_CHUNK_LOOKUP_OFFSET \"chunk lookup table entry missing\"\n+'\n+\n test_expect_success 'git fsck (checks commit-graph)' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit fsck &&\n-- \n2.20.0.rc2.10.g7519fc76df\n\n"},{"id":"364706","messageId":"7519fc76df085ce8e34cbedde2814bf8ac12d7b5.1544127439.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544127439.git.steadmon@google.com","subject":"[PATCH v2 3/3] Makefile: correct example fuzz build","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-06T20:20:55Z","receivedAt":"2018-12-06T20:21:08Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Signed-off-by: Josh Steadmon \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 6b72f37c29..bbcfc2bc9f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3104,7 +3104,7 @@ cover_db_html: cover_db\n # An example command to build against libFuzzer from LLVM 4.0.0:\n #\n # make CC=clang CXX=clang++ \\\n-# FUZZ_CXXFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n+# CFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \\\n # fuzz-all\n #\n-- \n2.20.0.rc2.10.g7519fc76df\n\n"},{"id":"364739","messageId":"20181207090755.GB5167@sigill.intra.peff.net","threadId":"49952","inReplyTo":"af45c2337fbe2a59ac95aff3ce90a69d8c30416f.1544127439.git.steadmon@google.com","subject":"Re: [PATCH v2 2/3] commit-graph: fix buffer read-overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-07T09:07:55Z","receivedAt":"2018-12-07T09:07:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 06, 2018 at 12:20:54PM -0800, Josh Steadmon wrote:\n\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 07dd410f3c..224a5f161e 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> \tlast_chunk_offset = 8;\n> \tchunk_lookup = data + 8;\n> \tfor (i = 0; i < graph->num_chunks; i++) {\n> -\t\tuint32_t chunk_id = get_be32(chunk_lookup + 0);\n> -\t\tuint64_t chunk_offset = get_be64(chunk_lookup + 4);\n> +\t\tuint32_t chunk_id;\n> +\t\tuint64_t chunk_offset;\n> \t\tint chunk_repeated = 0;\n> \n> +\t\tif (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH >\n> +\t\t data + graph_size) {\n> +\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n> +\t\t\tfree(graph);\n> +\t\t\treturn NULL;\n> +\t\t}\n\nIs it possible to overflow the addition here? E.g., if I'm on a 32-bit\nsystem and the truncated chunk appears right at the 4GB limit, in which\ncase we wrap back around? I guess that's pretty implausible, since it\nwould mean that the mmap is bumping up against the end of the address\nspace. I didn't check, but I wouldn't be surprised if sane operating\nsystems avoid allocating those addresses.\n\nBut I think you could write this as:\n\n if (data + graph_size - chunk_lookup < GRAPH_CHUNKLOOKUP_WIDTH)\n\nto avoid overflow (we know that \"data + graph_size\" is sane because\nthat's our mmap, and chunk_lookup is somewhere between \"data\" and \"data\n+ graph_size\", so the result is between 0 and graph_size).\n\nI dunno. I think I've convinced myself it's a non-issue here, but it may\nbe good to get in the habit of writing these sorts of offset checks in\nan overflow-proof order.\n\n-Peff\n"},{"id":"364743","messageId":"7d3693c7-3926-b09d-fa17-faf5aa1dcde9@gmail.com","threadId":"49952","inReplyTo":"af45c2337fbe2a59ac95aff3ce90a69d8c30416f.1544127439.git.steadmon@google.com","subject":"Re: [PATCH v2 2/3] commit-graph: fix buffer read-overflow","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-12-07T13:33:34Z","receivedAt":"2018-12-07T13:33:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/6/2018 3:20 PM, Josh Steadmon wrote:\n> +\n> +# usage: corrupt_and_zero_graph_then_verify \n> +# Manipulates the commit-graph file at by inserting the data,\n> +# then zeros the file starting at . Finally, runs\n> +# 'git commit-graph verify' and places the output in the file 'err'. Tests 'err'\n> +# for the given string.\n> +corrupt_and_zero_graph_then_verify() {\n\nThis method is very similar to to 'corrupt_graph_and_verify()', the only \ndifference being the zero_pos, which zeroes the graph.\n\nCould it instead be a modification of corrupt_graph_and_verify() where \n$4 is interpreted as zero_pos, and if it is blank we don't do the \ntruncation?\n\n> +test_expect_success 'detect truncated graph' '\n> +\tcorrupt_and_zero_graph_then_verify $GRAPH_BYTE_CHUNK_COUNT \"\\xff\" \\\n> +\t\t$GRAPH_CHUNK_LOOKUP_OFFSET \"chunk lookup table entry missing\"\n> +'\n> +\n\nThanks for this! I think it's valuable to keep explicit tests around \nthat were discovered from your fuzz tests. Specifically, I can repeat \nthe test when I get around to the next file format.\n\nThanks,\n-Stolee\n"},{"id":"364765","messageId":"cover.1544221121.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544127439.git.steadmon@google.com","subject":"[PATCH v3 0/3] Add commit-graph fuzzer and fix buffer overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-07T22:27:21Z","receivedAt":"2018-12-07T22:27:30Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Ad a new fuzz test for the commit graph and fix a buffer read-overflow\nthat it discovered. Additionally, fix the Makefile instructions for\nbuilding fuzzers.\n\nChanges since V2:\n * Avoid pointer arithmetic overflow when checking the graph's chunk\n count.\n * Merge the corrupt_graph_and_verify and\n corrupt_and_zero_graph_then_verify test functions.\n\nJosh Steadmon (3):\n commit-graph, fuzz: Add fuzzer for commit-graph\n commit-graph: fix buffer read-overflow\n Makefile: correct example fuzz build\n\n .gitignore | 1 +\n Makefile | 3 +-\n commit-graph.c | 67 +++++++++++++++++++++++++++++------------\n commit-graph.h | 3 ++\n fuzz-commit-graph.c | 16 ++++++++++\n t/t5318-commit-graph.sh | 15 +++++++--\n 6 files changed, 83 insertions(+), 22 deletions(-)\n create mode 100644 fuzz-commit-graph.c\n\nRange-diff against v2:\n1: af45c2337f ! 1: 675d58ecea commit-graph: fix buffer read-overflow\n @@ -22,8 +22,8 @@\n +\t\tuint64_t chunk_offset;\n \t\tint chunk_repeated = 0;\n \n -+\t\tif (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH >\n -+\t\t data + graph_size) {\n ++\t\tif (data + graph_size - chunk_lookup <\n ++\t\t GRAPH_CHUNKLOOKUP_WIDTH) {\n +\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n +\t\t\tfree(graph);\n +\t\t\treturn NULL;\n @@ -40,31 +40,34 @@\n --- a/t/t5318-commit-graph.sh\n +++ b/t/t5318-commit-graph.sh\n @@\n - \ttest_i18ngrep \"$grepstr\" err\n - }\n + GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n + GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n \n -+\n -+# usage: corrupt_and_zero_graph_then_verify \n -+# Manipulates the commit-graph file at by inserting the data,\n -+# then zeros the file starting at . Finally, runs\n -+# 'git commit-graph verify' and places the output in the file 'err'. Tests 'err'\n -+# for the given string.\n -+corrupt_and_zero_graph_then_verify() {\n -+\tcorrupt_pos=$1\n -+\tdata=\"${2:-\\0}\"\n -+\tzero_pos=$3\n -+\tgrepstr=$4\n +-# usage: corrupt_graph_and_verify \n ++# usage: corrupt_graph_and_verify []\n + # Manipulates the commit-graph file at the position\n +-# by inserting the data, then runs 'git commit-graph verify'\n ++# by inserting the data, optionally zeroing the file\n ++# starting at , then runs 'git commit-graph verify'\n + # and places the output in the file 'err'. Test 'err' for\n + # the given string.\n + corrupt_graph_and_verify() {\n + \tpos=$1\n + \tdata=\"${2:-\\0}\"\n + \tgrepstr=$3\n +\torig_size=$(stat --format=%s $objdir/info/commit-graph)\n -+\tcd \"$TRASH_DIRECTORY/full\" &&\n -+\ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n -+\tcp $objdir/info/commit-graph commit-graph-backup &&\n -+\tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$corrupt_pos\" conv=notrunc &&\n ++\tzero_pos=${4:-${orig_size}}\n + \tcd \"$TRASH_DIRECTORY/full\" &&\n + \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n + \tcp $objdir/info/commit-graph commit-graph-backup &&\n + \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n +\ttruncate --size=$zero_pos $objdir/info/commit-graph &&\n +\ttruncate --size=$orig_size $objdir/info/commit-graph &&\n -+\ttest_must_fail git commit-graph verify 2>test_err &&\n -+\tgrep -v \"^+\" test_err >err &&\n -+\ttest_i18ngrep \"$grepstr\" err\n -+}\n + \ttest_must_fail git commit-graph verify 2>test_err &&\n + \tgrep -v \"^+\" test_err >err\n + \ttest_i18ngrep \"$grepstr\" err\n + }\n + \n +\n test_expect_success 'detect bad signature' '\n \tcorrupt_graph_and_verify 0 \"\\0\" \\\n @@ -73,9 +76,9 @@\n \t\t\"incorrect checksum\"\n '\n \n -+test_expect_success 'detect truncated graph' '\n -+\tcorrupt_and_zero_graph_then_verify $GRAPH_BYTE_CHUNK_COUNT \"\\xff\" \\\n -+\t\t$GRAPH_CHUNK_LOOKUP_OFFSET \"chunk lookup table entry missing\"\n ++test_expect_success 'detect incorrect chunk count' '\n ++\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\xff\" \\\n ++\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n +'\n +\n test_expect_success 'git fsck (checks commit-graph)' '\n2: 7519fc76df = 2: 06a32bfe8b Makefile: correct example fuzz build\n-- \n2.20.0.rc2.12.g4c11c11dec\n\n"},{"id":"364766","messageId":"0b57ecbe1b6c72d95311ba1bb30ea1ed9fd7e3e5.1544221121.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544221121.git.steadmon@google.com","subject":"[PATCH v3 1/3] commit-graph, fuzz: Add fuzzer for commit-graph","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-07T22:27:22Z","receivedAt":"2018-12-07T22:27:32Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Breaks load_commit_graph_one() into a new function,\nparse_commit_graph(). The latter function operates on arbitrary buffers,\nwhich makes it suitable as a fuzzing target. Since parse_commit_graph()\nis only called by load_commit_graph_one() (and the fuzzer described\nbelow), we omit error messages that would be duplicated by the caller.\n\nAdds fuzz-commit-graph.c, which provides a fuzzing entry point\ncompatible with libFuzzer (and possibly other fuzzing engines).\n\nSigned-off-by: Josh Steadmon \n---\n .gitignore | 1 +\n Makefile | 1 +\n commit-graph.c | 53 ++++++++++++++++++++++++++++++---------------\n commit-graph.h | 3 +++\n fuzz-commit-graph.c | 16 ++++++++++++++\n 5 files changed, 57 insertions(+), 17 deletions(-)\n create mode 100644 fuzz-commit-graph.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 0d77ea5894..8bcf153ed9 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -1,3 +1,4 @@\n+/fuzz-commit-graph\n /fuzz_corpora\n /fuzz-pack-headers\n /fuzz-pack-idx\ndiff --git a/Makefile b/Makefile\nindex 1a44c811aa..6b72f37c29 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \\\n \n ETAGS_TARGET = TAGS\n \n+FUZZ_OBJS += fuzz-commit-graph.o\n FUZZ_OBJS += fuzz-pack-headers.o\n FUZZ_OBJS += fuzz-pack-idx.o\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 40c855f185..07dd410f3c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -84,16 +84,10 @@ static int commit_graph_compatible(struct repository *r)\n struct commit_graph *load_commit_graph_one(const char *graph_file)\n {\n \tvoid *graph_map;\n-\tconst unsigned char *data, *chunk_lookup;\n \tsize_t graph_size;\n \tstruct stat st;\n-\tuint32_t i;\n-\tstruct commit_graph *graph;\n+\tstruct commit_graph *ret;\n \tint fd = git_open(graph_file);\n-\tuint64_t last_chunk_offset;\n-\tuint32_t last_chunk_id;\n-\tuint32_t graph_signature;\n-\tunsigned char graph_version, hash_version;\n \n \tif (fd < 0)\n \t\treturn NULL;\n@@ -108,27 +102,55 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t\tdie(_(\"graph file %s is too small\"), graph_file);\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tret = parse_commit_graph(graph_map, fd, graph_size);\n+\n+\tif (!ret) {\n+\t\tmunmap(graph_map, graph_size);\n+\t\tclose(fd);\n+\t\texit(1);\n+\t}\n+\n+\treturn ret;\n+}\n+\n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size)\n+{\n+\tconst unsigned char *data, *chunk_lookup;\n+\tuint32_t i;\n+\tstruct commit_graph *graph;\n+\tuint64_t last_chunk_offset;\n+\tuint32_t last_chunk_id;\n+\tuint32_t graph_signature;\n+\tunsigned char graph_version, hash_version;\n+\n+\tif (!graph_map)\n+\t\treturn NULL;\n+\n+\tif (graph_size < GRAPH_MIN_SIZE)\n+\t\treturn NULL;\n+\n \tdata = (const unsigned char *)graph_map;\n \n \tgraph_signature = get_be32(data);\n \tif (graph_signature != GRAPH_SIGNATURE) {\n \t\terror(_(\"graph signature %X does not match signature %X\"),\n \t\t graph_signature, GRAPH_SIGNATURE);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \tgraph_version = *(unsigned char*)(data + 4);\n \tif (graph_version != GRAPH_VERSION) {\n \t\terror(_(\"graph version %X does not match version %X\"),\n \t\t graph_version, GRAPH_VERSION);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \thash_version = *(unsigned char*)(data + 5);\n \tif (hash_version != GRAPH_OID_VERSION) {\n \t\terror(_(\"hash version %X does not match version %X\"),\n \t\t hash_version, GRAPH_OID_VERSION);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \tgraph = alloc_commit_graph();\n@@ -152,7 +174,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\n \t\t\terror(_(\"improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n \t\t\t (uint32_t)chunk_offset);\n-\t\t\tgoto cleanup_fail;\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n \t\t}\n \n \t\tswitch (chunk_id) {\n@@ -187,7 +210,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \n \t\tif (chunk_repeated) {\n \t\t\terror(_(\"chunk id %08x appears multiple times\"), chunk_id);\n-\t\t\tgoto cleanup_fail;\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n \t\t}\n \n \t\tif (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)\n@@ -201,11 +225,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t}\n \n \treturn graph;\n-\n-cleanup_fail:\n-\tmunmap(graph_map, graph_size);\n-\tclose(fd);\n-\texit(1);\n }\n \n static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 9db40b4d3a..813e7c19f1 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -54,6 +54,9 @@ struct commit_graph {\n \n struct commit_graph *load_commit_graph_one(const char *graph_file);\n \n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size);\n+\n /*\n * Return 1 if and only if the repository has a commit-graph\n * file and generation numbers are computed in that file.\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nnew file mode 100644\nindex 0000000000..cf790c9d04\n--- /dev/null\n+++ b/fuzz-commit-graph.c\n@@ -0,0 +1,16 @@\n+#include \"commit-graph.h\"\n+\n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size);\n+\n+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n+\n+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n+{\n+\tstruct commit_graph *g;\n+\n+\tg = parse_commit_graph((void *)data, -1, size);\n+\tfree(g);\n+\n+\treturn 0;\n+}\n-- \n2.20.0.rc2.12.g4c11c11dec\n\n"},{"id":"364767","messageId":"675d58ecea2f315bd05d2a21f6a473e9de1105a0.1544221121.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544221121.git.steadmon@google.com","subject":"[PATCH v3 2/3] commit-graph: fix buffer read-overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-07T22:27:23Z","receivedAt":"2018-12-07T22:27:35Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"fuzz-commit-graph identified a case where Git will read past the end of\na buffer containing a commit graph if the graph's header has an\nincorrect chunk count. A simple bounds check in parse_commit_graph()\nprevents this.\n\nSigned-off-by: Josh Steadmon \n---\n commit-graph.c | 14 ++++++++++++--\n t/t5318-commit-graph.sh | 15 +++++++++++++--\n 2 files changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 07dd410f3c..836d65a1d3 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \tlast_chunk_offset = 8;\n \tchunk_lookup = data + 8;\n \tfor (i = 0; i < graph->num_chunks; i++) {\n-\t\tuint32_t chunk_id = get_be32(chunk_lookup + 0);\n-\t\tuint64_t chunk_offset = get_be64(chunk_lookup + 4);\n+\t\tuint32_t chunk_id;\n+\t\tuint64_t chunk_offset;\n \t\tint chunk_repeated = 0;\n \n+\t\tif (data + graph_size - chunk_lookup <\n+\t\t GRAPH_CHUNKLOOKUP_WIDTH) {\n+\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\tchunk_id = get_be32(chunk_lookup + 0);\n+\t\tchunk_offset = get_be64(chunk_lookup + 4);\n+\n \t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n \n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 5fe21db99f..5b6b44b78e 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n \n-# usage: corrupt_graph_and_verify \n+# usage: corrupt_graph_and_verify []\n # Manipulates the commit-graph file at the position\n-# by inserting the data, then runs 'git commit-graph verify'\n+# by inserting the data, optionally zeroing the file\n+# starting at , then runs 'git commit-graph verify'\n # and places the output in the file 'err'. Test 'err' for\n # the given string.\n corrupt_graph_and_verify() {\n \tpos=$1\n \tdata=\"${2:-\\0}\"\n \tgrepstr=$3\n+\torig_size=$(stat --format=%s $objdir/info/commit-graph)\n+\tzero_pos=${4:-${orig_size}}\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n \tcp $objdir/info/commit-graph commit-graph-backup &&\n \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n+\ttruncate --size=$zero_pos $objdir/info/commit-graph &&\n+\ttruncate --size=$orig_size $objdir/info/commit-graph &&\n \ttest_must_fail git commit-graph verify 2>test_err &&\n \tgrep -v \"^+\" test_err >err\n \ttest_i18ngrep \"$grepstr\" err\n }\n \n+\n test_expect_success 'detect bad signature' '\n \tcorrupt_graph_and_verify 0 \"\\0\" \\\n \t\t\"graph signature\"\n@@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' '\n \t\t\"incorrect checksum\"\n '\n \n+test_expect_success 'detect incorrect chunk count' '\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\xff\" \\\n+\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n+'\n+\n test_expect_success 'git fsck (checks commit-graph)' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit fsck &&\n-- \n2.20.0.rc2.12.g4c11c11dec\n\n"},{"id":"364768","messageId":"06a32bfe8bcde47ede70f2447477803964fbc284.1544221121.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544221121.git.steadmon@google.com","subject":"[PATCH v3 3/3] Makefile: correct example fuzz build","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-07T22:27:24Z","receivedAt":"2018-12-07T22:27:38Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Signed-off-by: Josh Steadmon \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 6b72f37c29..bbcfc2bc9f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3104,7 +3104,7 @@ cover_db_html: cover_db\n # An example command to build against libFuzzer from LLVM 4.0.0:\n #\n # make CC=clang CXX=clang++ \\\n-# FUZZ_CXXFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n+# CFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \\\n # fuzz-all\n #\n-- \n2.20.0.rc2.12.g4c11c11dec\n\n"},{"id":"364835","messageId":"xmqqsgz74acm.fsf@gitster-ct.c.googlers.com","threadId":"49952","inReplyTo":"675d58ecea2f315bd05d2a21f6a473e9de1105a0.1544221121.git.steadmon@google.com","subject":"Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-09T04:01:29Z","receivedAt":"2018-12-09T04:04:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index 5fe21db99f..5b6b44b78e 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n> GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n> GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n> \n> -# usage: corrupt_graph_and_verify \n> +# usage: corrupt_graph_and_verify []\n> # Manipulates the commit-graph file at the position\n> -# by inserting the data, then runs 'git commit-graph verify'\n> +# by inserting the data, optionally zeroing the file\n> +# starting at , then runs 'git commit-graph verify'\n> # and places the output in the file 'err'. Test 'err' for\n> # the given string.\n> corrupt_graph_and_verify() {\n> \tpos=$1\n> \tdata=\"${2:-\\0}\"\n> \tgrepstr=$3\n> +\torig_size=$(stat --format=%s $objdir/info/commit-graph)\n\n\"stat(1)\" is not so portable, so you'll get complaints from minority\nplatform users later. So is \"truncate(1)\".\n\n> +\tzero_pos=${4:-${orig_size}}\n> \tcd \"$TRASH_DIRECTORY/full\" &&\n> \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n> \tcp $objdir/info/commit-graph commit-graph-backup &&\n> \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n> +\ttruncate --size=$zero_pos $objdir/info/commit-graph &&\n> +\ttruncate --size=$orig_size $objdir/info/commit-graph &&\n> \ttest_must_fail git commit-graph verify 2>test_err &&\n> \tgrep -v \"^+\" test_err >err\n> \ttest_i18ngrep \"$grepstr\" err\n> }\n> \n> +\n> test_expect_success 'detect bad signature' '\n> \tcorrupt_graph_and_verify 0 \"\\0\" \\\n> \t\t\"graph signature\"\n> @@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' '\n> \t\t\"incorrect checksum\"\n> '\n> \n> +test_expect_success 'detect incorrect chunk count' '\n> +\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\xff\" \\\n\nImplementations of printf(1) may not grok \"\\xff\" as a valid\nrepresentation of \"\\377\". The shell built-in of dash(1) for example\nwould not work with this.\n\n> +\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n> +'\n> +\n> test_expect_success 'git fsck (checks commit-graph)' '\n> \tcd \"$TRASH_DIRECTORY/full\" &&\n> \tgit fsck &&\n"},{"id":"364901","messageId":"20181210042843.GQ30222@szeder.dev","threadId":"49952","inReplyTo":"xmqqsgz74acm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-12-10T04:28:43Z","receivedAt":"2018-12-10T04:28:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Dec 09, 2018 at 01:01:29PM +0900, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> > index 5fe21db99f..5b6b44b78e 100755\n> > --- a/t/t5318-commit-graph.sh\n> > +++ b/t/t5318-commit-graph.sh\n> > @@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n> > GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n> > GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n> > \n> > -# usage: corrupt_graph_and_verify \n> > +# usage: corrupt_graph_and_verify []\n> > # Manipulates the commit-graph file at the position\n> > -# by inserting the data, then runs 'git commit-graph verify'\n> > +# by inserting the data, optionally zeroing the file\n> > +# starting at , then runs 'git commit-graph verify'\n> > # and places the output in the file 'err'. Test 'err' for\n> > # the given string.\n> > corrupt_graph_and_verify() {\n> > \tpos=$1\n> > \tdata=\"${2:-\\0}\"\n> > \tgrepstr=$3\n> > +\torig_size=$(stat --format=%s $objdir/info/commit-graph)\n> \n> \"stat(1)\" is not so portable, so you'll get complaints from minority\n> platform users later. So is \"truncate(1)\".\n\nI complain: this patch breaks on macOS (on Travis CI), but in a\ncurious way. First, 'stat' in the above line errors out with:\n\n +++stat --format=%s .git/objects/info/commit-graph\n stat: illegal option -- -\n usage: stat [-FlLnqrsx] [-f format] [-t timefmt] [file ...]\n\nAlas, this doesn't immediately fail the test, because it's not part of\nthe &&-chain.\n\n> > +\tzero_pos=${4:-${orig_size}}\n\nNo && here, either.\n\n> > \tcd \"$TRASH_DIRECTORY/full\" &&\n> > \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n> > \tcp $objdir/info/commit-graph commit-graph-backup &&\n> > \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n> > +\ttruncate --size=$zero_pos $objdir/info/commit-graph &&\n\n ++truncate --size= .git/objects/info/commit-graph\n t5318-commit-graph.sh: line 385: truncate: command not found\n\nNote that even if 'truncate' were available, it would most likely\ncomplain about the empty '--size=' argument resulting from the 'stat'\nerror above.\n\nAlas, this doesn't fail the test, either, because ...\n\n> > +\ttruncate --size=$orig_size $objdir/info/commit-graph &&\n> > \ttest_must_fail git commit-graph verify 2>test_err &&\n> > \tgrep -v \"^+\" test_err >err\n\n... here the &&-chain was broken already before this patch. However,\nsince this above command was not executed due to the missing\n'truncate', it didn't have a chance to create the 'err' file, ...\n\n> > \ttest_i18ngrep \"$grepstr\" err\n\n... so 'test_i18ngrep' can't find the file, which triggers its linting\nerror, finally aborting the whole test script.\n\n> > }\n> > \n> > +\n\nStray newline.\n\n> > test_expect_success 'detect bad signature' '\n> > \tcorrupt_graph_and_verify 0 \"\\0\" \\\n> > \t\t\"graph signature\"\n> > @@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' '\n> > \t\t\"incorrect checksum\"\n> > '\n> > \n> > +test_expect_success 'detect incorrect chunk count' '\n> > +\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\xff\" \\\n> \n> Implementations of printf(1) may not grok \"\\xff\" as a valid\n> representation of \"\\377\". The shell built-in of dash(1) for example\n> would not work with this.\n> \n> > +\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n> > +'\n> > +\n> > test_expect_success 'git fsck (checks commit-graph)' '\n> > \tcd \"$TRASH_DIRECTORY/full\" &&\n> > \tgit fsck &&\n"},{"id":"364980","messageId":"20181210215649.GC37614@google.com","threadId":"49952","inReplyTo":"xmqqsgz74acm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-10T21:56:49Z","receivedAt":"2018-12-10T21:56:57Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.12.09 13:01, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> > index 5fe21db99f..5b6b44b78e 100755\n> > --- a/t/t5318-commit-graph.sh\n> > +++ b/t/t5318-commit-graph.sh\n> > @@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n> > GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n> > GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n> > \n> > -# usage: corrupt_graph_and_verify \n> > +# usage: corrupt_graph_and_verify []\n> > # Manipulates the commit-graph file at the position\n> > -# by inserting the data, then runs 'git commit-graph verify'\n> > +# by inserting the data, optionally zeroing the file\n> > +# starting at , then runs 'git commit-graph verify'\n> > # and places the output in the file 'err'. Test 'err' for\n> > # the given string.\n> > corrupt_graph_and_verify() {\n> > \tpos=$1\n> > \tdata=\"${2:-\\0}\"\n> > \tgrepstr=$3\n> > +\torig_size=$(stat --format=%s $objdir/info/commit-graph)\n> \n> \"stat(1)\" is not so portable, so you'll get complaints from minority\n> platform users later. So is \"truncate(1)\".\n\nAck, thanks for the catch. I have a workaround for stat in the form of\n\"wc -c\", and for truncate with a combination of dd and /dev/zero.\nHowever, I'm finding conflicting information about whether or not\n/dev/zero exists on macOS. At the least, it sounds like it might not\nwork on very old versions. Would this be acceptable, or should I add a\nnew test function to do this?\n\n> > +\tzero_pos=${4:-${orig_size}}\n> > \tcd \"$TRASH_DIRECTORY/full\" &&\n> > \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n> > \tcp $objdir/info/commit-graph commit-graph-backup &&\n> > \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n> > +\ttruncate --size=$zero_pos $objdir/info/commit-graph &&\n> > +\ttruncate --size=$orig_size $objdir/info/commit-graph &&\n> > \ttest_must_fail git commit-graph verify 2>test_err &&\n> > \tgrep -v \"^+\" test_err >err\n> > \ttest_i18ngrep \"$grepstr\" err\n> > }\n> > \n> > +\n> > test_expect_success 'detect bad signature' '\n> > \tcorrupt_graph_and_verify 0 \"\\0\" \\\n> > \t\t\"graph signature\"\n> > @@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' '\n> > \t\t\"incorrect checksum\"\n> > '\n> > \n> > +test_expect_success 'detect incorrect chunk count' '\n> > +\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\xff\" \\\n> \n> Implementations of printf(1) may not grok \"\\xff\" as a valid\n> representation of \"\\377\". The shell built-in of dash(1) for example\n> would not work with this.\n\nAck, will fix in V4. Thanks.\n\n> > +\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n> > +'\n> > +\n> > test_expect_success 'git fsck (checks commit-graph)' '\n> > \tcd \"$TRASH_DIRECTORY/full\" &&\n> > \tgit fsck &&\n"},{"id":"364981","messageId":"20181210215807.GD37614@google.com","threadId":"49952","inReplyTo":"20181210042843.GQ30222@szeder.dev","subject":"Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-10T21:58:07Z","receivedAt":"2018-12-10T21:58:16Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.12.10 05:28, SZEDER Gábor wrote:\n> On Sun, Dec 09, 2018 at 01:01:29PM +0900, Junio C Hamano wrote:\n> > Josh Steadmon writes:\n> > \n> > > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> > > index 5fe21db99f..5b6b44b78e 100755\n> > > --- a/t/t5318-commit-graph.sh\n> > > +++ b/t/t5318-commit-graph.sh\n> > > @@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n> > > GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n> > > GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n> > > \n> > > -# usage: corrupt_graph_and_verify \n> > > +# usage: corrupt_graph_and_verify []\n> > > # Manipulates the commit-graph file at the position\n> > > -# by inserting the data, then runs 'git commit-graph verify'\n> > > +# by inserting the data, optionally zeroing the file\n> > > +# starting at , then runs 'git commit-graph verify'\n> > > # and places the output in the file 'err'. Test 'err' for\n> > > # the given string.\n> > > corrupt_graph_and_verify() {\n> > > \tpos=$1\n> > > \tdata=\"${2:-\\0}\"\n> > > \tgrepstr=$3\n> > > +\torig_size=$(stat --format=%s $objdir/info/commit-graph)\n> > \n> > \"stat(1)\" is not so portable, so you'll get complaints from minority\n> > platform users later. So is \"truncate(1)\".\n> \n> I complain: this patch breaks on macOS (on Travis CI), but in a\n> curious way. First, 'stat' in the above line errors out with:\n> \n> +++stat --format=%s .git/objects/info/commit-graph\n> stat: illegal option -- -\n> usage: stat [-FlLnqrsx] [-f format] [-t timefmt] [file ...]\n> \n> Alas, this doesn't immediately fail the test, because it's not part of\n> the &&-chain.\n> \n> > > +\tzero_pos=${4:-${orig_size}}\n> \n> No && here, either.\n> \n> > > \tcd \"$TRASH_DIRECTORY/full\" &&\n> > > \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n> > > \tcp $objdir/info/commit-graph commit-graph-backup &&\n> > > \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n> > > +\ttruncate --size=$zero_pos $objdir/info/commit-graph &&\n> \n> ++truncate --size= .git/objects/info/commit-graph\n> t5318-commit-graph.sh: line 385: truncate: command not found\n> \n> Note that even if 'truncate' were available, it would most likely\n> complain about the empty '--size=' argument resulting from the 'stat'\n> error above.\n> \n> Alas, this doesn't fail the test, either, because ...\n> \n> > > +\ttruncate --size=$orig_size $objdir/info/commit-graph &&\n> > > \ttest_must_fail git commit-graph verify 2>test_err &&\n> > > \tgrep -v \"^+\" test_err >err\n> \n> ... here the &&-chain was broken already before this patch. However,\n> since this above command was not executed due to the missing\n> 'truncate', it didn't have a chance to create the 'err' file, ...\n> \n> > > \ttest_i18ngrep \"$grepstr\" err\n> \n> ... so 'test_i18ngrep' can't find the file, which triggers its linting\n> error, finally aborting the whole test script.\n> \n> > > }\n> > > \n> > > +\n> \n> Stray newline.\n> \n> > > test_expect_success 'detect bad signature' '\n> > > \tcorrupt_graph_and_verify 0 \"\\0\" \\\n> > > \t\t\"graph signature\"\n> > > @@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' '\n> > > \t\t\"incorrect checksum\"\n> > > '\n> > > \n> > > +test_expect_success 'detect incorrect chunk count' '\n> > > +\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\xff\" \\\n> > \n> > Implementations of printf(1) may not grok \"\\xff\" as a valid\n> > representation of \"\\377\". The shell built-in of dash(1) for example\n> > would not work with this.\n> > \n> > > +\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n> > > +'\n> > > +\n> > > test_expect_success 'git fsck (checks commit-graph)' '\n> > > \tcd \"$TRASH_DIRECTORY/full\" &&\n> > > \tgit fsck &&\n\nThanks for the catch. All these will be fixed in V4.\n"},{"id":"365048","messageId":"20181211095002.GB31588@sigill.intra.peff.net","threadId":"49952","inReplyTo":"20181210215649.GC37614@google.com","subject":"Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-11T09:50:02Z","receivedAt":"2018-12-11T09:50:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 10, 2018 at 01:56:49PM -0800, Josh Steadmon wrote:\n\n> > \"stat(1)\" is not so portable, so you'll get complaints from minority\n> > platform users later. So is \"truncate(1)\".\n> \n> Ack, thanks for the catch. I have a workaround for stat in the form of\n> \"wc -c\", and for truncate with a combination of dd and /dev/zero.\n> However, I'm finding conflicting information about whether or not\n> /dev/zero exists on macOS. At the least, it sounds like it might not\n> work on very old versions. Would this be acceptable, or should I add a\n> new test function to do this?\n\nIf you're just interested in truncation (and not a bunch of zero bytes),\nyou can dd from /dev/null.\n\nAnother way to truncate is to move the file elsewhere, and then copy\nonly some of the bytes back (see t1450's \"fsck detects truncated loose\nobject\").\n\n-Peff\n"},{"id":"365289","messageId":"cover.1544729841.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544048946.git.steadmon@google.com","subject":"[PATCH v4 0/3] Add commit-graph fuzzer and fix buffer overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-13T19:43:55Z","receivedAt":"2018-12-13T19:44:04Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Add a new fuzz test for the commit graph and fix a buffer read-overflow\nthat it discovered. Additionally, fix the Makefile instructions for\nbuilding fuzzers.\n\nChanges since V3:\n * Improve portability of the new test functionality.\n * Fix broken &&-chains in tests.\n\nChanges since V2:\n * Avoid pointer arithmetic overflow when checking the graph's chunk\n count.\n * Merge the corrupt_graph_and_verify and\n corrupt_and_zero_graph_then_verify test functions.\n\nJosh Steadmon (3):\n commit-graph, fuzz: Add fuzzer for commit-graph\n commit-graph: fix buffer read-overflow\n Makefile: correct example fuzz build\n\n .gitignore | 1 +\n Makefile | 3 +-\n commit-graph.c | 67 +++++++++++++++++++++++++++++------------\n commit-graph.h | 3 ++\n fuzz-commit-graph.c | 16 ++++++++++\n t/t5318-commit-graph.sh | 16 ++++++++--\n 6 files changed, 83 insertions(+), 23 deletions(-)\n create mode 100644 fuzz-commit-graph.c\n\nRange-diff against v3:\n1: 675d58ecea ! 1: 80b5662f30 commit-graph: fix buffer read-overflow\n @@ -55,29 +55,26 @@\n \tpos=$1\n \tdata=\"${2:-\\0}\"\n \tgrepstr=$3\n -+\torig_size=$(stat --format=%s $objdir/info/commit-graph)\n -+\tzero_pos=${4:-${orig_size}}\n ++\torig_size=$(wc -c < $objdir/info/commit-graph) &&\n ++\tzero_pos=${4:-${orig_size}} &&\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n \tcp $objdir/info/commit-graph commit-graph-backup &&\n \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n -+\ttruncate --size=$zero_pos $objdir/info/commit-graph &&\n -+\ttruncate --size=$orig_size $objdir/info/commit-graph &&\n ++\tdd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=0 &&\n ++\tdd if=/dev/zero of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=$(($orig_size - $zero_pos)) &&\n \ttest_must_fail git commit-graph verify 2>test_err &&\n - \tgrep -v \"^+\" test_err >err\n +-\tgrep -v \"^+\" test_err >err\n ++\tgrep -v \"^+\" test_err >err &&\n \ttest_i18ngrep \"$grepstr\" err\n }\n \n -+\n - test_expect_success 'detect bad signature' '\n - \tcorrupt_graph_and_verify 0 \"\\0\" \\\n - \t\t\"graph signature\"\n @@\n \t\t\"incorrect checksum\"\n '\n \n +test_expect_success 'detect incorrect chunk count' '\n -+\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\xff\" \\\n ++\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\377\" \\\n +\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n +'\n +\n2: 06a32bfe8b = 2: 21101b961a Makefile: correct example fuzz build\n-- \n2.20.0.rc2.10.g21101b961a\n\n"},{"id":"365290","messageId":"0b57ecbe1b6c72d95311ba1bb30ea1ed9fd7e3e5.1544729841.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544729841.git.steadmon@google.com","subject":"[PATCH v4 1/3] commit-graph, fuzz: Add fuzzer for commit-graph","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-13T19:43:56Z","receivedAt":"2018-12-13T19:44:07Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Breaks load_commit_graph_one() into a new function,\nparse_commit_graph(). The latter function operates on arbitrary buffers,\nwhich makes it suitable as a fuzzing target. Since parse_commit_graph()\nis only called by load_commit_graph_one() (and the fuzzer described\nbelow), we omit error messages that would be duplicated by the caller.\n\nAdds fuzz-commit-graph.c, which provides a fuzzing entry point\ncompatible with libFuzzer (and possibly other fuzzing engines).\n\nSigned-off-by: Josh Steadmon \n---\n .gitignore | 1 +\n Makefile | 1 +\n commit-graph.c | 53 ++++++++++++++++++++++++++++++---------------\n commit-graph.h | 3 +++\n fuzz-commit-graph.c | 16 ++++++++++++++\n 5 files changed, 57 insertions(+), 17 deletions(-)\n create mode 100644 fuzz-commit-graph.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 0d77ea5894..8bcf153ed9 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -1,3 +1,4 @@\n+/fuzz-commit-graph\n /fuzz_corpora\n /fuzz-pack-headers\n /fuzz-pack-idx\ndiff --git a/Makefile b/Makefile\nindex 1a44c811aa..6b72f37c29 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \\\n \n ETAGS_TARGET = TAGS\n \n+FUZZ_OBJS += fuzz-commit-graph.o\n FUZZ_OBJS += fuzz-pack-headers.o\n FUZZ_OBJS += fuzz-pack-idx.o\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 40c855f185..07dd410f3c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -84,16 +84,10 @@ static int commit_graph_compatible(struct repository *r)\n struct commit_graph *load_commit_graph_one(const char *graph_file)\n {\n \tvoid *graph_map;\n-\tconst unsigned char *data, *chunk_lookup;\n \tsize_t graph_size;\n \tstruct stat st;\n-\tuint32_t i;\n-\tstruct commit_graph *graph;\n+\tstruct commit_graph *ret;\n \tint fd = git_open(graph_file);\n-\tuint64_t last_chunk_offset;\n-\tuint32_t last_chunk_id;\n-\tuint32_t graph_signature;\n-\tunsigned char graph_version, hash_version;\n \n \tif (fd < 0)\n \t\treturn NULL;\n@@ -108,27 +102,55 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t\tdie(_(\"graph file %s is too small\"), graph_file);\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tret = parse_commit_graph(graph_map, fd, graph_size);\n+\n+\tif (!ret) {\n+\t\tmunmap(graph_map, graph_size);\n+\t\tclose(fd);\n+\t\texit(1);\n+\t}\n+\n+\treturn ret;\n+}\n+\n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size)\n+{\n+\tconst unsigned char *data, *chunk_lookup;\n+\tuint32_t i;\n+\tstruct commit_graph *graph;\n+\tuint64_t last_chunk_offset;\n+\tuint32_t last_chunk_id;\n+\tuint32_t graph_signature;\n+\tunsigned char graph_version, hash_version;\n+\n+\tif (!graph_map)\n+\t\treturn NULL;\n+\n+\tif (graph_size < GRAPH_MIN_SIZE)\n+\t\treturn NULL;\n+\n \tdata = (const unsigned char *)graph_map;\n \n \tgraph_signature = get_be32(data);\n \tif (graph_signature != GRAPH_SIGNATURE) {\n \t\terror(_(\"graph signature %X does not match signature %X\"),\n \t\t graph_signature, GRAPH_SIGNATURE);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \tgraph_version = *(unsigned char*)(data + 4);\n \tif (graph_version != GRAPH_VERSION) {\n \t\terror(_(\"graph version %X does not match version %X\"),\n \t\t graph_version, GRAPH_VERSION);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \thash_version = *(unsigned char*)(data + 5);\n \tif (hash_version != GRAPH_OID_VERSION) {\n \t\terror(_(\"hash version %X does not match version %X\"),\n \t\t hash_version, GRAPH_OID_VERSION);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \tgraph = alloc_commit_graph();\n@@ -152,7 +174,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\n \t\t\terror(_(\"improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n \t\t\t (uint32_t)chunk_offset);\n-\t\t\tgoto cleanup_fail;\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n \t\t}\n \n \t\tswitch (chunk_id) {\n@@ -187,7 +210,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \n \t\tif (chunk_repeated) {\n \t\t\terror(_(\"chunk id %08x appears multiple times\"), chunk_id);\n-\t\t\tgoto cleanup_fail;\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n \t\t}\n \n \t\tif (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)\n@@ -201,11 +225,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t}\n \n \treturn graph;\n-\n-cleanup_fail:\n-\tmunmap(graph_map, graph_size);\n-\tclose(fd);\n-\texit(1);\n }\n \n static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 9db40b4d3a..813e7c19f1 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -54,6 +54,9 @@ struct commit_graph {\n \n struct commit_graph *load_commit_graph_one(const char *graph_file);\n \n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size);\n+\n /*\n * Return 1 if and only if the repository has a commit-graph\n * file and generation numbers are computed in that file.\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nnew file mode 100644\nindex 0000000000..cf790c9d04\n--- /dev/null\n+++ b/fuzz-commit-graph.c\n@@ -0,0 +1,16 @@\n+#include \"commit-graph.h\"\n+\n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size);\n+\n+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n+\n+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n+{\n+\tstruct commit_graph *g;\n+\n+\tg = parse_commit_graph((void *)data, -1, size);\n+\tfree(g);\n+\n+\treturn 0;\n+}\n-- \n2.20.0.rc2.10.g21101b961a\n\n"},{"id":"365291","messageId":"80b5662f30cb87210385a6bd26e037ce39e46b6d.1544729841.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544729841.git.steadmon@google.com","subject":"[PATCH v4 2/3] commit-graph: fix buffer read-overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-13T19:43:57Z","receivedAt":"2018-12-13T19:44:10Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"fuzz-commit-graph identified a case where Git will read past the end of\na buffer containing a commit graph if the graph's header has an\nincorrect chunk count. A simple bounds check in parse_commit_graph()\nprevents this.\n\nSigned-off-by: Josh Steadmon \n---\n commit-graph.c | 14 ++++++++++++--\n t/t5318-commit-graph.sh | 16 +++++++++++++---\n 2 files changed, 25 insertions(+), 5 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 07dd410f3c..836d65a1d3 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \tlast_chunk_offset = 8;\n \tchunk_lookup = data + 8;\n \tfor (i = 0; i < graph->num_chunks; i++) {\n-\t\tuint32_t chunk_id = get_be32(chunk_lookup + 0);\n-\t\tuint64_t chunk_offset = get_be64(chunk_lookup + 4);\n+\t\tuint32_t chunk_id;\n+\t\tuint64_t chunk_offset;\n \t\tint chunk_repeated = 0;\n \n+\t\tif (data + graph_size - chunk_lookup <\n+\t\t GRAPH_CHUNKLOOKUP_WIDTH) {\n+\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\tchunk_id = get_be32(chunk_lookup + 0);\n+\t\tchunk_offset = get_be64(chunk_lookup + 4);\n+\n \t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n \n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 5fe21db99f..a1b5a75882 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -366,21 +366,26 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n \n-# usage: corrupt_graph_and_verify \n+# usage: corrupt_graph_and_verify []\n # Manipulates the commit-graph file at the position\n-# by inserting the data, then runs 'git commit-graph verify'\n+# by inserting the data, optionally zeroing the file\n+# starting at , then runs 'git commit-graph verify'\n # and places the output in the file 'err'. Test 'err' for\n # the given string.\n corrupt_graph_and_verify() {\n \tpos=$1\n \tdata=\"${2:-\\0}\"\n \tgrepstr=$3\n+\torig_size=$(wc -c < $objdir/info/commit-graph) &&\n+\tzero_pos=${4:-${orig_size}} &&\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n \tcp $objdir/info/commit-graph commit-graph-backup &&\n \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n+\tdd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=0 &&\n+\tdd if=/dev/zero of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=$(($orig_size - $zero_pos)) &&\n \ttest_must_fail git commit-graph verify 2>test_err &&\n-\tgrep -v \"^+\" test_err >err\n+\tgrep -v \"^+\" test_err >err &&\n \ttest_i18ngrep \"$grepstr\" err\n }\n \n@@ -484,6 +489,11 @@ test_expect_success 'detect invalid checksum hash' '\n \t\t\"incorrect checksum\"\n '\n \n+test_expect_success 'detect incorrect chunk count' '\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\377\" \\\n+\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n+'\n+\n test_expect_success 'git fsck (checks commit-graph)' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit fsck &&\n-- \n2.20.0.rc2.10.g21101b961a\n\n"},{"id":"365292","messageId":"21101b961ac602be766b6947143cf80d322e36c2.1544729841.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544729841.git.steadmon@google.com","subject":"[PATCH v4 3/3] Makefile: correct example fuzz build","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-13T19:43:58Z","receivedAt":"2018-12-13T19:44:12Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Signed-off-by: Josh Steadmon \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 6b72f37c29..bbcfc2bc9f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3104,7 +3104,7 @@ cover_db_html: cover_db\n # An example command to build against libFuzzer from LLVM 4.0.0:\n #\n # make CC=clang CXX=clang++ \\\n-# FUZZ_CXXFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n+# CFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \\\n # fuzz-all\n #\n-- \n2.20.0.rc2.10.g21101b961a\n\n"},{"id":"365545","messageId":"20181218173539.GA31070@sigill.intra.peff.net","threadId":"49952","inReplyTo":"cover.1544729841.git.steadmon@google.com","subject":"Re: [PATCH v4 0/3] Add commit-graph fuzzer and fix buffer overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-18T17:35:40Z","receivedAt":"2018-12-18T17:35:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:\n\n> Add a new fuzz test for the commit graph and fix a buffer read-overflow\n> that it discovered. Additionally, fix the Makefile instructions for\n> building fuzzers.\n> \n> Changes since V3:\n> * Improve portability of the new test functionality.\n\nI thought there was some question about /dev/zero, which I think is\nin this version (I don't actually know whether there are portability\nissues or not, but somebody did mention it).\n\n-Peff\n"},{"id":"365552","messageId":"20181218210551.GG37614@google.com","threadId":"49952","inReplyTo":"20181218173539.GA31070@sigill.intra.peff.net","subject":"Re: [PATCH v4 0/3] Add commit-graph fuzzer and fix buffer overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-18T21:05:51Z","receivedAt":"2018-12-18T21:06:00Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.12.18 12:35, Jeff King wrote:\n> On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:\n> \n> > Add a new fuzz test for the commit graph and fix a buffer read-overflow\n> > that it discovered. Additionally, fix the Makefile instructions for\n> > building fuzzers.\n> > \n> > Changes since V3:\n> > * Improve portability of the new test functionality.\n> \n> I thought there was some question about /dev/zero, which I think is\n> in this version (I don't actually know whether there are portability\n> issues or not, but somebody did mention it).\n> \n> -Peff\n\nI've only found one reference [1] (from 1999) of OS X Server not having\na /dev/zero. It appears to be present as of 2010 though [2].\n\n[1]: https://macosx-admin.omnigroup.narkive.com/sAxj0XeP/dev-zero-equivalent-in-mac-os-x\n[2]: https://serverfault.com/questions/143248/zeroing-a-disk-with-dd-vs-disk-utility\n"},{"id":"365580","messageId":"20181219155107.GD14802@sigill.intra.peff.net","threadId":"49952","inReplyTo":"20181218210551.GG37614@google.com","subject":"Re: [PATCH v4 0/3] Add commit-graph fuzzer and fix buffer overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-19T15:51:07Z","receivedAt":"2018-12-19T15:51:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 18, 2018 at 01:05:51PM -0800, Josh Steadmon wrote:\n\n> On 2018.12.18 12:35, Jeff King wrote:\n> > On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:\n> > \n> > > Add a new fuzz test for the commit graph and fix a buffer read-overflow\n> > > that it discovered. Additionally, fix the Makefile instructions for\n> > > building fuzzers.\n> > > \n> > > Changes since V3:\n> > > * Improve portability of the new test functionality.\n> > \n> > I thought there was some question about /dev/zero, which I think is\n> > in this version (I don't actually know whether there are portability\n> > issues or not, but somebody did mention it).\n> > \n> > -Peff\n> \n> I've only found one reference [1] (from 1999) of OS X Server not having\n> a /dev/zero. It appears to be present as of 2010 though [2].\n\nThanks for digging. That seems like enough to assume we should try it\nand see if any macOS people complain.\n\nI do wonder if we'll run into problems on Windows, though.\n\n-Peff\n"},{"id":"365670","messageId":"nycvar.QRO.7.76.6.1812202034390.41@tvgsbejvaqbjf.bet","threadId":"49952","inReplyTo":"20181219155107.GD14802@sigill.intra.peff.net","subject":"Re: [PATCH v4 0/3] Add commit-graph fuzzer and fix buffer overflow","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-20T19:35:57Z","receivedAt":"2018-12-20T19:36:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\n\nOn Wed, 19 Dec 2018, Jeff King wrote:\n\n> On Tue, Dec 18, 2018 at 01:05:51PM -0800, Josh Steadmon wrote:\n> \n> > On 2018.12.18 12:35, Jeff King wrote:\n> > > On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:\n> > > \n> > > > Add a new fuzz test for the commit graph and fix a buffer read-overflow\n> > > > that it discovered. Additionally, fix the Makefile instructions for\n> > > > building fuzzers.\n> > > > \n> > > > Changes since V3:\n> > > > * Improve portability of the new test functionality.\n> > > \n> > > I thought there was some question about /dev/zero, which I think is\n> > > in this version (I don't actually know whether there are portability\n> > > issues or not, but somebody did mention it).\n> > > \n> > > -Peff\n> > \n> > I've only found one reference [1] (from 1999) of OS X Server not having\n> > a /dev/zero. It appears to be present as of 2010 though [2].\n> \n> Thanks for digging. That seems like enough to assume we should try it\n> and see if any macOS people complain.\n> \n> I do wonder if we'll run into problems on Windows, though.\n\nAs long as we're talking about Unix shell scripts, /dev/zero should be\nfine, as we are essentially running in a variant of Cygwin.\n\nIf you try to pass /dev/zero as an argument to a Git command, that's an\nentirely different thing: this most likely won't work.\n\nCiao,\nDscho\n"},{"id":"365701","messageId":"20181220201145.GA11002@sigill.intra.peff.net","threadId":"49952","inReplyTo":"nycvar.QRO.7.76.6.1812202034390.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 0/3] Add commit-graph fuzzer and fix buffer overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-20T20:11:46Z","receivedAt":"2018-12-20T20:13:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 20, 2018 at 08:35:57PM +0100, Johannes Schindelin wrote:\n\n> > I do wonder if we'll run into problems on Windows, though.\n> \n> As long as we're talking about Unix shell scripts, /dev/zero should be\n> fine, as we are essentially running in a variant of Cygwin.\n> \n> If you try to pass /dev/zero as an argument to a Git command, that's an\n> entirely different thing: this most likely won't work.\n\nThanks for confirming. We're talking about passing it to dd here, so I\nthink it should be OK.\n\n-Peff\n"},{"id":"365820","messageId":"xmqqbm57rkg5.fsf@gitster-ct.c.googlers.com","threadId":"49952","inReplyTo":"20181219155107.GD14802@sigill.intra.peff.net","subject":"Re: [PATCH v4 0/3] Add commit-graph fuzzer and fix buffer overflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-26T22:29:14Z","receivedAt":"2018-12-26T22:29:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Dec 18, 2018 at 01:05:51PM -0800, Josh Steadmon wrote:\n>\n>> On 2018.12.18 12:35, Jeff King wrote:\n>> > On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:\n>> > \n>> > > Add a new fuzz test for the commit graph and fix a buffer read-overflow\n>> > > that it discovered. Additionally, fix the Makefile instructions for\n>> > > building fuzzers.\n>> > > \n>> > > Changes since V3:\n>> > > * Improve portability of the new test functionality.\n>> > \n>> > I thought there was some question about /dev/zero, which I think is\n>> > in this version (I don't actually know whether there are portability\n>> > issues or not, but somebody did mention it).\n>> > \n>> > -Peff\n>> \n>> I've only found one reference [1] (from 1999) of OS X Server not having\n>> a /dev/zero. It appears to be present as of 2010 though [2].\n>\n> Thanks for digging. That seems like enough to assume we should try it\n> and see if any macOS people complain.\n\nOur tests have been relying on /dev/zero since 852a1710 (\"am: let\ncommand-line options override saved options\", 2015-08-04) that\nappeared in v2.6.0. Anybody who has trouble with /dev/zero now has\nkept silent for about a dozen major releases, I think, and will be\nsilent with this one, too ;-)\n\n>\n> I do wonder if we'll run into problems on Windows, though.\n>\n> -Peff\n"},{"id":"366621","messageId":"20190112105757.GE840@szeder.dev","threadId":"49952","inReplyTo":"80b5662f30cb87210385a6bd26e037ce39e46b6d.1544729841.git.steadmon@google.com","subject":"Re: [PATCH v4 2/3] commit-graph: fix buffer read-overflow","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-12T10:57:57Z","receivedAt":"2019-01-12T10:58:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Dec 13, 2018 at 11:43:57AM -0800, Josh Steadmon wrote:\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index 5fe21db99f..a1b5a75882 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -366,21 +366,26 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n> GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n> GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n> \n> -# usage: corrupt_graph_and_verify \n> +# usage: corrupt_graph_and_verify []\n> # Manipulates the commit-graph file at the position\n> -# by inserting the data, then runs 'git commit-graph verify'\n> +# by inserting the data, optionally zeroing the file\n> +# starting at , then runs 'git commit-graph verify'\n> # and places the output in the file 'err'. Test 'err' for\n> # the given string.\n> corrupt_graph_and_verify() {\n> \tpos=$1\n> \tdata=\"${2:-\\0}\"\n> \tgrepstr=$3\n> +\torig_size=$(wc -c < $objdir/info/commit-graph) &&\n\nA minor nit: this test script is unusually prudent about which\ndirectory/repository each test is executed in, as the first thing each\ntest does is to 'cd' into the right directory. (I think this is a\nGood Thing, and other test scripts should follow suit if they use a\nrepo other than $TRASH_DIRECTORY.) Though it doesn't cause any\nimmediate issues (the previous test happens to use the same\nrepository), the above line violates this, as it accesses the\n'.git/.../commit-graph' file ...\n\n> +\tzero_pos=${4:-${orig_size}} &&\n> \tcd \"$TRASH_DIRECTORY/full\" &&\n\n... before this line could ensure that it's in the right repository.\n\n> \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n> \tcp $objdir/info/commit-graph commit-graph-backup &&\n> \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n> +\tdd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=0 &&\n> +\tdd if=/dev/zero of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=$(($orig_size - $zero_pos)) &&\n> \ttest_must_fail git commit-graph verify 2>test_err &&\n> -\tgrep -v \"^+\" test_err >err\n> +\tgrep -v \"^+\" test_err >err &&\n> \ttest_i18ngrep \"$grepstr\" err\n> }\n> \n> @@ -484,6 +489,11 @@ test_expect_success 'detect invalid checksum hash' '\n> \t\t\"incorrect checksum\"\n> '\n> \n> +test_expect_success 'detect incorrect chunk count' '\n> +\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\377\" \\\n> +\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n> +'\n> +\n> test_expect_success 'git fsck (checks commit-graph)' '\n> \tcd \"$TRASH_DIRECTORY/full\" &&\n> \tgit fsck &&\n> -- \n> 2.20.0.rc2.10.g21101b961a\n> \n"},{"id":"366756","messageId":"20190115195808.GA35794@google.com","threadId":"49952","inReplyTo":"20190112105757.GE840@szeder.dev","subject":"Re: [PATCH v4 2/3] commit-graph: fix buffer read-overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-15T19:58:08Z","receivedAt":"2019-01-15T19:58:19Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2019.01.12 11:57, SZEDER Gábor wrote:\n> On Thu, Dec 13, 2018 at 11:43:57AM -0800, Josh Steadmon wrote:\n> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> > index 5fe21db99f..a1b5a75882 100755\n> > --- a/t/t5318-commit-graph.sh\n> > +++ b/t/t5318-commit-graph.sh\n> > @@ -366,21 +366,26 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n> > GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n> > GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n> > \n> > -# usage: corrupt_graph_and_verify \n> > +# usage: corrupt_graph_and_verify []\n> > # Manipulates the commit-graph file at the position\n> > -# by inserting the data, then runs 'git commit-graph verify'\n> > +# by inserting the data, optionally zeroing the file\n> > +# starting at , then runs 'git commit-graph verify'\n> > # and places the output in the file 'err'. Test 'err' for\n> > # the given string.\n> > corrupt_graph_and_verify() {\n> > \tpos=$1\n> > \tdata=\"${2:-\\0}\"\n> > \tgrepstr=$3\n> > +\torig_size=$(wc -c < $objdir/info/commit-graph) &&\n> \n> A minor nit: this test script is unusually prudent about which\n> directory/repository each test is executed in, as the first thing each\n> test does is to 'cd' into the right directory. (I think this is a\n> Good Thing, and other test scripts should follow suit if they use a\n> repo other than $TRASH_DIRECTORY.) Though it doesn't cause any\n> immediate issues (the previous test happens to use the same\n> repository), the above line violates this, as it accesses the\n> '.git/.../commit-graph' file ...\n> \n> > +\tzero_pos=${4:-${orig_size}} &&\n> > \tcd \"$TRASH_DIRECTORY/full\" &&\n> \n> ... before this line could ensure that it's in the right repository.\n\nThanks for the catch. Fixed in v5.\n"},{"id":"366757","messageId":"cover.1547582104.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544048946.git.steadmon@google.com","subject":"[PATCH v5 0/3] Add commit-graph fuzzer and fix buffer overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-15T19:59:52Z","receivedAt":"2019-01-15T20:00:02Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Add a new fuzz test for the commit graph and fix a buffer read-overflow\nthat it discovered. Additionally, fix the Makefile instructions for\nbuilding fuzzers.\n\nChanges since V4:\n * Ensure that corrupt_graph_and_verify() in t5318 changes to the\n proper directory before accessing any files.\n\nChanges since V3:\n * Improve portability of the new test functionality.\n * Fix broken &&-chains in tests.\n\nChanges since V2:\n * Avoid pointer arithmetic overflow when checking the graph's chunk\n count.\n * Merge the corrupt_graph_and_verify and\n corrupt_and_zero_graph_then_verify test functions.\n\nJosh Steadmon (3):\n commit-graph, fuzz: Add fuzzer for commit-graph\n commit-graph: fix buffer read-overflow\n Makefile: correct example fuzz build\n\nJosh Steadmon (3):\n commit-graph, fuzz: Add fuzzer for commit-graph\n commit-graph: fix buffer read-overflow\n Makefile: correct example fuzz build\n\n .gitignore | 1 +\n Makefile | 3 +-\n commit-graph.c | 67 +++++++++++++++++++++++++++++------------\n commit-graph.h | 3 ++\n fuzz-commit-graph.c | 16 ++++++++++\n t/t5318-commit-graph.sh | 16 ++++++++--\n 6 files changed, 83 insertions(+), 23 deletions(-)\n create mode 100644 fuzz-commit-graph.c\n\nRange-diff against v4:\n1: 80b5662f30 ! 1: a3b5d33c4b commit-graph: fix buffer read-overflow\n @@ -52,12 +52,12 @@\n # and places the output in the file 'err'. Test 'err' for\n # the given string.\n corrupt_graph_and_verify() {\n - \tpos=$1\n +@@\n \tdata=\"${2:-\\0}\"\n \tgrepstr=$3\n + \tcd \"$TRASH_DIRECTORY/full\" &&\n +\torig_size=$(wc -c < $objdir/info/commit-graph) &&\n +\tzero_pos=${4:-${orig_size}} &&\n - \tcd \"$TRASH_DIRECTORY/full\" &&\n \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n \tcp $objdir/info/commit-graph commit-graph-backup &&\n \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n2: 21101b961a = 2: 350ea5f7c9 Makefile: correct example fuzz build\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366758","messageId":"0b57ecbe1b6c72d95311ba1bb30ea1ed9fd7e3e5.1547582104.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1547582104.git.steadmon@google.com","subject":"[PATCH v5 1/3] commit-graph, fuzz: Add fuzzer for commit-graph","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-15T19:59:53Z","receivedAt":"2019-01-15T20:00:05Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Breaks load_commit_graph_one() into a new function,\nparse_commit_graph(). The latter function operates on arbitrary buffers,\nwhich makes it suitable as a fuzzing target. Since parse_commit_graph()\nis only called by load_commit_graph_one() (and the fuzzer described\nbelow), we omit error messages that would be duplicated by the caller.\n\nAdds fuzz-commit-graph.c, which provides a fuzzing entry point\ncompatible with libFuzzer (and possibly other fuzzing engines).\n\nSigned-off-by: Josh Steadmon \n---\n .gitignore | 1 +\n Makefile | 1 +\n commit-graph.c | 53 ++++++++++++++++++++++++++++++---------------\n commit-graph.h | 3 +++\n fuzz-commit-graph.c | 16 ++++++++++++++\n 5 files changed, 57 insertions(+), 17 deletions(-)\n create mode 100644 fuzz-commit-graph.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 0d77ea5894..8bcf153ed9 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -1,3 +1,4 @@\n+/fuzz-commit-graph\n /fuzz_corpora\n /fuzz-pack-headers\n /fuzz-pack-idx\ndiff --git a/Makefile b/Makefile\nindex 1a44c811aa..6b72f37c29 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \\\n \n ETAGS_TARGET = TAGS\n \n+FUZZ_OBJS += fuzz-commit-graph.o\n FUZZ_OBJS += fuzz-pack-headers.o\n FUZZ_OBJS += fuzz-pack-idx.o\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 40c855f185..07dd410f3c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -84,16 +84,10 @@ static int commit_graph_compatible(struct repository *r)\n struct commit_graph *load_commit_graph_one(const char *graph_file)\n {\n \tvoid *graph_map;\n-\tconst unsigned char *data, *chunk_lookup;\n \tsize_t graph_size;\n \tstruct stat st;\n-\tuint32_t i;\n-\tstruct commit_graph *graph;\n+\tstruct commit_graph *ret;\n \tint fd = git_open(graph_file);\n-\tuint64_t last_chunk_offset;\n-\tuint32_t last_chunk_id;\n-\tuint32_t graph_signature;\n-\tunsigned char graph_version, hash_version;\n \n \tif (fd < 0)\n \t\treturn NULL;\n@@ -108,27 +102,55 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t\tdie(_(\"graph file %s is too small\"), graph_file);\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tret = parse_commit_graph(graph_map, fd, graph_size);\n+\n+\tif (!ret) {\n+\t\tmunmap(graph_map, graph_size);\n+\t\tclose(fd);\n+\t\texit(1);\n+\t}\n+\n+\treturn ret;\n+}\n+\n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size)\n+{\n+\tconst unsigned char *data, *chunk_lookup;\n+\tuint32_t i;\n+\tstruct commit_graph *graph;\n+\tuint64_t last_chunk_offset;\n+\tuint32_t last_chunk_id;\n+\tuint32_t graph_signature;\n+\tunsigned char graph_version, hash_version;\n+\n+\tif (!graph_map)\n+\t\treturn NULL;\n+\n+\tif (graph_size < GRAPH_MIN_SIZE)\n+\t\treturn NULL;\n+\n \tdata = (const unsigned char *)graph_map;\n \n \tgraph_signature = get_be32(data);\n \tif (graph_signature != GRAPH_SIGNATURE) {\n \t\terror(_(\"graph signature %X does not match signature %X\"),\n \t\t graph_signature, GRAPH_SIGNATURE);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \tgraph_version = *(unsigned char*)(data + 4);\n \tif (graph_version != GRAPH_VERSION) {\n \t\terror(_(\"graph version %X does not match version %X\"),\n \t\t graph_version, GRAPH_VERSION);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \thash_version = *(unsigned char*)(data + 5);\n \tif (hash_version != GRAPH_OID_VERSION) {\n \t\terror(_(\"hash version %X does not match version %X\"),\n \t\t hash_version, GRAPH_OID_VERSION);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \tgraph = alloc_commit_graph();\n@@ -152,7 +174,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\n \t\t\terror(_(\"improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n \t\t\t (uint32_t)chunk_offset);\n-\t\t\tgoto cleanup_fail;\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n \t\t}\n \n \t\tswitch (chunk_id) {\n@@ -187,7 +210,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \n \t\tif (chunk_repeated) {\n \t\t\terror(_(\"chunk id %08x appears multiple times\"), chunk_id);\n-\t\t\tgoto cleanup_fail;\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n \t\t}\n \n \t\tif (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)\n@@ -201,11 +225,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t}\n \n \treturn graph;\n-\n-cleanup_fail:\n-\tmunmap(graph_map, graph_size);\n-\tclose(fd);\n-\texit(1);\n }\n \n static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 9db40b4d3a..813e7c19f1 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -54,6 +54,9 @@ struct commit_graph {\n \n struct commit_graph *load_commit_graph_one(const char *graph_file);\n \n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size);\n+\n /*\n * Return 1 if and only if the repository has a commit-graph\n * file and generation numbers are computed in that file.\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nnew file mode 100644\nindex 0000000000..cf790c9d04\n--- /dev/null\n+++ b/fuzz-commit-graph.c\n@@ -0,0 +1,16 @@\n+#include \"commit-graph.h\"\n+\n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size);\n+\n+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n+\n+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n+{\n+\tstruct commit_graph *g;\n+\n+\tg = parse_commit_graph((void *)data, -1, size);\n+\tfree(g);\n+\n+\treturn 0;\n+}\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366759","messageId":"a3b5d33c4b4cb3df0c197f85e9e8ce37baf2a4db.1547582104.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1547582104.git.steadmon@google.com","subject":"[PATCH v5 2/3] commit-graph: fix buffer read-overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-15T19:59:54Z","receivedAt":"2019-01-15T20:00:08Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"fuzz-commit-graph identified a case where Git will read past the end of\na buffer containing a commit graph if the graph's header has an\nincorrect chunk count. A simple bounds check in parse_commit_graph()\nprevents this.\n\nSigned-off-by: Josh Steadmon \n---\n commit-graph.c | 14 ++++++++++++--\n t/t5318-commit-graph.sh | 16 +++++++++++++---\n 2 files changed, 25 insertions(+), 5 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 07dd410f3c..836d65a1d3 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \tlast_chunk_offset = 8;\n \tchunk_lookup = data + 8;\n \tfor (i = 0; i < graph->num_chunks; i++) {\n-\t\tuint32_t chunk_id = get_be32(chunk_lookup + 0);\n-\t\tuint64_t chunk_offset = get_be64(chunk_lookup + 4);\n+\t\tuint32_t chunk_id;\n+\t\tuint64_t chunk_offset;\n \t\tint chunk_repeated = 0;\n \n+\t\tif (data + graph_size - chunk_lookup <\n+\t\t GRAPH_CHUNKLOOKUP_WIDTH) {\n+\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\tchunk_id = get_be32(chunk_lookup + 0);\n+\t\tchunk_offset = get_be64(chunk_lookup + 4);\n+\n \t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n \n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 5fe21db99f..694f26079f 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -366,9 +366,10 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n \n-# usage: corrupt_graph_and_verify \n+# usage: corrupt_graph_and_verify []\n # Manipulates the commit-graph file at the position\n-# by inserting the data, then runs 'git commit-graph verify'\n+# by inserting the data, optionally zeroing the file\n+# starting at , then runs 'git commit-graph verify'\n # and places the output in the file 'err'. Test 'err' for\n # the given string.\n corrupt_graph_and_verify() {\n@@ -376,11 +377,15 @@ corrupt_graph_and_verify() {\n \tdata=\"${2:-\\0}\"\n \tgrepstr=$3\n \tcd \"$TRASH_DIRECTORY/full\" &&\n+\torig_size=$(wc -c < $objdir/info/commit-graph) &&\n+\tzero_pos=${4:-${orig_size}} &&\n \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n \tcp $objdir/info/commit-graph commit-graph-backup &&\n \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n+\tdd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=0 &&\n+\tdd if=/dev/zero of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=$(($orig_size - $zero_pos)) &&\n \ttest_must_fail git commit-graph verify 2>test_err &&\n-\tgrep -v \"^+\" test_err >err\n+\tgrep -v \"^+\" test_err >err &&\n \ttest_i18ngrep \"$grepstr\" err\n }\n \n@@ -484,6 +489,11 @@ test_expect_success 'detect invalid checksum hash' '\n \t\t\"incorrect checksum\"\n '\n \n+test_expect_success 'detect incorrect chunk count' '\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\377\" \\\n+\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n+'\n+\n test_expect_success 'git fsck (checks commit-graph)' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit fsck &&\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366760","messageId":"350ea5f7c97aa4166eea56aa57b66ddc9c53de4a.1547582104.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1547582104.git.steadmon@google.com","subject":"[PATCH v5 3/3] Makefile: correct example fuzz build","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-15T19:59:55Z","receivedAt":"2019-01-15T20:00:12Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Signed-off-by: Josh Steadmon \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 6b72f37c29..bbcfc2bc9f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3104,7 +3104,7 @@ cover_db_html: cover_db\n # An example command to build against libFuzzer from LLVM 4.0.0:\n #\n # make CC=clang CXX=clang++ \\\n-# FUZZ_CXXFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n+# CFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \\\n # fuzz-all\n #\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366762","messageId":"xmqqmuo1r77t.fsf@gitster-ct.c.googlers.com","threadId":"49952","inReplyTo":"0b57ecbe1b6c72d95311ba1bb30ea1ed9fd7e3e5.1547582104.git.steadmon@google.com","subject":"Re: [PATCH v5 1/3] commit-graph, fuzz: Add fuzzer for commit-graph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-15T20:33:58Z","receivedAt":"2019-01-15T20:34:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Breaks load_commit_graph_one() into a new function,\n\ns/Breaks/Break/\n\n> parse_commit_graph(). The latter function operates on arbitrary buffers,\n> which makes it suitable as a fuzzing target. Since parse_commit_graph()\n> is only called by load_commit_graph_one() (and the fuzzer described\n> below), we omit error messages that would be duplicated by the caller.\n>\n> Adds fuzz-commit-graph.c, which provides a fuzzing entry point\n> compatible with libFuzzer (and possibly other fuzzing engines).\n>\n> Signed-off-by: Josh Steadmon \n> ---\n> .gitignore | 1 +\n> Makefile | 1 +\n> commit-graph.c | 53 ++++++++++++++++++++++++++++++---------------\n> commit-graph.h | 3 +++\n> fuzz-commit-graph.c | 16 ++++++++++++++\n> 5 files changed, 57 insertions(+), 17 deletions(-)\n> create mode 100644 fuzz-commit-graph.c\n>\n> diff --git a/.gitignore b/.gitignore\n> index 0d77ea5894..8bcf153ed9 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -1,3 +1,4 @@\n> +/fuzz-commit-graph\n> /fuzz_corpora\n> /fuzz-pack-headers\n> /fuzz-pack-idx\n> diff --git a/Makefile b/Makefile\n> index 1a44c811aa..6b72f37c29 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \\\n> \n> ETAGS_TARGET = TAGS\n> \n> +FUZZ_OBJS += fuzz-commit-graph.o\n> FUZZ_OBJS += fuzz-pack-headers.o\n> FUZZ_OBJS += fuzz-pack-idx.o\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index 40c855f185..07dd410f3c 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -84,16 +84,10 @@ static int commit_graph_compatible(struct repository *r)\n> struct commit_graph *load_commit_graph_one(const char *graph_file)\n> {\n> \tvoid *graph_map;\n> -\tconst unsigned char *data, *chunk_lookup;\n> \tsize_t graph_size;\n> \tstruct stat st;\n> -\tuint32_t i;\n> -\tstruct commit_graph *graph;\n> +\tstruct commit_graph *ret;\n> \tint fd = git_open(graph_file);\n> -\tuint64_t last_chunk_offset;\n> -\tuint32_t last_chunk_id;\n> -\tuint32_t graph_signature;\n> -\tunsigned char graph_version, hash_version;\n> \n> \tif (fd < 0)\n> \t\treturn NULL;\n> @@ -108,27 +102,55 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n> \t\tdie(_(\"graph file %s is too small\"), graph_file);\n> \t}\n> \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\tret = parse_commit_graph(graph_map, fd, graph_size);\n> +\n> +\tif (!ret) {\n> +\t\tmunmap(graph_map, graph_size);\n> +\t\tclose(fd);\n> +\t\texit(1);\n> +\t}\n> +\n> +\treturn ret;\n> +}\n\nLooks like a reasonable splitting of a helper function with a clean\ninterface. I like it.\n\nThanks.\n\n\n> +struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> +\t\t\t\t\tsize_t graph_size)\n> +{\n> +\tconst unsigned char *data, *chunk_lookup;\n> +\tuint32_t i;\n> +\tstruct commit_graph *graph;\n> +\tuint64_t last_chunk_offset;\n> +\tuint32_t last_chunk_id;\n> +\tuint32_t graph_signature;\n> +\tunsigned char graph_version, hash_version;\n> +\n> +\tif (!graph_map)\n> +\t\treturn NULL;\n> +\n> +\tif (graph_size < GRAPH_MIN_SIZE)\n> +\t\treturn NULL;\n> +\n> \tdata = (const unsigned char *)graph_map;\n> \n> \tgraph_signature = get_be32(data);\n> \tif (graph_signature != GRAPH_SIGNATURE) {\n> \t\terror(_(\"graph signature %X does not match signature %X\"),\n> \t\t graph_signature, GRAPH_SIGNATURE);\n> -\t\tgoto cleanup_fail;\n> +\t\treturn NULL;\n> \t}\n> \n> \tgraph_version = *(unsigned char*)(data + 4);\n> \tif (graph_version != GRAPH_VERSION) {\n> \t\terror(_(\"graph version %X does not match version %X\"),\n> \t\t graph_version, GRAPH_VERSION);\n> -\t\tgoto cleanup_fail;\n> +\t\treturn NULL;\n> \t}\n> \n> \thash_version = *(unsigned char*)(data + 5);\n> \tif (hash_version != GRAPH_OID_VERSION) {\n> \t\terror(_(\"hash version %X does not match version %X\"),\n> \t\t hash_version, GRAPH_OID_VERSION);\n> -\t\tgoto cleanup_fail;\n> +\t\treturn NULL;\n> \t}\n> \n> \tgraph = alloc_commit_graph();\n> @@ -152,7 +174,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n> \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\n> \t\t\terror(_(\"improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n> \t\t\t (uint32_t)chunk_offset);\n> -\t\t\tgoto cleanup_fail;\n> +\t\t\tfree(graph);\n> +\t\t\treturn NULL;\n> \t\t}\n> \n> \t\tswitch (chunk_id) {\n> @@ -187,7 +210,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n> \n> \t\tif (chunk_repeated) {\n> \t\t\terror(_(\"chunk id %08x appears multiple times\"), chunk_id);\n> -\t\t\tgoto cleanup_fail;\n> +\t\t\tfree(graph);\n> +\t\t\treturn NULL;\n> \t\t}\n> \n> \t\tif (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)\n> @@ -201,11 +225,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n> \t}\n> \n> \treturn graph;\n> -\n> -cleanup_fail:\n> -\tmunmap(graph_map, graph_size);\n> -\tclose(fd);\n> -\texit(1);\n> }\n> \n> static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n> diff --git a/commit-graph.h b/commit-graph.h\n> index 9db40b4d3a..813e7c19f1 100644\n> --- a/commit-graph.h\n> +++ b/commit-graph.h\n> @@ -54,6 +54,9 @@ struct commit_graph {\n> \n> struct commit_graph *load_commit_graph_one(const char *graph_file);\n> \n> +struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> +\t\t\t\t\tsize_t graph_size);\n> +\n> /*\n> * Return 1 if and only if the repository has a commit-graph\n> * file and generation numbers are computed in that file.\n> diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\n> new file mode 100644\n> index 0000000000..cf790c9d04\n> --- /dev/null\n> +++ b/fuzz-commit-graph.c\n> @@ -0,0 +1,16 @@\n> +#include \"commit-graph.h\"\n> +\n> +struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> +\t\t\t\t\tsize_t graph_size);\n> +\n> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n> +\n> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n> +{\n> +\tstruct commit_graph *g;\n> +\n> +\tg = parse_commit_graph((void *)data, -1, size);\n> +\tfree(g);\n> +\n> +\treturn 0;\n> +}\n"},{"id":"366764","messageId":"xmqqimypr6yx.fsf@gitster-ct.c.googlers.com","threadId":"49952","inReplyTo":"350ea5f7c97aa4166eea56aa57b66ddc9c53de4a.1547582104.git.steadmon@google.com","subject":"Re: [PATCH v5 3/3] Makefile: correct example fuzz build","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-15T20:39:18Z","receivedAt":"2019-01-15T20:39:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Signed-off-by: Josh Steadmon \n> ---\n> Makefile | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 6b72f37c29..bbcfc2bc9f 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3104,7 +3104,7 @@ cover_db_html: cover_db\n> # An example command to build against libFuzzer from LLVM 4.0.0:\n> #\n> # make CC=clang CXX=clang++ \\\n> -# FUZZ_CXXFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n> +# CFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n> # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \\\n> # fuzz-all\n> #\n\nI know this appeared in v2 of the series, but I cannot quite read\nthe reasoning/justification behind this change. After this hunk\nthere is\n\n FUZZ_CXXFLAGS ?= $(CFLAGS)\n\nso if you do not give CFLAGS but give FUZZ_CXXFLAGS, like the\nsample, shouldn't it have worked just fine? IOW \"correct\" in the\ntitle is a bit too terse as an explanation for this change.\n\n"},{"id":"366769","messageId":"20190115215945.GB35794@google.com","threadId":"49952","inReplyTo":"xmqqimypr6yx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5 3/3] Makefile: correct example fuzz build","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-15T21:59:45Z","receivedAt":"2019-01-15T21:59:56Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2019.01.15 12:39, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > Signed-off-by: Josh Steadmon \n> > ---\n> > Makefile | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/Makefile b/Makefile\n> > index 6b72f37c29..bbcfc2bc9f 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -3104,7 +3104,7 @@ cover_db_html: cover_db\n> > # An example command to build against libFuzzer from LLVM 4.0.0:\n> > #\n> > # make CC=clang CXX=clang++ \\\n> > -# FUZZ_CXXFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n> > +# CFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n> > # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \\\n> > # fuzz-all\n> > #\n> \n> I know this appeared in v2 of the series, but I cannot quite read\n> the reasoning/justification behind this change. After this hunk\n> there is\n> \n> FUZZ_CXXFLAGS ?= $(CFLAGS)\n> \n> so if you do not give CFLAGS but give FUZZ_CXXFLAGS, like the\n> sample, shouldn't it have worked just fine? IOW \"correct\" in the\n> title is a bit too terse as an explanation for this change.\n\nSorry for being too vague. The problem with only including FUZZ_CXXFLAGS\nis that all the .c files need to be compiled with coverage tracking\nenabled, not just the fuzzer itself.\n\nThe motivation for splitting CFLAGS and FUZZ_CXXFLAGS in the first place\nwas to enable OSS-Fuzz (and others) to include C++-specific flags\nwithout causing a ton of compiler warnings when those are applied to the\n.c files. So OSS-Fuzz sets both CFLAGS and FUZZ_CXXFLAGS when building.\n\nWe could fix the comment by setting both CFLAGS and FUZZ_CXXFLAGS, but\nsince we're not including any C++-specific flags there's no reason to\nset both.\n\nI'll fix the commit message in v6.\n"},{"id":"366772","messageId":"cover.1547590928.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1544048946.git.steadmon@google.com","subject":"[PATCH v6 0/3] Add commit-graph fuzzer and fix buffer overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-15T22:25:49Z","receivedAt":"2019-01-15T22:25:58Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Add a new fuzz test for the commit graph and fix a buffer read-overflow\nthat it discovered. Additionally, fix the Makefile instructions for\nbuilding fuzzers.\n\nChanges since V5:\n * Conform to commit message standards for the 1st patch in the series.\n * Clarify commit message for the 3rd patch in the series.\n\nChanges since V4:\n * Ensure that corrupt_graph_and_verify() in t5318 changes to the\n proper directory before accessing any files.\n\nChanges since V3:\n * Improve portability of the new test functionality.\n * Fix broken &&-chains in tests.\n\nChanges since V2:\n * Avoid pointer arithmetic overflow when checking the graph's chunk\n count.\n * Merge the corrupt_graph_and_verify and\n corrupt_and_zero_graph_then_verify test functions.\n\nJosh Steadmon (3):\n commit-graph, fuzz: Add fuzzer for commit-graph\n commit-graph: fix buffer read-overflow\n Makefile: correct example fuzz build\n\n .gitignore | 1 +\n Makefile | 3 +-\n commit-graph.c | 67 +++++++++++++++++++++++++++++------------\n commit-graph.h | 3 ++\n fuzz-commit-graph.c | 16 ++++++++++\n t/t5318-commit-graph.sh | 16 ++++++++--\n 6 files changed, 83 insertions(+), 23 deletions(-)\n create mode 100644 fuzz-commit-graph.c\n\nRange-diff against v5:\n1: 0b57ecbe1b ! 1: c4ec3fc3fc commit-graph, fuzz: Add fuzzer for commit-graph\n @@ -2,11 +2,11 @@\n \n commit-graph, fuzz: Add fuzzer for commit-graph\n \n - Breaks load_commit_graph_one() into a new function,\n - parse_commit_graph(). The latter function operates on arbitrary buffers,\n - which makes it suitable as a fuzzing target. Since parse_commit_graph()\n - is only called by load_commit_graph_one() (and the fuzzer described\n - below), we omit error messages that would be duplicated by the caller.\n + Break load_commit_graph_one() into a new function, parse_commit_graph().\n + The latter function operates on arbitrary buffers, which makes it\n + suitable as a fuzzing target. Since parse_commit_graph() is only called\n + by load_commit_graph_one() (and the fuzzer described below), we omit\n + error messages that would be duplicated by the caller.\n \n Adds fuzz-commit-graph.c, which provides a fuzzing entry point\n compatible with libFuzzer (and possibly other fuzzing engines).\n2: a3b5d33c4b = 2: d7b137650f commit-graph: fix buffer read-overflow\n3: 350ea5f7c9 ! 3: c06e0667fa Makefile: correct example fuzz build\n @@ -2,6 +2,15 @@\n \n Makefile: correct example fuzz build\n \n + The comment explaining how to build the fuzzers was broken in\n + 927c77e7d4d (\"Makefile: use FUZZ_CXXFLAGS for linking fuzzers\",\n + 2018-11-14).\n +\n + When building fuzzers, all .c files must be compiled with coverage\n + tracing enabled. This is not possible when using only FUZZ_CXXFLAGS, as\n + that flag is only applied to the fuzzers themselves. Switching back to\n + CFLAGS fixes the issue.\n +\n \n diff --git a/Makefile b/Makefile\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366773","messageId":"c4ec3fc3fc504c1d639845ea409108bb8f0b6d68.1547590928.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1547590928.git.steadmon@google.com","subject":"[PATCH v6 1/3] commit-graph, fuzz: Add fuzzer for commit-graph","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-15T22:25:50Z","receivedAt":"2019-01-15T22:26:01Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Break load_commit_graph_one() into a new function, parse_commit_graph().\nThe latter function operates on arbitrary buffers, which makes it\nsuitable as a fuzzing target. Since parse_commit_graph() is only called\nby load_commit_graph_one() (and the fuzzer described below), we omit\nerror messages that would be duplicated by the caller.\n\nAdds fuzz-commit-graph.c, which provides a fuzzing entry point\ncompatible with libFuzzer (and possibly other fuzzing engines).\n\nSigned-off-by: Josh Steadmon \n---\n .gitignore | 1 +\n Makefile | 1 +\n commit-graph.c | 53 ++++++++++++++++++++++++++++++---------------\n commit-graph.h | 3 +++\n fuzz-commit-graph.c | 16 ++++++++++++++\n 5 files changed, 57 insertions(+), 17 deletions(-)\n create mode 100644 fuzz-commit-graph.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 0d77ea5894..8bcf153ed9 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -1,3 +1,4 @@\n+/fuzz-commit-graph\n /fuzz_corpora\n /fuzz-pack-headers\n /fuzz-pack-idx\ndiff --git a/Makefile b/Makefile\nindex 1a44c811aa..6b72f37c29 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \\\n \n ETAGS_TARGET = TAGS\n \n+FUZZ_OBJS += fuzz-commit-graph.o\n FUZZ_OBJS += fuzz-pack-headers.o\n FUZZ_OBJS += fuzz-pack-idx.o\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 40c855f185..07dd410f3c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -84,16 +84,10 @@ static int commit_graph_compatible(struct repository *r)\n struct commit_graph *load_commit_graph_one(const char *graph_file)\n {\n \tvoid *graph_map;\n-\tconst unsigned char *data, *chunk_lookup;\n \tsize_t graph_size;\n \tstruct stat st;\n-\tuint32_t i;\n-\tstruct commit_graph *graph;\n+\tstruct commit_graph *ret;\n \tint fd = git_open(graph_file);\n-\tuint64_t last_chunk_offset;\n-\tuint32_t last_chunk_id;\n-\tuint32_t graph_signature;\n-\tunsigned char graph_version, hash_version;\n \n \tif (fd < 0)\n \t\treturn NULL;\n@@ -108,27 +102,55 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t\tdie(_(\"graph file %s is too small\"), graph_file);\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tret = parse_commit_graph(graph_map, fd, graph_size);\n+\n+\tif (!ret) {\n+\t\tmunmap(graph_map, graph_size);\n+\t\tclose(fd);\n+\t\texit(1);\n+\t}\n+\n+\treturn ret;\n+}\n+\n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size)\n+{\n+\tconst unsigned char *data, *chunk_lookup;\n+\tuint32_t i;\n+\tstruct commit_graph *graph;\n+\tuint64_t last_chunk_offset;\n+\tuint32_t last_chunk_id;\n+\tuint32_t graph_signature;\n+\tunsigned char graph_version, hash_version;\n+\n+\tif (!graph_map)\n+\t\treturn NULL;\n+\n+\tif (graph_size < GRAPH_MIN_SIZE)\n+\t\treturn NULL;\n+\n \tdata = (const unsigned char *)graph_map;\n \n \tgraph_signature = get_be32(data);\n \tif (graph_signature != GRAPH_SIGNATURE) {\n \t\terror(_(\"graph signature %X does not match signature %X\"),\n \t\t graph_signature, GRAPH_SIGNATURE);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \tgraph_version = *(unsigned char*)(data + 4);\n \tif (graph_version != GRAPH_VERSION) {\n \t\terror(_(\"graph version %X does not match version %X\"),\n \t\t graph_version, GRAPH_VERSION);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \thash_version = *(unsigned char*)(data + 5);\n \tif (hash_version != GRAPH_OID_VERSION) {\n \t\terror(_(\"hash version %X does not match version %X\"),\n \t\t hash_version, GRAPH_OID_VERSION);\n-\t\tgoto cleanup_fail;\n+\t\treturn NULL;\n \t}\n \n \tgraph = alloc_commit_graph();\n@@ -152,7 +174,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\n \t\t\terror(_(\"improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n \t\t\t (uint32_t)chunk_offset);\n-\t\t\tgoto cleanup_fail;\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n \t\t}\n \n \t\tswitch (chunk_id) {\n@@ -187,7 +210,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \n \t\tif (chunk_repeated) {\n \t\t\terror(_(\"chunk id %08x appears multiple times\"), chunk_id);\n-\t\t\tgoto cleanup_fail;\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n \t\t}\n \n \t\tif (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)\n@@ -201,11 +225,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \t}\n \n \treturn graph;\n-\n-cleanup_fail:\n-\tmunmap(graph_map, graph_size);\n-\tclose(fd);\n-\texit(1);\n }\n \n static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 9db40b4d3a..813e7c19f1 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -54,6 +54,9 @@ struct commit_graph {\n \n struct commit_graph *load_commit_graph_one(const char *graph_file);\n \n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size);\n+\n /*\n * Return 1 if and only if the repository has a commit-graph\n * file and generation numbers are computed in that file.\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nnew file mode 100644\nindex 0000000000..cf790c9d04\n--- /dev/null\n+++ b/fuzz-commit-graph.c\n@@ -0,0 +1,16 @@\n+#include \"commit-graph.h\"\n+\n+struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n+\t\t\t\t\tsize_t graph_size);\n+\n+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n+\n+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n+{\n+\tstruct commit_graph *g;\n+\n+\tg = parse_commit_graph((void *)data, -1, size);\n+\tfree(g);\n+\n+\treturn 0;\n+}\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366774","messageId":"d7b137650f9203148c1fb998a3dd4af7a2105e62.1547590928.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1547590928.git.steadmon@google.com","subject":"[PATCH v6 2/3] commit-graph: fix buffer read-overflow","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-15T22:25:51Z","receivedAt":"2019-01-15T22:26:04Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"fuzz-commit-graph identified a case where Git will read past the end of\na buffer containing a commit graph if the graph's header has an\nincorrect chunk count. A simple bounds check in parse_commit_graph()\nprevents this.\n\nSigned-off-by: Josh Steadmon \n---\n commit-graph.c | 14 ++++++++++++--\n t/t5318-commit-graph.sh | 16 +++++++++++++---\n 2 files changed, 25 insertions(+), 5 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 07dd410f3c..836d65a1d3 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \tlast_chunk_offset = 8;\n \tchunk_lookup = data + 8;\n \tfor (i = 0; i < graph->num_chunks; i++) {\n-\t\tuint32_t chunk_id = get_be32(chunk_lookup + 0);\n-\t\tuint64_t chunk_offset = get_be64(chunk_lookup + 4);\n+\t\tuint32_t chunk_id;\n+\t\tuint64_t chunk_offset;\n \t\tint chunk_repeated = 0;\n \n+\t\tif (data + graph_size - chunk_lookup <\n+\t\t GRAPH_CHUNKLOOKUP_WIDTH) {\n+\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n+\t\t\tfree(graph);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\tchunk_id = get_be32(chunk_lookup + 0);\n+\t\tchunk_offset = get_be64(chunk_lookup + 4);\n+\n \t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n \n \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 5fe21db99f..694f26079f 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -366,9 +366,10 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n \n-# usage: corrupt_graph_and_verify \n+# usage: corrupt_graph_and_verify []\n # Manipulates the commit-graph file at the position\n-# by inserting the data, then runs 'git commit-graph verify'\n+# by inserting the data, optionally zeroing the file\n+# starting at , then runs 'git commit-graph verify'\n # and places the output in the file 'err'. Test 'err' for\n # the given string.\n corrupt_graph_and_verify() {\n@@ -376,11 +377,15 @@ corrupt_graph_and_verify() {\n \tdata=\"${2:-\\0}\"\n \tgrepstr=$3\n \tcd \"$TRASH_DIRECTORY/full\" &&\n+\torig_size=$(wc -c < $objdir/info/commit-graph) &&\n+\tzero_pos=${4:-${orig_size}} &&\n \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n \tcp $objdir/info/commit-graph commit-graph-backup &&\n \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n+\tdd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=0 &&\n+\tdd if=/dev/zero of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=$(($orig_size - $zero_pos)) &&\n \ttest_must_fail git commit-graph verify 2>test_err &&\n-\tgrep -v \"^+\" test_err >err\n+\tgrep -v \"^+\" test_err >err &&\n \ttest_i18ngrep \"$grepstr\" err\n }\n \n@@ -484,6 +489,11 @@ test_expect_success 'detect invalid checksum hash' '\n \t\t\"incorrect checksum\"\n '\n \n+test_expect_success 'detect incorrect chunk count' '\n+\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\377\" \\\n+\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n+'\n+\n test_expect_success 'git fsck (checks commit-graph)' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit fsck &&\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366775","messageId":"c06e0667fa332edaea3ddc87af5b0481e2544c2e.1547590928.git.steadmon@google.com","threadId":"49952","inReplyTo":"cover.1547590928.git.steadmon@google.com","subject":"[PATCH v6 3/3] Makefile: correct example fuzz build","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-15T22:25:52Z","receivedAt":"2019-01-15T22:26:07Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"The comment explaining how to build the fuzzers was broken in\n927c77e7d4d (\"Makefile: use FUZZ_CXXFLAGS for linking fuzzers\",\n2018-11-14).\n\nWhen building fuzzers, all .c files must be compiled with coverage\ntracing enabled. This is not possible when using only FUZZ_CXXFLAGS, as\nthat flag is only applied to the fuzzers themselves. Switching back to\nCFLAGS fixes the issue.\n\nSigned-off-by: Josh Steadmon \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 6b72f37c29..bbcfc2bc9f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3104,7 +3104,7 @@ cover_db_html: cover_db\n # An example command to build against libFuzzer from LLVM 4.0.0:\n #\n # make CC=clang CXX=clang++ \\\n-# FUZZ_CXXFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n+# CFLAGS=\"-fsanitize-coverage=trace-pc-guard -fsanitize=address\" \\\n # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \\\n # fuzz-all\n #\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366777","messageId":"xmqqzhs1pn35.fsf@gitster-ct.c.googlers.com","threadId":"49952","inReplyTo":"20190115215945.GB35794@google.com","subject":"Re: [PATCH v5 3/3] Makefile: correct example fuzz build","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-15T22:34:06Z","receivedAt":"2019-01-15T22:34:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> Sorry for being too vague. The problem with only including FUZZ_CXXFLAGS\n> is that all the .c files need to be compiled with coverage tracking\n> enabled, not just the fuzzer itself.\n\nOK.\n\n> We could fix the comment by setting both CFLAGS and FUZZ_CXXFLAGS, but\n> since we're not including any C++-specific flags there's no reason to\n> set both.\n\nYes.\n"},{"id":"369721","messageId":"87d0nm5x1d.fsf@evledraar.gmail.com","threadId":"49952","inReplyTo":"d7b137650f9203148c1fb998a3dd4af7a2105e62.1547590928.git.steadmon@google.com","subject":"Re: [PATCH v6 2/3] commit-graph: fix buffer read-overflow","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-20T14:55:58Z","receivedAt":"2019-02-20T14:56:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 15 2019, Josh Steadmon wrote:\n\n> fuzz-commit-graph identified a case where Git will read past the end of\n> a buffer containing a commit graph if the graph's header has an\n> incorrect chunk count. A simple bounds check in parse_commit_graph()\n> prevents this.\n\nThis has a 2.21 regression where the test fails on NetBSD:\nhttps://gitlab.com/git-vcs/git-ci/-/jobs/164224275\n\n> Signed-off-by: Josh Steadmon \n> ---\n> commit-graph.c | 14 ++++++++++++--\n> t/t5318-commit-graph.sh | 16 +++++++++++++---\n> 2 files changed, 25 insertions(+), 5 deletions(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 07dd410f3c..836d65a1d3 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> \tlast_chunk_offset = 8;\n> \tchunk_lookup = data + 8;\n> \tfor (i = 0; i < graph->num_chunks; i++) {\n> -\t\tuint32_t chunk_id = get_be32(chunk_lookup + 0);\n> -\t\tuint64_t chunk_offset = get_be64(chunk_lookup + 4);\n> +\t\tuint32_t chunk_id;\n> +\t\tuint64_t chunk_offset;\n> \t\tint chunk_repeated = 0;\n>\n> +\t\tif (data + graph_size - chunk_lookup <\n> +\t\t GRAPH_CHUNKLOOKUP_WIDTH) {\n> +\t\t\terror(_(\"chunk lookup table entry missing; graph file may be incomplete\"));\n> +\t\t\tfree(graph);\n> +\t\t\treturn NULL;\n> +\t\t}\n> +\n> +\t\tchunk_id = get_be32(chunk_lookup + 0);\n> +\t\tchunk_offset = get_be64(chunk_lookup + 4);\n> +\n> \t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n>\n> \t\tif (chunk_offset > graph_size - GIT_MAX_RAWSZ) {\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index 5fe21db99f..694f26079f 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -366,9 +366,10 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \\\n> GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))\n> GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))\n>\n> -# usage: corrupt_graph_and_verify \n> +# usage: corrupt_graph_and_verify []\n> # Manipulates the commit-graph file at the position\n> -# by inserting the data, then runs 'git commit-graph verify'\n> +# by inserting the data, optionally zeroing the file\n> +# starting at , then runs 'git commit-graph verify'\n> # and places the output in the file 'err'. Test 'err' for\n> # the given string.\n> corrupt_graph_and_verify() {\n> @@ -376,11 +377,15 @@ corrupt_graph_and_verify() {\n> \tdata=\"${2:-\\0}\"\n> \tgrepstr=$3\n> \tcd \"$TRASH_DIRECTORY/full\" &&\n> +\torig_size=$(wc -c < $objdir/info/commit-graph) &&\n> +\tzero_pos=${4:-${orig_size}} &&\n> \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n> \tcp $objdir/info/commit-graph commit-graph-backup &&\n> \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n> +\tdd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=0 &&\n> +\tdd if=/dev/zero of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=$(($orig_size - $zero_pos)) &&\n\nIn the limited time I had to dig it starts failing at test 46, when\ncount=0 is given. dd on NetBSD exits with 127 when given count=0 it\nseems.\n\n> \ttest_must_fail git commit-graph verify 2>test_err &&\n> -\tgrep -v \"^+\" test_err >err\n> +\tgrep -v \"^+\" test_err >err &&\n> \ttest_i18ngrep \"$grepstr\" err\n> }\n>\n> @@ -484,6 +489,11 @@ test_expect_success 'detect invalid checksum hash' '\n> \t\t\"incorrect checksum\"\n> '\n>\n> +test_expect_success 'detect incorrect chunk count' '\n> +\tcorrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT \"\\377\" \\\n> +\t\t\"chunk lookup table entry missing\" $GRAPH_CHUNK_LOOKUP_OFFSET\n> +'\n> +\n\nHacking around the above (e.g. \"dd ... || :\") makes all the failing\ntests pass except this new one, which I didn't dig into.\n"},{"id":"369726","messageId":"20190220165048.GR1622@szeder.dev","threadId":"49952","inReplyTo":"87d0nm5x1d.fsf@evledraar.gmail.com","subject":"Re: [PATCH v6 2/3] commit-graph: fix buffer read-overflow","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-20T16:50:48Z","receivedAt":"2019-02-20T16:50:55Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Feb 20, 2019 at 03:55:58PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > @@ -376,11 +377,15 @@ corrupt_graph_and_verify() {\n> > \tdata=\"${2:-\\0}\"\n> > \tgrepstr=$3\n> > \tcd \"$TRASH_DIRECTORY/full\" &&\n> > +\torig_size=$(wc -c < $objdir/info/commit-graph) &&\n> > +\tzero_pos=${4:-${orig_size}} &&\n> > \ttest_when_finished mv commit-graph-backup $objdir/info/commit-graph &&\n> > \tcp $objdir/info/commit-graph commit-graph-backup &&\n> > \tprintf \"$data\" | dd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$pos\" conv=notrunc &&\n> > +\tdd of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=0 &&\n> > +\tdd if=/dev/zero of=\"$objdir/info/commit-graph\" bs=1 seek=\"$zero_pos\" count=$(($orig_size - $zero_pos)) &&\n> \n> In the limited time I had to dig it starts failing at test 46, when\n> count=0 is given. dd on NetBSD exits with 127 when given count=0 it\n> seems.\n\nSo the first 'dd' is supposed to truncate the commit-graph file at\n$zero_pos. I don't think we need 'count=0' for that: in the absence\nof the 'if=...' operand, 'dd' reads from standard input, which is\nredirected from /dev/null in our test scripts, i.e. there is nothing\nto read, and, consequently, there is nothing to write, either.\n\nThough not strictly necessary, I would feel more comfortable if\n'if=/dev/null' would be explicitly specified, and even more so with a\n\"# truncate at $zero_pos\" comment above that command.\n\nAs to the second 'dd', I think we should not run it at all when count\nwould be zero, i.e. when $orig_size = $zero_pos, because in\ncombination with 'if=/dev/zero' it's asking for trouble. According to\nPOSIX [1]:\n\n count=n\n Copy only n input blocks. If n is zero, it is unspecified\n whether no blocks or all blocks are copied.\n\nImagine a 'dd' that implements the second option: there are infinite\nblocks in /dev/zero to copy! OTOH, if an implementation chooses the\nfirst option (e.g. the usual Linux 'dd' from coreutils), then both of\nthese 'dd' invocations will leave the commit-graph file as-is, so it\ndoesn't matter whether we run them or not.\n\n\n[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/dd.html\n\n"}]}