Add commit-graph fuzzer and fix buffer overflow
cover Add commit-graph fuzzer and fix buffer overflow
Add a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered.
Josh Steadmon (2):
commit-graph, fuzz: Add fuzzer for commit-graph
commit-graph: fix buffer read-overflow
.gitignore | 1 + Makefile | 1 + commit-graph.c | 76 +++++++++++++++++++++++++++++++++------------ fuzz-commit-graph.c | 18 +++++++++++ 4 files changed, 77 insertions(+), 19 deletions(-) create mode 100644 fuzz-commit-graph.c
Signature · 1 line
2.20.0.rc2.403.gdbc3b29805-goog
PATCH 1/2 commit-graph, fuzz: Add fuzzer for commit-graph
Breaks load_commit_graph_one() into a new function, parse_commit_graph(). The latter function operates on arbitrary buffers, which makes it suitable as a fuzzing target.
Adds fuzz-commit-graph.c, which provides a fuzzing entry point compatible with libFuzzer (and possibly other fuzzing engines).
--- .gitignore | 1 + Makefile | 1 + commit-graph.c | 63 +++++++++++++++++++++++++++++++++------------ fuzz-commit-graph.c | 18 +++++++++++++ 4 files changed, 66 insertions(+), 17 deletions(-) create mode 100644 fuzz-commit-graph.c
.gitignore +1 -0
@@ -1,3 +1,4 @@ +/fuzz-commit-graph /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx
Makefile +1 -0
@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \ ETAGS_TARGET = TAGS +FUZZ_OBJS += fuzz-commit-graph.o FUZZ_OBJS += fuzz-pack-headers.o FUZZ_OBJS += fuzz-pack-idx.o
commit-graph.c +46 -17
120 lines not on this page · raw message
fuzz-commit-graph.c new file +18 -0
@@ -0,0 +1,18 @@ +#include "object-store.h" +#include "commit-graph.h" + +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size) +{ +struct commit_graph *g; + + g =parse_commit_graph ((void *) data,-1 , size); +if (g) +free (g); + +return 0 ; +}
Signature · 1 line
2.20.0.rc2.403.gdbc3b29805-goog
Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph
Quoted 71 lines · Josh Steadmon
Breaks
load_commit_graph_one()into a new function,parse_commit_graph(). The latter function operates on arbitrary buffers, which makes it suitable as a fuzzing target.Adds
fuzz-commit-graph.c, which provides a fuzzing entry point compatible with libFuzzer (and possibly other fuzzing engines).Signed-off-by Josh Steadmon <[email protected]>--- .gitignore | 1 + Makefile | 1 + commit-graph.c | 63 +++++++++++++++++++++++++++++++++------------ fuzz-commit-graph.c | 18 +++++++++++++ 4 files changed, 66 insertions(+), 17 deletions(-) create mode 100644 fuzz-commit-graph.c.gitignore +1
-0@@ -1,3 +1,4 @@ +/fuzz-commit-graph /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idxMakefile +1
-0@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \ ETAGS_TARGET = TAGS +FUZZ_OBJS += fuzz-commit-graph.o FUZZ_OBJS += fuzz-pack-headers.o FUZZ_OBJS += fuzz-pack-idx.ocommit-graph.c +5
-7@@ -46,6 +46,10 @@#define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \ + GRAPH_FANOUT_SIZE + GRAPH_OID_LEN) +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); + +char *get_commit_graph_filename (const char *obj_dir) {return xstrfmt ("%s/info/commit-graph" , obj_dir);@@ -84,16 +88,10 @@ static int commit_graph_compatible(struct repository *r)struct commit_graph *load_commit_graph_one (const char *graph_file) {void *graph_map; -const unsigned char *data, *chunk_lookup;size_t graph_size;struct stat st; -uint32_t i; -struct commit_graph *graph; +struct commit_graph *ret;int fd =git_open (graph_file); -uint64_t last_chunk_offset; -uint32_t last_chunk_id; -uint32_t graph_signature; -unsigned char graph_version, hash_version;if (fd <0 )return NULL ;
commit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 111
Quoted 6 lines
commit-graph.c +3
-0@@ -108,27 +106,61 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)die (_ ("graph file %s is too small" ), graph_file); } graph_map =xmmap (NULL , graph_size,PROT_READ ,MAP_PRIVATE , fd,0 ); + ret =parse_commit_graph (graph_map, fd, graph_size); + +if (ret ==NULL ) {
Code in git usually uses just !ret.
commit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 114
Quoted 3 lines
commit-graph.c +3
-0+munmap (graph_map, graph_size); +close (fd); +exit (1 );
Ouch, exit(1) from load_commit_graph_one()? Can't we return NULL here instead? Nasty to exit from a library routine, but then I see later...
commit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 238
Quoted 10 lines
commit-graph.c +0
-5@@ -201,11 +235,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file) }return graph; - -cleanup_fail: -munmap (graph_map, graph_size); -close (fd); -exit (1 ); }
... ah, I see this is where you got the exit(1) from. So it was there already.
Quoted 15 lines
commit-graph.c +0
-0static void prepare_commit_graph_one (struct repository *r,const char *obj_dir)fuzz-commit-graph.c new file +8
-0@@ -0,0 +1,18 @@ +#include "object-store.h" +#include "commit-graph.h" + +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size); +
fuzz-commit-graph.c line 18
Quoted 10 lines
fuzz-commit-graph.c new file +10
-0+int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size) +{ +struct commit_graph *g; + + g =parse_commit_graph ((void *) data,-1 , size); +if (g) +free (g); + +return 0 ; +}
I hadn't looked at this before, but see your 5e47215080 ("fuzz: add basic fuzz testing target.", 2018-10-12) for some prior art.
There's instructions there for a very long "make" invocation. Would be nice if this were friendlier and we could just do "make test-fuzz" or something...
Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph
Quoted 71 lines · Ævar Arnfjörð Bjarmason
Quoted 68 lines · Josh Steadmon
Breaks
load_commit_graph_one()into a new function,parse_commit_graph(). The latter function operates on arbitrary buffers, which makes it suitable as a fuzzing target.Adds
fuzz-commit-graph.c, which provides a fuzzing entry point compatible with libFuzzer (and possibly other fuzzing engines).Signed-off-by Josh Steadmon <[email protected]>--- .gitignore | 1 + Makefile | 1 + commit-graph.c | 63 +++++++++++++++++++++++++++++++++------------ fuzz-commit-graph.c | 18 +++++++++++++ 4 files changed, 66 insertions(+), 17 deletions(-) create mode 100644 fuzz-commit-graph.c.gitignore +1
-0@@ -1,3 +1,4 @@ +/fuzz-commit-graph /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idxMakefile +1
-0@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \ ETAGS_TARGET = TAGS +FUZZ_OBJS += fuzz-commit-graph.o FUZZ_OBJS += fuzz-pack-headers.o FUZZ_OBJS += fuzz-pack-idx.ocommit-graph.c +5
-7@@ -46,6 +46,10 @@#define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \ + GRAPH_FANOUT_SIZE + GRAPH_OID_LEN) +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); + +char *get_commit_graph_filename (const char *obj_dir) {return xstrfmt ("%s/info/commit-graph" , obj_dir);@@ -84,16 +88,10 @@ static int commit_graph_compatible(struct repository *r)struct commit_graph *load_commit_graph_one (const char *graph_file) {void *graph_map; -const unsigned char *data, *chunk_lookup;size_t graph_size;struct stat st; -uint32_t i; -struct commit_graph *graph; +struct commit_graph *ret;int fd =git_open (graph_file); -uint64_t last_chunk_offset; -uint32_t last_chunk_id; -uint32_t graph_signature; -unsigned char graph_version, hash_version;
Quoted 11 lines
commit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 111
Quoted 9 lines
commit-graph.c +3
-0if (fd <0 )return NULL ;@@ -108,27 +106,61 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)die (_ ("graph file %s is too small" ), graph_file); } graph_map =xmmap (NULL , graph_size,PROT_READ ,MAP_PRIVATE , fd,0 ); + ret =parse_commit_graph (graph_map, fd, graph_size); + +if (ret ==NULL ) {Code in git usually uses just !ret.
Will fix in V2, thanks.
Quoted 50 lines
commit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 114
Quoted 3 lines
commit-graph.c +3
-0+munmap (graph_map, graph_size); +close (fd); +exit (1 );Ouch, exit(1) from
load_commit_graph_one()? Can't we return NULL here instead? Nasty to exit from a library routine, but then I see later...commit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 238
Quoted 10 lines
commit-graph.c +0
-5@@ -201,11 +235,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file) }return graph; - -cleanup_fail: -munmap (graph_map, graph_size); -close (fd); -exit (1 ); }... ah, I see this is where you got the exit(1) from. So it was there already.
Quoted 15 lines
commit-graph.c +0
-0static void prepare_commit_graph_one (struct repository *r,const char *obj_dir)fuzz-commit-graph.c new file +8
-0@@ -0,0 +1,18 @@ +#include "object-store.h" +#include "commit-graph.h" + +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size); +fuzz-commit-graph.c line 18
Quoted 10 lines
fuzz-commit-graph.c new file +10
-0+int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size) +{ +struct commit_graph *g; + + g =parse_commit_graph ((void *) data,-1 , size); +if (g) +free (g); + +return 0 ; +}I hadn't looked at this before, but see your
5e47215080("fuzz: add basic fuzz testing target.", 2018-10-12) for some prior art.
Quoted 3 lines
There's instructions there for a very long "make" invocation. Would be nice if this were friendlier and we could just do "make test-fuzz" or something...
Yeah, the problem is that there are too many combinations of fuzzing engine, sanitizer, and compiler to make any reasonable default here. Even if you just stick with libFuzzer, address sanitizer, and clang, the flags change radically depending on which version of clang you're using.
Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph
In reply to Josh Steadmoncommit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 109
Quoted 5 lines · Josh Steadmon
commit-graph.c +1
-0@@ -108,27 +106,61 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)die (_ ("graph file %s is too small" ), graph_file); } graph_map =xmmap (NULL , graph_size,PROT_READ ,MAP_PRIVATE , fd,0 ); + ret =parse_commit_graph (graph_map, fd, graph_size);
OK, assuming that the new helper returns NULL from all places in the original that would have jumped to the cleanup-fail label, this would do the same thing (it may not be the right thing to exit, but that is OK for the purpose of this change).
commit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 123
Quoted 13 lines
commit-graph.c +13
-0+if (ret ==NULL ) { +munmap (graph_map, graph_size); +close (fd); +exit (1 ); + } + +return ret; +} + +/* +* This function is intended to be used only from load_commit_graph_one() or in +* fuzz tests. +*/
Hmph, is that necessary to say?
"Right now, it has only these two callers" is sometimes handy for those without good static analysis tools, like "git grep" ;-), but I do not think of a reason why a new caller we'll add in the future, who happens to have the data of the graph file in-core, should not be allowed to call this function.
Quoted 6 lines
commit-graph.c +6
-0+struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size) +{ +const unsigned char *data, *chunk_lookup; +uint32_t i; +struct commit_graph *graph;
commit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 139
Quoted 10 lines
commit-graph.c +10
-0+uint64_t last_chunk_offset; +uint32_t last_chunk_id; +uint32_t graph_signature; +unsigned char graph_version, hash_version; + +/* +* This should already be checked in load_commit_graph_one, but we still +* need a check here for when we're calling parse_commit_graph directly +* from fuzz tests. We can omit the error message in that case. +*/
In the same spirit, I am dubious of the longer-term value of this comment. As an explanation of why this conversion is correct in the proposed log message for this change, it perfectly makes sense, though.
commit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 142
Quoted 3 lines
commit-graph.c +3
-0+if (graph_size <GRAPH_MIN_SIZE ) +return NULL ; +
The load_commit_graph_one() grabbed graph_map out of xmmap() so it is guaranteed to be non-NULL, but we need to check graph_map != NULL when we're calling this directly from the fuzz tests, exactly in the same spirit that we check graph_size above. Besides, these are to make sure future callers won't misuse the API.
commit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 143
Quoted 1 line
commit-graph.c +0
-0data = (const unsigned char *)graph_map;
And the reset of the function is the same as the original modulo jumping to the cleanup_fail label has been replaced with returning NULL.
Looks good.
Quoted 33 lines
...
commit-graph.c +0
-5- -cleanup_fail: -munmap (graph_map, graph_size); -close (fd); -exit (1 ); }static void prepare_commit_graph_one (struct repository *r,const char *obj_dir)fuzz-commit-graph.c new file +18
-0@@ -0,0 +1,18 @@ +#include "object-store.h" +#include "commit-graph.h" + +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size) +{ +struct commit_graph *g; + + g =parse_commit_graph ((void *) data,-1 , size); +if (g) +free (g); + +return 0 ; +}
Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph
Quoted 9 lines · Junio C Hamano
commit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 142
Quoted 3 lines
commit-graph.c +3
-0+if (graph_size <GRAPH_MIN_SIZE ) +return NULL ; +The
load_commit_graph_one()grabbedgraph_mapout ofxmmap()so it is guaranteed to be non-NULL, but we need to checkgraph_map!= NULL when we're calling this directly from the fuzz tests, exactly in the same spirit that we checkgraph_sizeabove. Besides, these are to make sure future callers won't misuse the API.
Insert "Please check graph_map != NULL here, too." before the above paragraph.
Quoted 7 lines
Quoted 1 line
data = (const unsigned char *)graph_map;
And the reset of the function is the same as the original modulo jumping to the
cleanup_faillabel has been replaced with returning NULL.Looks good.
Of course, s/reset/rest/ is what I meant.
Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph
In reply to Josh SteadmonQuoted 11 lines · Josh Steadmon
fuzz-commit-graph.c new file +5
-0@@ -0,0 +1,18 @@ +#include "object-store.h" +#include "commit-graph.h" + +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size);
fuzz-commit-graph.c line 15
Quoted 10 lines
fuzz-commit-graph.c new file +10
-0+ +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size) +{ +struct commit_graph *g; + + g =parse_commit_graph ((void *) data,-1 , size); +if (g) +free (g);
As it is perfectly OK to free(NULL), please lose "if (g)" and a level of indentation; otherwise, "make coccicheck" would complain.
Thanks.
Quoted 2 lines
fuzz-commit-graph.c new file +2
-0+return 0 ; +}
PATCH 2/2 commit-graph: fix buffer read-overflow
In reply to Josh Steadmonfuzz-commit-graph identified a case where Git will read past the end of a buffer containing a commit graph if the graph's header has an incorrect chunk count. A simple bounds check in parse_commit_graph() prevents this.
--- commit-graph.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
commit-graph.c +11 -2
@@ -175,10 +175,19 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd, last_chunk_offset =8 ; chunk_lookup = data +8 ;for (i =0 ; i < graph->num_chunks; i++) { -uint32_t chunk_id =get_be32 (chunk_lookup +0 ); -uint64_t chunk_offset =get_be64 (chunk_lookup +4 ); +uint32_t chunk_id; +uint64_t chunk_offset;int chunk_repeated =0 ; +if (chunk_lookup +GRAPH_CHUNKLOOKUP_WIDTH > data + graph_size) { +error (_ ("chunk lookup table entry missing; graph file may be incomplete" )); +free (graph); +return NULL ; + } + + chunk_id =get_be32 (chunk_lookup +0 ); + chunk_offset =get_be64 (chunk_lookup +4 ); + chunk_lookup +=GRAPH_CHUNKLOOKUP_WIDTH ;if (chunk_offset > graph_size -GIT_MAX_RAWSZ ) {
Signature · 1 line
2.20.0.rc2.403.gdbc3b29805-goog
Re: [PATCH 2/2] commit-graph: fix buffer read-overflow
commit-graph.c struct commit_graph *parse_commit_graph(void *graph_map, int fd, · line 186
Quoted 6 lines · Josh Steadmon
commit-graph.c +5
-0+if (chunk_lookup +GRAPH_CHUNKLOOKUP_WIDTH > data + graph_size) { +error (_ ("chunk lookup table entry missing; graph file may be incomplete" )); +free (graph); +return NULL ; + }
Something I forgot earlier: there are several tests in t5318-commit-graph.sh that use 'git commit-graph verify' to ensure we hit these error conditions on a corrupted commit-graph file. Could you try adding a test there that looks for this error message?
Thanks,
-Stolee
v2 cover Add commit-graph fuzzer and fix buffer overflow
In reply to Josh SteadmonAdd a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered. Additionally, fix the Makefile instructions for building fuzzers.
Changes since V1:
* Moved the parse_commit_graph() declaration to the header file, since we don't mind if others use it.
* Moved some unnecessary comments into commit messages.
* Fixed some style issues.
* Added a test case for detecting commit graphs with missing chunk lookup entries.
* Ævar's comments on the Makefile made me realize the fuzzer build instructions were using the wrong variable. Added a new commit to fix this.
Josh Steadmon (3):
commit-graph, fuzz: Add fuzzer for commit-graph
commit-graph: fix buffer read-overflow
Makefile: correct example fuzz build
.gitignore | 1 + Makefile | 3 +- commit-graph.c | 67 +++++++++++++++++++++++++++++------------ commit-graph.h | 3 ++ fuzz-commit-graph.c | 16 ++++++++++ t/t5318-commit-graph.sh | 28 +++++++++++++++++ 6 files changed, 98 insertions(+), 20 deletions(-) create mode 100644 fuzz-commit-graph.c
Range-diff against v1:
1: 53e62baaa8 ! 1: 0b57ecbe1b commit-graph, fuzz: Add fuzzer for commit-graph
@@ -4,7 +4,9 @@
Breaks load_commit_graph_one() into a new function,
parse_commit_graph(). The latter function operates on arbitrary buffers,
- which makes it suitable as a fuzzing target.
+ which makes it suitable as a fuzzing target. Since parse_commit_graph()
+ is only called by load_commit_graph_one() (and the fuzzer described
+ below), we omit error messages that would be duplicated by the caller.
Adds fuzz-commit-graph.c, which provides a fuzzing entry point
compatible with libFuzzer (and possibly other fuzzing engines).
@@ -35,17 +37,6 @@
diff --git a/commit-graph.c b/commit-graph.c
--- a/commit-graph.c
+++ b/commit-graph.c
-@@
- #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \
- + GRAPH_FANOUT_SIZE + GRAPH_OID_LEN)
-
-+struct commit_graph *parse_commit_graph(void *graph_map, int fd,
-+ size_t graph_size);
-+
-+
- char *get_commit_graph_filename(const char *obj_dir)
- {
- return xstrfmt("%s/info/commit-graph", obj_dir);
@@
struct commit_graph *load_commit_graph_one(const char *graph_file)
{
@@ -70,7 +61,7 @@
graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
+ ret = parse_commit_graph(graph_map, fd, graph_size);
+
-+ if (ret == NULL) {
++ if (!ret) {
+ munmap(graph_map, graph_size);
+ close(fd);
+ exit(1);
@@ -79,10 +70,6 @@
+ return ret;
+}
+
-+/*
-+ * This function is intended to be used only from load_commit_graph_one() or in
-+ * fuzz tests.
-+ */
+struct commit_graph *parse_commit_graph(void *graph_map, int fd,
+ size_t graph_size)
+{
@@ -94,11 +81,9 @@
+ uint32_t graph_signature;
+ unsigned char graph_version, hash_version;
+
-+ /*
-+ * This should already be checked in load_commit_graph_one, but we still
-+ * need a check here for when we're calling parse_commit_graph directly
-+ * from fuzz tests. We can omit the error message in that case.
-+ */
++ if (!graph_map)
++ return NULL;
++
+ if (graph_size < GRAPH_MIN_SIZE)
+ return NULL;
+
@@ -162,12 +147,25 @@
static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
+ diff --git a/commit-graph.h b/commit-graph.h
+ --- a/commit-graph.h
+ +++ b/commit-graph.h
+@@
+
+ struct commit_graph *load_commit_graph_one(const char *graph_file);
+
++struct commit_graph *parse_commit_graph(void *graph_map, int fd,
++ size_t graph_size);
++
+ /*
+ * Return 1 if and only if the repository has a commit-graph
+ * file and generation numbers are computed in that file.
+
diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c
new file mode 100644
--- /dev/null
+++ b/fuzz-commit-graph.c
@@
-+#include "object-store.h"
+#include "commit-graph.h"
+
+struct commit_graph *parse_commit_graph(void *graph_map, int fd,
@@ -179,9 +177,8 @@
+{
+ struct commit_graph *g;
+
-+ g = parse_commit_graph((void *) data, -1, size);
-+ if (g)
-+ free(g);
++ g = parse_commit_graph((void *)data, -1, size);
++ free(g);
+
+ return 0;
+}
2: ad2e761f44 ! 2: af45c2337f commit-graph: fix buffer read-overflow
@@ -22,7 +22,8 @@
+ uint64_t chunk_offset;
int chunk_repeated = 0;
-+ if (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH > data + graph_size) {
++ if (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH >
++ data + graph_size) {
+ error(_("chunk lookup table entry missing; graph file may be incomplete"));
+ free(graph);
+ return NULL;
@@ -34,3 +35,49 @@
chunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;
if (chunk_offset > graph_size - GIT_MAX_RAWSZ) {
+
+ diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
+ --- a/t/t5318-commit-graph.sh
+ +++ b/t/t5318-commit-graph.sh
+@@
+ test_i18ngrep "$grepstr" err
+ }
+
++
++# usage: corrupt_and_zero_graph_then_verify <corrupt_position> <data> <zero_position> <string>
++# Manipulates the commit-graph file at <corrupt_position> by inserting the data,
++# then zeros the file starting at <zero_position>. Finally, runs
++# 'git commit-graph verify' and places the output in the file 'err'. Tests 'err'
++# for the given string.
++corrupt_and_zero_graph_then_verify() {
++ corrupt_pos=$1
++ data="${2:-\0}"
++ zero_pos=$3
++ grepstr=$4
++ orig_size=$(stat --format=%s $objdir/info/commit-graph)
++ cd "$TRASH_DIRECTORY/full" &&
++ test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
++ cp $objdir/info/commit-graph commit-graph-backup &&
++ printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$corrupt_pos" conv=notrunc &&
++ truncate --size=$zero_pos $objdir/info/commit-graph &&
++ truncate --size=$orig_size $objdir/info/commit-graph &&
++ test_must_fail git commit-graph verify 2>test_err &&
++ grep -v "^+" test_err >err &&
++ test_i18ngrep "$grepstr" err
++}
++
+ test_expect_success 'detect bad signature' '
+ corrupt_graph_and_verify 0 "\0" \
+ "graph signature"
+@@
+ "incorrect checksum"
+ '
+
++test_expect_success 'detect truncated graph' '
++ corrupt_and_zero_graph_then_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \
++ $GRAPH_CHUNK_LOOKUP_OFFSET "chunk lookup table entry missing"
++'
++
+ test_expect_success 'git fsck (checks commit-graph)' '
+ cd "$TRASH_DIRECTORY/full" &&
+ git fsck &&-: ---------- > 3: 7519fc76df Makefile: correct example fuzz build
Signature · 1 line
2.20.0.rc2.10.g7519fc76df
PATCH v2 1/3 commit-graph, fuzz: Add fuzzer for commit-graph
Breaks load_commit_graph_one() into a new function, parse_commit_graph(). The latter function operates on arbitrary buffers, which makes it suitable as a fuzzing target. Since parse_commit_graph() is only called by load_commit_graph_one() (and the fuzzer described below), we omit error messages that would be duplicated by the caller.
Adds fuzz-commit-graph.c, which provides a fuzzing entry point compatible with libFuzzer (and possibly other fuzzing engines).
--- .gitignore | 1 + Makefile | 1 + commit-graph.c | 53 ++++++++++++++++++++++++++++++--------------- commit-graph.h | 3 +++ fuzz-commit-graph.c | 16 ++++++++++++++ 5 files changed, 57 insertions(+), 17 deletions(-) create mode 100644 fuzz-commit-graph.c
.gitignore +1 -0
@@ -1,3 +1,4 @@ +/fuzz-commit-graph /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx
Makefile +1 -0
@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \ ETAGS_TARGET = TAGS +FUZZ_OBJS += fuzz-commit-graph.o FUZZ_OBJS += fuzz-pack-headers.o FUZZ_OBJS += fuzz-pack-idx.o
commit-graph.c +36 -17
104 lines not on this page · raw message
commit-graph.h +3 -0
@@ -54,6 +54,9 @@ struct commit_graph {struct commit_graph *load_commit_graph_one (const char *graph_file); +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); +/* * Return 1 if and only if the repository has a commit-graph * file and generation numbers are computed in that file.
fuzz-commit-graph.c new file +16 -0
@@ -0,0 +1,16 @@ +#include "commit-graph.h" + +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size) +{ +struct commit_graph *g; + + g =parse_commit_graph ((void *)data,-1 , size); +free (g); + +return 0 ; +}
Signature · 1 line
2.20.0.rc2.10.g7519fc76df
PATCH v2 2/3 commit-graph: fix buffer read-overflow
In reply to Josh Steadmonfuzz-commit-graph identified a case where Git will read past the end of a buffer containing a commit graph if the graph's header has an incorrect chunk count. A simple bounds check in parse_commit_graph() prevents this.
--- commit-graph.c | 14 ++++++++++++-- t/t5318-commit-graph.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-)
commit-graph.c +12 -2
@@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd, last_chunk_offset =8 ; chunk_lookup = data +8 ;for (i =0 ; i < graph->num_chunks; i++) { -uint32_t chunk_id =get_be32 (chunk_lookup +0 ); -uint64_t chunk_offset =get_be64 (chunk_lookup +4 ); +uint32_t chunk_id; +uint64_t chunk_offset;int chunk_repeated =0 ; +if (chunk_lookup +GRAPH_CHUNKLOOKUP_WIDTH > + data + graph_size) { +error (_ ("chunk lookup table entry missing; graph file may be incomplete" )); +free (graph); +return NULL ; + } + + chunk_id =get_be32 (chunk_lookup +0 ); + chunk_offset =get_be64 (chunk_lookup +4 ); + chunk_lookup +=GRAPH_CHUNKLOOKUP_WIDTH ;if (chunk_offset > graph_size -GIT_MAX_RAWSZ ) {
t/t5318-commit-graph.sh +28 -0
@@ -384,6 +384,29 @@ corrupt_graph_and_verify() { test_i18ngrep "$grepstr" err } + +# usage: corrupt_and_zero_graph_then_verify <corrupt_position> <data> <zero_position> <string> +# Manipulates the commit-graph file at <corrupt_position> by inserting the data, +# then zeros the file starting at <zero_position>. Finally, runs +# 'git commit-graph verify' and places the output in the file 'err'. Tests 'err' +# for the given string. +corrupt_and_zero_graph_then_verify() { + corrupt_pos=$1 + data="${2:-\0}" + zero_pos=$3 + grepstr=$4 + orig_size=$(stat --format=%s $objdir/info/commit-graph) + cd "$TRASH_DIRECTORY/full" && + test_when_finished mv commit-graph-backup $objdir/info/commit-graph && + cp $objdir/info/commit-graph commit-graph-backup && + printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$corrupt_pos" conv=notrunc && + truncate --size=$zero_pos $objdir/info/commit-graph && + truncate --size=$orig_size $objdir/info/commit-graph && + test_must_fail git commit-graph verify 2>test_err && + grep -v "^+" test_err >err && + test_i18ngrep "$grepstr" err +} + test_expect_success 'detect bad signature' ' corrupt_graph_and_verify 0 "\0" \ "graph signature"
@@ -484,6 +507,11 @@ test_expect_success 'detect invalid checksum hash' ' "incorrect checksum" ' +test_expect_success 'detect truncated graph' ' + corrupt_and_zero_graph_then_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \ + $GRAPH_CHUNK_LOOKUP_OFFSET "chunk lookup table entry missing" +' + test_expect_success 'git fsck (checks commit-graph)' ' cd "$TRASH_DIRECTORY/full" && git fsck &&
Signature · 1 line
2.20.0.rc2.10.g7519fc76df
Re: [PATCH v2 2/3] commit-graph: fix buffer read-overflow
Quoted 10 lines · Josh Steadmon
commit-graph.c +0
-2@@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd, last_chunk_offset =8 ; chunk_lookup = data +8 ;for (i =0 ; i < graph->num_chunks; i++) { -uint32_t chunk_id =get_be32 (chunk_lookup +0 ); -uint64_t chunk_offset =get_be64 (chunk_lookup +4 );
commit-graph.c struct commit_graph *parse_commit_graph(void *graph_map, int fd, · line 177
Quoted 10 lines
commit-graph.c +8
-0+uint32_t chunk_id; +uint64_t chunk_offset;int chunk_repeated =0 ; +if (chunk_lookup +GRAPH_CHUNKLOOKUP_WIDTH > + data + graph_size) { +error (_ ("chunk lookup table entry missing; graph file may be incomplete" )); +free (graph); +return NULL ; + }
Is it possible to overflow the addition here? E.g., if I'm on a 32-bit system and the truncated chunk appears right at the 4GB limit, in which case we wrap back around? I guess that's pretty implausible, since it would mean that the mmap is bumping up against the end of the address space. I didn't check, but I wouldn't be surprised if sane operating systems avoid allocating those addresses.
But I think you could write this as:
if (data + graph_size - chunk_lookup < GRAPH_CHUNKLOOKUP_WIDTH)
to avoid overflow (we know that "data + graph_size" is sane because that's our mmap, and chunk_lookup is somewhere between "data" and "data
+ graph_size", so the result is between 0 and graph_size).
I dunno. I think I've convinced myself it's a non-issue here, but it may be good to get in the habit of writing these sorts of offset checks in an overflow-proof order.
-Peff
Re: [PATCH v2 2/3] commit-graph: fix buffer read-overflow
In reply to Josh Steadmont/t5318-commit-graph.sh corrupt_graph_and_verify() { · line 393
Quoted 7 lines · Josh Steadmon
t/t5318-commit-graph.sh +7
-0+ +# usage: corrupt_and_zero_graph_then_verify <corrupt_position> <data> <zero_position> <string> +# Manipulates the commit-graph file at <corrupt_position> by inserting the data, +# then zeros the file starting at <zero_position>. Finally, runs +# 'git commit-graph verify' and places the output in the file 'err'. Tests 'err' +# for the given string. +corrupt_and_zero_graph_then_verify() {
This method is very similar to to 'corrupt_graph_and_verify()', the only difference being the zero_pos, which zeroes the graph.
Could it instead be a modification of corrupt_graph_and_verify() where $4 is interpreted as zero_pos, and if it is blank we don't do the truncation?
t/t5318-commit-graph.sh test_expect_success 'detect invalid checksum hash' ' · line 514
Quoted 5 lines
t/t5318-commit-graph.sh +5
-0+test_expect_success 'detect truncated graph' ' + corrupt_and_zero_graph_then_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \ + $GRAPH_CHUNK_LOOKUP_OFFSET "chunk lookup table entry missing" +' +
Thanks for this! I think it's valuable to keep explicit tests around that were discovered from your fuzz tests. Specifically, I can repeat the test when I get around to the next file format.
Thanks,
-Stolee
PATCH v2 3/3 Makefile: correct example fuzz build
In reply to Josh Steadmon--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
@@ -3104,7 +3104,7 @@ cover_db_html: cover_db # An example command to build against libFuzzer from LLVM 4.0.0: # # make CC=clang CXX=clang++ \ -# FUZZ_CXXFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ +# CFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \ # fuzz-all #
Signature · 1 line
2.20.0.rc2.10.g7519fc76df
v3 cover Add commit-graph fuzzer and fix buffer overflow
In reply to Josh SteadmonAd a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered. Additionally, fix the Makefile instructions for building fuzzers.
Changes since V2:
* Avoid pointer arithmetic overflow when checking the graph's chunk count.
* Merge the corrupt_graph_and_verify and corrupt_and_zero_graph_then_verify test functions.
Josh Steadmon (3):
commit-graph, fuzz: Add fuzzer for commit-graph
commit-graph: fix buffer read-overflow
Makefile: correct example fuzz build
.gitignore | 1 + Makefile | 3 +- commit-graph.c | 67 +++++++++++++++++++++++++++++------------ commit-graph.h | 3 ++ fuzz-commit-graph.c | 16 ++++++++++ t/t5318-commit-graph.sh | 15 +++++++-- 6 files changed, 83 insertions(+), 22 deletions(-) create mode 100644 fuzz-commit-graph.c
Range-diff against v2:
1: af45c2337f ! 1: 675d58ecea commit-graph: fix buffer read-overflow
@@ -22,8 +22,8 @@
+ uint64_t chunk_offset;
int chunk_repeated = 0;
-+ if (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH >
-+ data + graph_size) {
++ if (data + graph_size - chunk_lookup <
++ GRAPH_CHUNKLOOKUP_WIDTH) {
+ error(_("chunk lookup table entry missing; graph file may be incomplete"));
+ free(graph);
+ return NULL;
@@ -40,31 +40,34 @@
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@
- test_i18ngrep "$grepstr" err
- }
+ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))
+ GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
-+
-+# usage: corrupt_and_zero_graph_then_verify <corrupt_position> <data> <zero_position> <string>
-+# Manipulates the commit-graph file at <corrupt_position> by inserting the data,
-+# then zeros the file starting at <zero_position>. Finally, runs
-+# 'git commit-graph verify' and places the output in the file 'err'. Tests 'err'
-+# for the given string.
-+corrupt_and_zero_graph_then_verify() {
-+ corrupt_pos=$1
-+ data="${2:-\0}"
-+ zero_pos=$3
-+ grepstr=$4
+-# usage: corrupt_graph_and_verify <position> <data> <string>
++# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
+ # Manipulates the commit-graph file at the position
+-# by inserting the data, then runs 'git commit-graph verify'
++# by inserting the data, optionally zeroing the file
++# starting at <zero_pos>, then runs 'git commit-graph verify'
+ # and places the output in the file 'err'. Test 'err' for
+ # the given string.
+ corrupt_graph_and_verify() {
+ pos=$1
+ data="${2:-\0}"
+ grepstr=$3
+ orig_size=$(stat --format=%s $objdir/info/commit-graph)
-+ cd "$TRASH_DIRECTORY/full" &&
-+ test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
-+ cp $objdir/info/commit-graph commit-graph-backup &&
-+ printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$corrupt_pos" conv=notrunc &&
++ zero_pos=${4:-${orig_size}}
+ cd "$TRASH_DIRECTORY/full" &&
+ test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
+ cp $objdir/info/commit-graph commit-graph-backup &&
+ printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
+ truncate --size=$zero_pos $objdir/info/commit-graph &&
+ truncate --size=$orig_size $objdir/info/commit-graph &&
-+ test_must_fail git commit-graph verify 2>test_err &&
-+ grep -v "^+" test_err >err &&
-+ test_i18ngrep "$grepstr" err
-+}
+ test_must_fail git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err
+ test_i18ngrep "$grepstr" err
+ }
+
+
test_expect_success 'detect bad signature' '
corrupt_graph_and_verify 0 "\0" \
@@ -73,9 +76,9 @@
"incorrect checksum"
'
-+test_expect_success 'detect truncated graph' '
-+ corrupt_and_zero_graph_then_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \
-+ $GRAPH_CHUNK_LOOKUP_OFFSET "chunk lookup table entry missing"
++test_expect_success 'detect incorrect chunk count' '
++ corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \
++ "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET
+'
+
test_expect_success 'git fsck (checks commit-graph)' '2: 7519fc76df = 2: 06a32bfe8b Makefile: correct example fuzz build
Signature · 1 line
2.20.0.rc2.12.g4c11c11dec
PATCH v3 1/3 commit-graph, fuzz: Add fuzzer for commit-graph
Breaks load_commit_graph_one() into a new function, parse_commit_graph(). The latter function operates on arbitrary buffers, which makes it suitable as a fuzzing target. Since parse_commit_graph() is only called by load_commit_graph_one() (and the fuzzer described below), we omit error messages that would be duplicated by the caller.
Adds fuzz-commit-graph.c, which provides a fuzzing entry point compatible with libFuzzer (and possibly other fuzzing engines).
--- .gitignore | 1 + Makefile | 1 + commit-graph.c | 53 ++++++++++++++++++++++++++++++--------------- commit-graph.h | 3 +++ fuzz-commit-graph.c | 16 ++++++++++++++ 5 files changed, 57 insertions(+), 17 deletions(-) create mode 100644 fuzz-commit-graph.c
.gitignore +1 -0
@@ -1,3 +1,4 @@ +/fuzz-commit-graph /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx
Makefile +1 -0
@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \ ETAGS_TARGET = TAGS +FUZZ_OBJS += fuzz-commit-graph.o FUZZ_OBJS += fuzz-pack-headers.o FUZZ_OBJS += fuzz-pack-idx.o
commit-graph.c +36 -17
104 lines not on this page · raw message
commit-graph.h +3 -0
@@ -54,6 +54,9 @@ struct commit_graph {struct commit_graph *load_commit_graph_one (const char *graph_file); +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); +/* * Return 1 if and only if the repository has a commit-graph * file and generation numbers are computed in that file.
fuzz-commit-graph.c new file +16 -0
@@ -0,0 +1,16 @@ +#include "commit-graph.h" + +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size) +{ +struct commit_graph *g; + + g =parse_commit_graph ((void *)data,-1 , size); +free (g); + +return 0 ; +}
Signature · 1 line
2.20.0.rc2.12.g4c11c11dec
PATCH v3 2/3 commit-graph: fix buffer read-overflow
In reply to Josh Steadmonfuzz-commit-graph identified a case where Git will read past the end of a buffer containing a commit graph if the graph's header has an incorrect chunk count. A simple bounds check in parse_commit_graph() prevents this.
--- commit-graph.c | 14 ++++++++++++-- t/t5318-commit-graph.sh | 15 +++++++++++++-- 2 files changed, 25 insertions(+), 4 deletions(-)
commit-graph.c +12 -2
@@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd, last_chunk_offset =8 ; chunk_lookup = data +8 ;for (i =0 ; i < graph->num_chunks; i++) { -uint32_t chunk_id =get_be32 (chunk_lookup +0 ); -uint64_t chunk_offset =get_be64 (chunk_lookup +4 ); +uint32_t chunk_id; +uint64_t chunk_offset;int chunk_repeated =0 ; +if (data + graph_size - chunk_lookup < +GRAPH_CHUNKLOOKUP_WIDTH ) { +error (_ ("chunk lookup table entry missing; graph file may be incomplete" )); +free (graph); +return NULL ; + } + + chunk_id =get_be32 (chunk_lookup +0 ); + chunk_offset =get_be64 (chunk_lookup +4 ); + chunk_lookup +=GRAPH_CHUNKLOOKUP_WIDTH ;if (chunk_offset > graph_size -GIT_MAX_RAWSZ ) {
t/t5318-commit-graph.sh +13 -2
@@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES)) -# usage: corrupt_graph_and_verify <position> <data> <string> +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] # Manipulates the commit-graph file at the position -# by inserting the data, then runs 'git commit-graph verify' +# by inserting the data, optionally zeroing the file +# starting at <zero_pos>, then runs 'git commit-graph verify' # and places the output in the file 'err'. Test 'err' for # the given string. corrupt_graph_and_verify() { pos=$1 data="${2:-\0}" grepstr=$3 + orig_size=$(stat --format=%s $objdir/info/commit-graph) + zero_pos=${4:-${orig_size}} cd "$TRASH_DIRECTORY/full" && test_when_finished mv commit-graph-backup $objdir/info/commit-graph && cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + truncate --size=$zero_pos $objdir/info/commit-graph && + truncate --size=$orig_size $objdir/info/commit-graph && test_must_fail git commit-graph verify 2>test_err && grep -v "^+" test_err >err test_i18ngrep "$grepstr" err } + test_expect_success 'detect bad signature' ' corrupt_graph_and_verify 0 "\0" \ "graph signature"
@@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' ' "incorrect checksum" ' +test_expect_success 'detect incorrect chunk count' ' + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \ + "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' + test_expect_success 'git fsck (checks commit-graph)' ' cd "$TRASH_DIRECTORY/full" && git fsck &&
Signature · 1 line
2.20.0.rc2.12.g4c11c11dec
Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow
Quoted 11 lines · Josh Steadmon
t/t5318-commit-graph.sh +1
-1@@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES)) -# usage: corrupt_graph_and_verify <position> <data> <string> +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] # Manipulates the commit-graph file at the position
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 379
Quoted 10 lines
t/t5318-commit-graph.sh +3
-1-# by inserting the data, then runs 'git commit-graph verify' +# by inserting the data, optionally zeroing the file +# starting at <zero_pos>, then runs 'git commit-graph verify' # and places the output in the file 'err'. Test 'err' for # the given string. corrupt_graph_and_verify() { pos=$1 data="${2:-\0}" grepstr=$3 + orig_size=$(stat --format=%s $objdir/info/commit-graph)
"stat(1)" is not so portable, so you'll get complaints from minority platform users later. So is "truncate(1)".
Quoted 17 lines
t/t5318-commit-graph.sh +4
-0+ zero_pos=${4:-${orig_size}} cd "$TRASH_DIRECTORY/full" && test_when_finished mv commit-graph-backup $objdir/info/commit-graph && cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + truncate --size=$zero_pos $objdir/info/commit-graph && + truncate --size=$orig_size $objdir/info/commit-graph && test_must_fail git commit-graph verify 2>test_err && grep -v "^+" test_err >err test_i18ngrep "$grepstr" err } + test_expect_success 'detect bad signature' ' corrupt_graph_and_verify 0 "\0" \ "graph signature"
t/t5318-commit-graph.sh test_expect_success 'detect invalid checksum hash' ' · line 494
Quoted 5 lines
t/t5318-commit-graph.sh +2
-0@@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' ' "incorrect checksum" ' +test_expect_success 'detect incorrect chunk count' ' + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \
Implementations of printf(1) may not grok "\xff" as a valid representation of "\377". The shell built-in of dash(1) for example would not work with this.
Quoted 6 lines
t/t5318-commit-graph.sh +3
-0+ "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' + test_expect_success 'git fsck (checks commit-graph)' ' cd "$TRASH_DIRECTORY/full" && git fsck &&
Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Josh Steadmon
t/t5318-commit-graph.sh +0
-0@@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
Quoted 16 lines
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 379
Quoted 13 lines
t/t5318-commit-graph.sh +4
-2-# usage: corrupt_graph_and_verify <position> <data> <string> +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] # Manipulates the commit-graph file at the position -# by inserting the data, then runs 'git commit-graph verify' +# by inserting the data, optionally zeroing the file +# starting at <zero_pos>, then runs 'git commit-graph verify' # and places the output in the file 'err'. Test 'err' for # the given string. corrupt_graph_and_verify() { pos=$1 data="${2:-\0}" grepstr=$3 + orig_size=$(stat --format=%s $objdir/info/commit-graph)"stat(1)" is not so portable, so you'll get complaints from minority platform users later. So is "truncate(1)".
I complain: this patch breaks on macOS (on Travis CI), but in a curious way. First, 'stat' in the above line errors out with:
+++stat --format=%s .git/objects/info/commit-graph stat: illegal option -- - usage: stat [-FlLnqrsx] [-f format] [-t timefmt] [file ...]
Alas, this doesn't immediately fail the test, because it's not part of the &&-chain.
Quoted 1 line
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 380
Quoted 1 line
t/t5318-commit-graph.sh +1
-0+ zero_pos=${4:-${orig_size}}
No && here, either.
Quoted 5 lines
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 385
Quoted 5 lines
t/t5318-commit-graph.sh +1
-0cd "$TRASH_DIRECTORY/full" && test_when_finished mv commit-graph-backup $objdir/info/commit-graph && cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + truncate --size=$zero_pos $objdir/info/commit-graph &&
++truncate --size= .git/objects/info/commit-graph t5318-commit-graph.sh: line 385: truncate: command not found
Note that even if 'truncate' were available, it would most likely complain about the empty '--size=' argument resulting from the 'stat' error above.
Alas, this doesn't fail the test, either, because ...
Quoted 3 lines
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 388
Quoted 3 lines
t/t5318-commit-graph.sh +1
-0+ truncate --size=$orig_size $objdir/info/commit-graph && test_must_fail git commit-graph verify 2>test_err && grep -v "^+" test_err >err
... here the &&-chain was broken already before this patch. However, since this above command was not executed due to the missing 'truncate', it didn't have a chance to create the 'err' file, ...
Quoted 1 line
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 389
Quoted 1 line
t/t5318-commit-graph.sh +0
-0test_i18ngrep "$grepstr" err
... so 'test_i18ngrep' can't find the file, which triggers its linting error, finally aborting the whole test script.
Quoted 3 lines
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 392
Quoted 3 lines
t/t5318-commit-graph.sh +1
-0} +
Stray newline.
Quoted 20 lines
t/t5318-commit-graph.sh test_expect_success 'detect invalid checksum hash' ' · line 494
Quoted 9 lines
t/t5318-commit-graph.sh +2
-0test_expect_success 'detect bad signature' ' corrupt_graph_and_verify 0 "\0" \ "graph signature"@@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' ' "incorrect checksum" ' +test_expect_success 'detect incorrect chunk count' ' + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \Implementations of printf(1) may not grok "\xff" as a valid representation of "\377". The shell built-in of dash(1) for example would not work with this.
Quoted 6 lines
t/t5318-commit-graph.sh +3
-0+ "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' + test_expect_success 'git fsck (checks commit-graph)' ' cd "$TRASH_DIRECTORY/full" && git fsck &&
Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow
Quoted 90 lines · SZEDER Gábor
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Josh Steadmon
t/t5318-commit-graph.sh +0
-0@@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))Quoted 16 lines
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 379
Quoted 13 lines
t/t5318-commit-graph.sh +4
-2-# usage: corrupt_graph_and_verify <position> <data> <string> +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] # Manipulates the commit-graph file at the position -# by inserting the data, then runs 'git commit-graph verify' +# by inserting the data, optionally zeroing the file +# starting at <zero_pos>, then runs 'git commit-graph verify' # and places the output in the file 'err'. Test 'err' for # the given string. corrupt_graph_and_verify() { pos=$1 data="${2:-\0}" grepstr=$3 + orig_size=$(stat --format=%s $objdir/info/commit-graph)"stat(1)" is not so portable, so you'll get complaints from minority platform users later. So is "truncate(1)".
I complain: this patch breaks on macOS (on Travis CI), but in a curious way. First, 'stat' in the above line errors out with:
+++stat --format=%s .git/objects/info/commit-graph stat: illegal option -- - usage: stat [-FlLnqrsx] [-f format] [-t timefmt] [file ...]Alas, this doesn't immediately fail the test, because it's not part of the &&-chain.
Quoted 1 line
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 380
Quoted 1 line
t/t5318-commit-graph.sh +1
-0+ zero_pos=${4:-${orig_size}}No && here, either.
Quoted 5 lines
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 385
Quoted 5 lines
t/t5318-commit-graph.sh +1
-0cd "$TRASH_DIRECTORY/full" && test_when_finished mv commit-graph-backup $objdir/info/commit-graph && cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + truncate --size=$zero_pos $objdir/info/commit-graph &&++truncate --size= .git/objects/info/commit-graph
t5318-commit-graph.sh: line 385: truncate: command not foundNote that even if 'truncate' were available, it would most likely complain about the empty '--size=' argument resulting from the 'stat' error above.
Alas, this doesn't fail the test, either, because ...
Quoted 3 lines
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 388
Quoted 3 lines
t/t5318-commit-graph.sh +1
-0+ truncate --size=$orig_size $objdir/info/commit-graph && test_must_fail git commit-graph verify 2>test_err && grep -v "^+" test_err >err... here the &&-chain was broken already before this patch. However, since this above command was not executed due to the missing 'truncate', it didn't have a chance to create the 'err' file, ...
Quoted 1 line
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 389
Quoted 1 line
t/t5318-commit-graph.sh +0
-0test_i18ngrep "$grepstr" err... so '
test_i18ngrep' can't find the file, which triggers its linting error, finally aborting the whole test script.Quoted 3 lines
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 392
Quoted 3 lines
t/t5318-commit-graph.sh +1
-0} +Stray newline.
Quoted 14 lines
t/t5318-commit-graph.sh test_expect_success 'detect invalid checksum hash' ' · line 494
Quoted 9 lines
t/t5318-commit-graph.sh +2
-0test_expect_success 'detect bad signature' ' corrupt_graph_and_verify 0 "\0" \ "graph signature"@@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' ' "incorrect checksum" ' +test_expect_success 'detect incorrect chunk count' ' + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \Implementations of printf(1) may not grok "\xff" as a valid representation of "\377". The shell built-in of dash(1) for example would not work with this.
Quoted 6 lines
Quoted 6 lines
t/t5318-commit-graph.sh test_expect_success 'detect invalid checksum hash' ' · line 500
Quoted 6 lines
t/t5318-commit-graph.sh +3
-0+ "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' + test_expect_success 'git fsck (checks commit-graph)' ' cd "$TRASH_DIRECTORY/full" && git fsck &&
Thanks for the catch. All these will be fixed in V4.
Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow
In reply to Junio C HamanoQuoted 10 lines · Junio C Hamano
Quoted 8 lines · Josh Steadmon
t/t5318-commit-graph.sh +0
-0@@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
Quoted 16 lines
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 379
Quoted 13 lines
t/t5318-commit-graph.sh +4
-2-# usage: corrupt_graph_and_verify <position> <data> <string> +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] # Manipulates the commit-graph file at the position -# by inserting the data, then runs 'git commit-graph verify' +# by inserting the data, optionally zeroing the file +# starting at <zero_pos>, then runs 'git commit-graph verify' # and places the output in the file 'err'. Test 'err' for # the given string. corrupt_graph_and_verify() { pos=$1 data="${2:-\0}" grepstr=$3 + orig_size=$(stat --format=%s $objdir/info/commit-graph)"stat(1)" is not so portable, so you'll get complaints from minority platform users later. So is "truncate(1)".
Ack, thanks for the catch. I have a workaround for stat in the form of "wc -c", and for truncate with a combination of dd and /dev/zero. However, I'm finding conflicting information about whether or not /dev/zero exists on macOS. At the least, it sounds like it might not work on very old versions. Would this be acceptable, or should I add a new test function to do this?
Quoted 20 lines
Quoted 20 lines
t/t5318-commit-graph.sh +4
-0+ zero_pos=${4:-${orig_size}} cd "$TRASH_DIRECTORY/full" && test_when_finished mv commit-graph-backup $objdir/info/commit-graph && cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + truncate --size=$zero_pos $objdir/info/commit-graph && + truncate --size=$orig_size $objdir/info/commit-graph && test_must_fail git commit-graph verify 2>test_err && grep -v "^+" test_err >err test_i18ngrep "$grepstr" err } + test_expect_success 'detect bad signature' ' corrupt_graph_and_verify 0 "\0" \ "graph signature"@@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' ' "incorrect checksum" '
Quoted 6 lines
t/t5318-commit-graph.sh test_expect_success 'detect invalid checksum hash' ' · line 494
Quoted 2 lines
t/t5318-commit-graph.sh +2
-0+test_expect_success 'detect incorrect chunk count' ' + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \Implementations of printf(1) may not grok "\xff" as a valid representation of "\377". The shell built-in of dash(1) for example would not work with this.
Ack, will fix in V4. Thanks.
Quoted 6 lines
Quoted 6 lines
t/t5318-commit-graph.sh +3
-0+ "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' + test_expect_success 'git fsck (checks commit-graph)' ' cd "$TRASH_DIRECTORY/full" && git fsck &&
Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow
Quoted 9 lines · Josh Steadmon
Quoted 2 lines
"stat(1)" is not so portable, so you'll get complaints from minority platform users later. So is "truncate(1)".
Ack, thanks for the catch. I have a workaround for stat in the form of "wc -c", and for truncate with a combination of dd and
/dev/zero. However, I'm finding conflicting information about whether or not/dev/zeroexists on macOS. At the least, it sounds like it might not work on very old versions. Would this be acceptable, or should I add a new test function to do this?
If you're just interested in truncation (and not a bunch of zero bytes), you can dd from /dev/null.
Another way to truncate is to move the file elsewhere, and then copy only some of the bytes back (see t1450's "fsck detects truncated loose object").
-Peff
PATCH v3 3/3 Makefile: correct example fuzz build
In reply to Josh Steadmon--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
@@ -3104,7 +3104,7 @@ cover_db_html: cover_db # An example command to build against libFuzzer from LLVM 4.0.0: # # make CC=clang CXX=clang++ \ -# FUZZ_CXXFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ +# CFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \ # fuzz-all #
Signature · 1 line
2.20.0.rc2.12.g4c11c11dec
v4 cover Add commit-graph fuzzer and fix buffer overflow
In reply to Josh SteadmonAdd a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered. Additionally, fix the Makefile instructions for building fuzzers.
Changes since V3:
* Improve portability of the new test functionality.
* Fix broken &&-chains in tests.
Changes since V2:
* Avoid pointer arithmetic overflow when checking the graph's chunk count.
* Merge the corrupt_graph_and_verify and corrupt_and_zero_graph_then_verify test functions.
Josh Steadmon (3):
commit-graph, fuzz: Add fuzzer for commit-graph
commit-graph: fix buffer read-overflow
Makefile: correct example fuzz build
.gitignore | 1 + Makefile | 3 +- commit-graph.c | 67 +++++++++++++++++++++++++++++------------ commit-graph.h | 3 ++ fuzz-commit-graph.c | 16 ++++++++++ t/t5318-commit-graph.sh | 16 ++++++++-- 6 files changed, 83 insertions(+), 23 deletions(-) create mode 100644 fuzz-commit-graph.c
Range-diff against v3:
1: 675d58ecea ! 1: 80b5662f30 commit-graph: fix buffer read-overflow
@@ -55,29 +55,26 @@
pos=$1
data="${2:-\0}"
grepstr=$3
-+ orig_size=$(stat --format=%s $objdir/info/commit-graph)
-+ zero_pos=${4:-${orig_size}}
++ orig_size=$(wc -c < $objdir/info/commit-graph) &&
++ zero_pos=${4:-${orig_size}} &&
cd "$TRASH_DIRECTORY/full" &&
test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
cp $objdir/info/commit-graph commit-graph-backup &&
printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
-+ truncate --size=$zero_pos $objdir/info/commit-graph &&
-+ truncate --size=$orig_size $objdir/info/commit-graph &&
++ dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 &&
++ dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) &&
test_must_fail git commit-graph verify 2>test_err &&
- grep -v "^+" test_err >err
+- grep -v "^+" test_err >err
++ grep -v "^+" test_err >err &&
test_i18ngrep "$grepstr" err
}
-+
- test_expect_success 'detect bad signature' '
- corrupt_graph_and_verify 0 "\0" \
- "graph signature"
@@
"incorrect checksum"
'
+test_expect_success 'detect incorrect chunk count' '
-+ corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \
++ corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\377" \
+ "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET
+'
+2: 06a32bfe8b = 2: 21101b961a Makefile: correct example fuzz build
Signature · 1 line
2.20.0.rc2.10.g21101b961a
PATCH v4 1/3 commit-graph, fuzz: Add fuzzer for commit-graph
Breaks load_commit_graph_one() into a new function, parse_commit_graph(). The latter function operates on arbitrary buffers, which makes it suitable as a fuzzing target. Since parse_commit_graph() is only called by load_commit_graph_one() (and the fuzzer described below), we omit error messages that would be duplicated by the caller.
Adds fuzz-commit-graph.c, which provides a fuzzing entry point compatible with libFuzzer (and possibly other fuzzing engines).
--- .gitignore | 1 + Makefile | 1 + commit-graph.c | 53 ++++++++++++++++++++++++++++++--------------- commit-graph.h | 3 +++ fuzz-commit-graph.c | 16 ++++++++++++++ 5 files changed, 57 insertions(+), 17 deletions(-) create mode 100644 fuzz-commit-graph.c
.gitignore +1 -0
@@ -1,3 +1,4 @@ +/fuzz-commit-graph /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx
Makefile +1 -0
@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \ ETAGS_TARGET = TAGS +FUZZ_OBJS += fuzz-commit-graph.o FUZZ_OBJS += fuzz-pack-headers.o FUZZ_OBJS += fuzz-pack-idx.o
commit-graph.c +36 -17
104 lines not on this page · raw message
commit-graph.h +3 -0
@@ -54,6 +54,9 @@ struct commit_graph {struct commit_graph *load_commit_graph_one (const char *graph_file); +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); +/* * Return 1 if and only if the repository has a commit-graph * file and generation numbers are computed in that file.
fuzz-commit-graph.c new file +16 -0
@@ -0,0 +1,16 @@ +#include "commit-graph.h" + +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size) +{ +struct commit_graph *g; + + g =parse_commit_graph ((void *)data,-1 , size); +free (g); + +return 0 ; +}
Signature · 1 line
2.20.0.rc2.10.g21101b961a
PATCH v4 2/3 commit-graph: fix buffer read-overflow
In reply to Josh Steadmonfuzz-commit-graph identified a case where Git will read past the end of a buffer containing a commit graph if the graph's header has an incorrect chunk count. A simple bounds check in parse_commit_graph() prevents this.
--- commit-graph.c | 14 ++++++++++++-- t/t5318-commit-graph.sh | 16 +++++++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-)
commit-graph.c +12 -2
@@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd, last_chunk_offset =8 ; chunk_lookup = data +8 ;for (i =0 ; i < graph->num_chunks; i++) { -uint32_t chunk_id =get_be32 (chunk_lookup +0 ); -uint64_t chunk_offset =get_be64 (chunk_lookup +4 ); +uint32_t chunk_id; +uint64_t chunk_offset;int chunk_repeated =0 ; +if (data + graph_size - chunk_lookup < +GRAPH_CHUNKLOOKUP_WIDTH ) { +error (_ ("chunk lookup table entry missing; graph file may be incomplete" )); +free (graph); +return NULL ; + } + + chunk_id =get_be32 (chunk_lookup +0 ); + chunk_offset =get_be64 (chunk_lookup +4 ); + chunk_lookup +=GRAPH_CHUNKLOOKUP_WIDTH ;if (chunk_offset > graph_size -GIT_MAX_RAWSZ ) {
t/t5318-commit-graph.sh +13 -3
@@ -366,21 +366,26 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES)) -# usage: corrupt_graph_and_verify <position> <data> <string> +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] # Manipulates the commit-graph file at the position -# by inserting the data, then runs 'git commit-graph verify' +# by inserting the data, optionally zeroing the file +# starting at <zero_pos>, then runs 'git commit-graph verify' # and places the output in the file 'err'. Test 'err' for # the given string. corrupt_graph_and_verify() { pos=$1 data="${2:-\0}" grepstr=$3 + orig_size=$(wc -c < $objdir/info/commit-graph) && + zero_pos=${4:-${orig_size}} && cd "$TRASH_DIRECTORY/full" && test_when_finished mv commit-graph-backup $objdir/info/commit-graph && cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 && + dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) && test_must_fail git commit-graph verify 2>test_err && - grep -v "^+" test_err >err + grep -v "^+" test_err >err && test_i18ngrep "$grepstr" err }
@@ -484,6 +489,11 @@ test_expect_success 'detect invalid checksum hash' ' "incorrect checksum" ' +test_expect_success 'detect incorrect chunk count' ' + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\377" \ + "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' + test_expect_success 'git fsck (checks commit-graph)' ' cd "$TRASH_DIRECTORY/full" && git fsck &&
Signature · 1 line
2.20.0.rc2.10.g21101b961a
Re: [PATCH v4 2/3] commit-graph: fix buffer read-overflow
Quoted 11 lines · Josh Steadmon
t/t5318-commit-graph.sh +1
-1@@ -366,21 +366,26 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES)) -# usage: corrupt_graph_and_verify <position> <data> <string> +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] # Manipulates the commit-graph file at the position
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 379
Quoted 10 lines
t/t5318-commit-graph.sh +3
-1-# by inserting the data, then runs 'git commit-graph verify' +# by inserting the data, optionally zeroing the file +# starting at <zero_pos>, then runs 'git commit-graph verify' # and places the output in the file 'err'. Test 'err' for # the given string. corrupt_graph_and_verify() { pos=$1 data="${2:-\0}" grepstr=$3 + orig_size=$(wc -c < $objdir/info/commit-graph) &&
A minor nit: this test script is unusually prudent about which directory/repository each test is executed in, as the first thing each test does is to 'cd' into the right directory. (I think this is a Good Thing, and other test scripts should follow suit if they use a repo other than $TRASH_DIRECTORY.) Though it doesn't cause any immediate issues (the previous test happens to use the same repository), the above line violates this, as it accesses the '.git/.../commit-graph' file ...
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 381
Quoted 2 lines
t/t5318-commit-graph.sh +1
-0+ zero_pos=${4:-${orig_size}} && cd "$TRASH_DIRECTORY/full" &&
... before this line could ensure that it's in the right repository.
Quoted 26 lines
t/t5318-commit-graph.sh +8
-1test_when_finished mv commit-graph-backup $objdir/info/commit-graph && cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 && + dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) && test_must_fail git commit-graph verify 2>test_err && - grep -v "^+" test_err >err + grep -v "^+" test_err >err && test_i18ngrep "$grepstr" err }@@ -484,6 +489,11 @@ test_expect_success 'detect invalid checksum hash' ' "incorrect checksum" ' +test_expect_success 'detect incorrect chunk count' ' + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\377" \ + "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' + test_expect_success 'git fsck (checks commit-graph)' ' cd "$TRASH_DIRECTORY/full" && git fsck &&Signature · 1 line
2.20.0.rc2.10.g21101b961a
Re: [PATCH v4 2/3] commit-graph: fix buffer read-overflow
Quoted 32 lines · SZEDER Gábor
Quoted 11 lines · Josh Steadmon
t/t5318-commit-graph.sh +1
-1@@ -366,21 +366,26 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES)) -# usage: corrupt_graph_and_verify <position> <data> <string> +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] # Manipulates the commit-graph file at the positiont/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 379
Quoted 10 lines
t/t5318-commit-graph.sh +3
-1-# by inserting the data, then runs 'git commit-graph verify' +# by inserting the data, optionally zeroing the file +# starting at <zero_pos>, then runs 'git commit-graph verify' # and places the output in the file 'err'. Test 'err' for # the given string. corrupt_graph_and_verify() { pos=$1 data="${2:-\0}" grepstr=$3 + orig_size=$(wc -c < $objdir/info/commit-graph) &&A minor nit: this test script is unusually prudent about which directory/repository each test is executed in, as the first thing each test does is to 'cd' into the right directory. (I think this is a Good Thing, and other test scripts should follow suit if they use a repo other than $TRASH_DIRECTORY.) Though it doesn't cause any immediate issues (the previous test happens to use the same repository), the above line violates this, as it accesses the '.git/.../commit-graph' file ...
Quoted 4 lines
t/t5318-commit-graph.sh GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ · line 381
Quoted 2 lines
t/t5318-commit-graph.sh +1
-0+ zero_pos=${4:-${orig_size}} && cd "$TRASH_DIRECTORY/full" &&... before this line could ensure that it's in the right repository.
Thanks for the catch. Fixed in v5.
PATCH v4 3/3 Makefile: correct example fuzz build
In reply to Josh Steadmon--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
@@ -3104,7 +3104,7 @@ cover_db_html: cover_db # An example command to build against libFuzzer from LLVM 4.0.0: # # make CC=clang CXX=clang++ \ -# FUZZ_CXXFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ +# CFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \ # fuzz-all #
Signature · 1 line
2.20.0.rc2.10.g21101b961a
Quoted 6 lines · Josh Steadmon
Add a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered. Additionally, fix the
Makefileinstructions for building fuzzers.Changes since V3:
* Improve portability of the new test functionality.
I thought there was some question about /dev/zero, which I think is in this version (I don't actually know whether there are portability issues or not, but somebody did mention it).
-Peff
Quoted 6 lines · Jeff King
Quoted 4 lines · Josh Steadmon
Add a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered. Additionally, fix the
Makefileinstructions for building fuzzers.
Quoted 8 lines
Quoted 2 lines
Changes since V3:
* Improve portability of the new test functionality.I thought there was some question about
/dev/zero, which I think is in this version (I don't actually know whether there are portability issues or not, but somebody did mention it).-Peff
I've only found one reference [1] (from 1999) of OS X Server not having a /dev/zero. It appears to be present as of 2010 though [2].
[1]: https://macosx-admin.omnigroup.narkive.com/sAxj0XeP/dev-zero-equivalent-in-mac-os-x
[2]: https://serverfault.com/questions/143248/zeroing-a-disk-with-dd-vs-disk-utility
Quoted 10 lines · Josh Steadmon
Quoted 9 lines · Jeff King
Quoted 6 lines · Josh Steadmon
Add a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered. Additionally, fix the
Makefileinstructions for building fuzzers.Changes since V3:
* Improve portability of the new test functionality.
Quoted 8 lines
Quoted 5 lines
I thought there was some question about
/dev/zero, which I think is in this version (I don't actually know whether there are portability issues or not, but somebody did mention it).-Peff
I've only found one reference [1] (from 1999) of OS X Server not having a
/dev/zero. It appears to be present as of 2010 though [2].
Thanks for digging. That seems like enough to assume we should try it and see if any macOS people complain.
I do wonder if we'll run into problems on Windows, though.
-Peff
Hi Peff,
Quoted 18 lines · Jeff King
Quoted 16 lines · Josh Steadmon
Quoted 14 lines · Jeff King
Quoted 6 lines · Josh Steadmon
Add a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered. Additionally, fix the
Makefileinstructions for building fuzzers.Changes since V3:
* Improve portability of the new test functionality.I thought there was some question about
/dev/zero, which I think is in this version (I don't actually know whether there are portability issues or not, but somebody did mention it).-Peff
Quoted 7 lines
Quoted 2 lines
I've only found one reference [1] (from 1999) of OS X Server not having a
/dev/zero. It appears to be present as of 2010 though [2].Thanks for digging. That seems like enough to assume we should try it and see if any macOS people complain.
I do wonder if we'll run into problems on Windows, though.
As long as we're talking about Unix shell scripts, /dev/zero should be fine, as we are essentially running in a variant of Cygwin.
If you try to pass /dev/zero as an argument to a Git command, that's an entirely different thing: this most likely won't work.
Ciao,
Dscho
Quoted 7 lines · Johannes Schindelin
Quoted 1 line
I do wonder if we'll run into problems on Windows, though.
As long as we're talking about Unix shell scripts,
/dev/zeroshould be fine, as we are essentially running in a variant of Cygwin.If you try to pass
/dev/zeroas an argument to a Git command, that's an entirely different thing: this most likely won't work.
Thanks for confirming. We're talking about passing it to dd here, so I think it should be OK.
-Peff
Quoted 16 lines · Jeff King
Quoted 14 lines · Josh Steadmon
Quoted 13 lines · Jeff King
Quoted 6 lines · Josh Steadmon
Add a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered. Additionally, fix the
Makefileinstructions for building fuzzers.Changes since V3:
* Improve portability of the new test functionality.I thought there was some question about
/dev/zero, which I think is in this version (I don't actually know whether there are portability issues or not, but somebody did mention it).
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
-Peff
I've only found one reference [1] (from 1999) of OS X Server not having a
/dev/zero. It appears to be present as of 2010 though [2].Thanks for digging. That seems like enough to assume we should try it and see if any macOS people complain.
Our tests have been relying on /dev/zero since 852a1710 ("am: let command-line options override saved options", 2015-08-04) that appeared in v2.6.0. Anybody who has trouble with /dev/zero now has kept silent for about a dozen major releases, I think, and will be silent with this one, too ;-)
Quoted 4 lines
I do wonder if we'll run into problems on Windows, though.
-Peff
v5 cover Add commit-graph fuzzer and fix buffer overflow
In reply to Josh SteadmonAdd a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered. Additionally, fix the Makefile instructions for building fuzzers.
Changes since V4:
* Ensure that corrupt_graph_and_verify() in t5318 changes to the proper directory before accessing any files.
Changes since V3:
* Improve portability of the new test functionality.
* Fix broken &&-chains in tests.
Changes since V2:
* Avoid pointer arithmetic overflow when checking the graph's chunk count.
* Merge the corrupt_graph_and_verify and corrupt_and_zero_graph_then_verify test functions.
Josh Steadmon (3):
commit-graph, fuzz: Add fuzzer for commit-graph
commit-graph: fix buffer read-overflow
Makefile: correct example fuzz build
Josh Steadmon (3):
commit-graph, fuzz: Add fuzzer for commit-graph
commit-graph: fix buffer read-overflow
Makefile: correct example fuzz build
.gitignore | 1 + Makefile | 3 +- commit-graph.c | 67 +++++++++++++++++++++++++++++------------ commit-graph.h | 3 ++ fuzz-commit-graph.c | 16 ++++++++++ t/t5318-commit-graph.sh | 16 ++++++++-- 6 files changed, 83 insertions(+), 23 deletions(-) create mode 100644 fuzz-commit-graph.c
Range-diff against v4:
1: 80b5662f30 ! 1: a3b5d33c4b commit-graph: fix buffer read-overflow
@@ -52,12 +52,12 @@
# and places the output in the file 'err'. Test 'err' for
# the given string.
corrupt_graph_and_verify() {
- pos=$1
+@@
data="${2:-\0}"
grepstr=$3
+ cd "$TRASH_DIRECTORY/full" &&
+ orig_size=$(wc -c < $objdir/info/commit-graph) &&
+ zero_pos=${4:-${orig_size}} &&
- cd "$TRASH_DIRECTORY/full" &&
test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
cp $objdir/info/commit-graph commit-graph-backup &&
printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&2: 21101b961a = 2: 350ea5f7c9 Makefile: correct example fuzz build
Signature · 1 line
2.20.1.97.g81188d93c3-goog
PATCH v5 1/3 commit-graph, fuzz: Add fuzzer for commit-graph
Breaks load_commit_graph_one() into a new function, parse_commit_graph(). The latter function operates on arbitrary buffers, which makes it suitable as a fuzzing target. Since parse_commit_graph() is only called by load_commit_graph_one() (and the fuzzer described below), we omit error messages that would be duplicated by the caller.
Adds fuzz-commit-graph.c, which provides a fuzzing entry point compatible with libFuzzer (and possibly other fuzzing engines).
--- .gitignore | 1 + Makefile | 1 + commit-graph.c | 53 ++++++++++++++++++++++++++++++--------------- commit-graph.h | 3 +++ fuzz-commit-graph.c | 16 ++++++++++++++ 5 files changed, 57 insertions(+), 17 deletions(-) create mode 100644 fuzz-commit-graph.c
.gitignore +1 -0
@@ -1,3 +1,4 @@ +/fuzz-commit-graph /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx
Makefile +1 -0
@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \ ETAGS_TARGET = TAGS +FUZZ_OBJS += fuzz-commit-graph.o FUZZ_OBJS += fuzz-pack-headers.o FUZZ_OBJS += fuzz-pack-idx.o
commit-graph.c +36 -17
104 lines not on this page · raw message
commit-graph.h +3 -0
@@ -54,6 +54,9 @@ struct commit_graph {struct commit_graph *load_commit_graph_one (const char *graph_file); +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); +/* * Return 1 if and only if the repository has a commit-graph * file and generation numbers are computed in that file.
fuzz-commit-graph.c new file +16 -0
@@ -0,0 +1,16 @@ +#include "commit-graph.h" + +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size) +{ +struct commit_graph *g; + + g =parse_commit_graph ((void *)data,-1 , size); +free (g); + +return 0 ; +}
Signature · 1 line
2.20.1.97.g81188d93c3-goog
Re: [PATCH v5 1/3] commit-graph, fuzz: Add fuzzer for commit-graph
Quoted 1 line · Josh Steadmon
Breaks
load_commit_graph_one()into a new function,
s/Breaks/Break/
Quoted 65 lines
parse_commit_graph(). The latter function operates on arbitrary buffers, which makes it suitable as a fuzzing target. Sinceparse_commit_graph()is only called byload_commit_graph_one()(and the fuzzer described below), we omit error messages that would be duplicated by the caller.Adds
fuzz-commit-graph.c, which provides a fuzzing entry point compatible with libFuzzer (and possibly other fuzzing engines).Signed-off-by Josh Steadmon <[email protected]>--- .gitignore | 1 + Makefile | 1 + commit-graph.c | 53 ++++++++++++++++++++++++++++++--------------- commit-graph.h | 3 +++ fuzz-commit-graph.c | 16 ++++++++++++++ 5 files changed, 57 insertions(+), 17 deletions(-) create mode 100644 fuzz-commit-graph.c.gitignore +1
-0@@ -1,3 +1,4 @@ +/fuzz-commit-graph /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idxMakefile +1
-0@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \ ETAGS_TARGET = TAGS +FUZZ_OBJS += fuzz-commit-graph.o FUZZ_OBJS += fuzz-pack-headers.o FUZZ_OBJS += fuzz-pack-idx.ocommit-graph.c +1
-7@@ -84,16 +84,10 @@ static int commit_graph_compatible(struct repository *r)struct commit_graph *load_commit_graph_one (const char *graph_file) {void *graph_map; -const unsigned char *data, *chunk_lookup;size_t graph_size;struct stat st; -uint32_t i; -struct commit_graph *graph; +struct commit_graph *ret;int fd =git_open (graph_file); -uint64_t last_chunk_offset; -uint32_t last_chunk_id; -uint32_t graph_signature; -unsigned char graph_version, hash_version;if (fd <0 )return NULL ;@@ -108,27 +102,55 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)die (_ ("graph file %s is too small" ), graph_file); } graph_map =xmmap (NULL , graph_size,PROT_READ ,MAP_PRIVATE , fd,0 );
commit-graph.c struct commit_graph *load_commit_graph_one(const char *graph_file) · line 114
Quoted 10 lines
commit-graph.c +10
-0+ ret =parse_commit_graph (graph_map, fd, graph_size); + +if (!ret) { +munmap (graph_map, graph_size); +close (fd); +exit (1 ); + } + +return ret; +}
Looks like a reasonable splitting of a helper function with a clean interface. I like it.
Thanks.
Quoted 112 lines
commit-graph.c +24
-10+struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size) +{ +const unsigned char *data, *chunk_lookup; +uint32_t i; +struct commit_graph *graph; +uint64_t last_chunk_offset; +uint32_t last_chunk_id; +uint32_t graph_signature; +unsigned char graph_version, hash_version; + +if (!graph_map) +return NULL ; + +if (graph_size <GRAPH_MIN_SIZE ) +return NULL ; + data = (const unsigned char *)graph_map; graph_signature =get_be32 (data);if (graph_signature !=GRAPH_SIGNATURE ) {error (_ ("graph signature %X does not match signature %X" ), graph_signature,GRAPH_SIGNATURE ); - goto cleanup_fail; +return NULL ; } graph_version = *(unsigned char *)(data +4 );if (graph_version !=GRAPH_VERSION ) {error (_ ("graph version %X does not match version %X" ), graph_version,GRAPH_VERSION ); - goto cleanup_fail; +return NULL ; } hash_version = *(unsigned char *)(data +5 );if (hash_version !=GRAPH_OID_VERSION ) {error (_ ("hash version %X does not match version %X" ), hash_version,GRAPH_OID_VERSION ); - goto cleanup_fail; +return NULL ; } graph =alloc_commit_graph ();@@ -152,7 +174,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)if (chunk_offset > graph_size -GIT_MAX_RAWSZ ) {error (_ ("improper chunk offset %08x%08x" ), (uint32_t )(chunk_offset >>32 ), (uint32_t )chunk_offset); - goto cleanup_fail; +free (graph); +return NULL ; }switch (chunk_id) {@@ -187,7 +210,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)if (chunk_repeated) {error (_ ("chunk id %08x appears multiple times" ), chunk_id); - goto cleanup_fail; +free (graph); +return NULL ; }if (last_chunk_id ==GRAPH_CHUNKID_OIDLOOKUP )@@ -201,11 +225,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file) }return graph; - -cleanup_fail: -munmap (graph_map, graph_size); -close (fd); -exit (1 ); }static void prepare_commit_graph_one (struct repository *r,const char *obj_dir)commit-graph.h +3
-0@@ -54,6 +54,9 @@ struct commit_graph {struct commit_graph *load_commit_graph_one (const char *graph_file); +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); +/* * Return 1 if and only if the repository has a commit-graph * file and generation numbers are computed in that file. fuzz-commit-graph.c new file +16
-0@@ -0,0 +1,16 @@ +#include "commit-graph.h" + +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size) +{ +struct commit_graph *g; + + g =parse_commit_graph ((void *)data,-1 , size); +free (g); + +return 0 ; +}
PATCH v5 2/3 commit-graph: fix buffer read-overflow
In reply to Josh Steadmonfuzz-commit-graph identified a case where Git will read past the end of a buffer containing a commit graph if the graph's header has an incorrect chunk count. A simple bounds check in parse_commit_graph() prevents this.
--- commit-graph.c | 14 ++++++++++++-- t/t5318-commit-graph.sh | 16 +++++++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-)
commit-graph.c +12 -2
@@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd, last_chunk_offset =8 ; chunk_lookup = data +8 ;for (i =0 ; i < graph->num_chunks; i++) { -uint32_t chunk_id =get_be32 (chunk_lookup +0 ); -uint64_t chunk_offset =get_be64 (chunk_lookup +4 ); +uint32_t chunk_id; +uint64_t chunk_offset;int chunk_repeated =0 ; +if (data + graph_size - chunk_lookup < +GRAPH_CHUNKLOOKUP_WIDTH ) { +error (_ ("chunk lookup table entry missing; graph file may be incomplete" )); +free (graph); +return NULL ; + } + + chunk_id =get_be32 (chunk_lookup +0 ); + chunk_offset =get_be64 (chunk_lookup +4 ); + chunk_lookup +=GRAPH_CHUNKLOOKUP_WIDTH ;if (chunk_offset > graph_size -GIT_MAX_RAWSZ ) {
t/t5318-commit-graph.sh +13 -3
@@ -366,9 +366,10 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES)) -# usage: corrupt_graph_and_verify <position> <data> <string> +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] # Manipulates the commit-graph file at the position -# by inserting the data, then runs 'git commit-graph verify' +# by inserting the data, optionally zeroing the file +# starting at <zero_pos>, then runs 'git commit-graph verify' # and places the output in the file 'err'. Test 'err' for # the given string. corrupt_graph_and_verify() {
@@ -376,11 +377,15 @@ corrupt_graph_and_verify() { data="${2:-\0}" grepstr=$3 cd "$TRASH_DIRECTORY/full" && + orig_size=$(wc -c < $objdir/info/commit-graph) && + zero_pos=${4:-${orig_size}} && test_when_finished mv commit-graph-backup $objdir/info/commit-graph && cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 && + dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) && test_must_fail git commit-graph verify 2>test_err && - grep -v "^+" test_err >err + grep -v "^+" test_err >err && test_i18ngrep "$grepstr" err }
@@ -484,6 +489,11 @@ test_expect_success 'detect invalid checksum hash' ' "incorrect checksum" ' +test_expect_success 'detect incorrect chunk count' ' + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\377" \ + "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' + test_expect_success 'git fsck (checks commit-graph)' ' cd "$TRASH_DIRECTORY/full" && git fsck &&
Signature · 1 line
2.20.1.97.g81188d93c3-goog
PATCH v5 3/3 Makefile: correct example fuzz build
In reply to Josh Steadmon--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
@@ -3104,7 +3104,7 @@ cover_db_html: cover_db # An example command to build against libFuzzer from LLVM 4.0.0: # # make CC=clang CXX=clang++ \ -# FUZZ_CXXFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ +# CFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \ # fuzz-all #
Signature · 1 line
2.20.1.97.g81188d93c3-goog
Re: [PATCH v5 3/3] Makefile: correct example fuzz build
Quoted 10 lines · Josh Steadmon
Signed-off-by Josh Steadmon <[email protected]>--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Makefile +0
-0
Makefile cover_db_html: cover_db · line 3110
Quoted 8 lines
Makefile +1
-1@@ -3104,7 +3104,7 @@ cover_db_html: cover_db # An example command to build against libFuzzer from LLVM 4.0.0: # # make CC=clang CXX=clang++ \ -# FUZZ_CXXFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ +# CFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \ # fuzz-all #
I know this appeared in v2 of the series, but I cannot quite read the reasoning/justification behind this change. After this hunk there is
FUZZ_CXXFLAGS ?= $(CFLAGS)
so if you do not give CFLAGS but give FUZZ_CXXFLAGS, like the sample, shouldn't it have worked just fine? IOW "correct" in the title is a bit too terse as an explanation for this change.
Re: [PATCH v5 3/3] Makefile: correct example fuzz build
Quoted 25 lines · Junio C Hamano
Quoted 10 lines · Josh Steadmon
Signed-off-by Josh Steadmon <[email protected]>--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Makefile +0
-0Makefile cover_db_html: cover_db · line 3110
Quoted 8 lines
Makefile +1
-1@@ -3104,7 +3104,7 @@ cover_db_html: cover_db # An example command to build against libFuzzer from LLVM 4.0.0: # # make CC=clang CXX=clang++ \ -# FUZZ_CXXFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ +# CFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \ # fuzz-all #I know this appeared in v2 of the series, but I cannot quite read the reasoning/justification behind this change. After this hunk there is
Quoted 5 lines
FUZZ_CXXFLAGS ?= $(CFLAGS)so if you do not give CFLAGS but give
FUZZ_CXXFLAGS, like the sample, shouldn't it have worked just fine? IOW "correct" in the title is a bit too terse as an explanation for this change.
Sorry for being too vague. The problem with only including FUZZ_CXXFLAGS is that all the .c files need to be compiled with coverage tracking enabled, not just the fuzzer itself.
The motivation for splitting CFLAGS and FUZZ_CXXFLAGS in the first place was to enable OSS-Fuzz (and others) to include C++-specific flags without causing a ton of compiler warnings when those are applied to the .c files. So OSS-Fuzz sets both CFLAGS and FUZZ_CXXFLAGS when building.
We could fix the comment by setting both CFLAGS and FUZZ_CXXFLAGS, but since we're not including any C++-specific flags there's no reason to set both.
I'll fix the commit message in v6.
Re: [PATCH v5 3/3] Makefile: correct example fuzz build
Quoted 3 lines · Josh Steadmon
Sorry for being too vague. The problem with only including
FUZZ_CXXFLAGSis that all the .c files need to be compiled with coverage tracking enabled, not just the fuzzer itself.
OK.
Quoted 3 lines
We could fix the comment by setting both CFLAGS and
FUZZ_CXXFLAGS, but since we're not including any C++-specific flags there's no reason to set both.
Yes.
v6 cover Add commit-graph fuzzer and fix buffer overflow
In reply to Josh SteadmonAdd a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered. Additionally, fix the Makefile instructions for building fuzzers.
Changes since V5:
* Conform to commit message standards for the 1st patch in the series.
* Clarify commit message for the 3rd patch in the series.
Changes since V4:
* Ensure that corrupt_graph_and_verify() in t5318 changes to the proper directory before accessing any files.
Changes since V3:
* Improve portability of the new test functionality.
* Fix broken &&-chains in tests.
Changes since V2:
* Avoid pointer arithmetic overflow when checking the graph's chunk count.
* Merge the corrupt_graph_and_verify and corrupt_and_zero_graph_then_verify test functions.
Josh Steadmon (3):
commit-graph, fuzz: Add fuzzer for commit-graph
commit-graph: fix buffer read-overflow
Makefile: correct example fuzz build
.gitignore | 1 + Makefile | 3 +- commit-graph.c | 67 +++++++++++++++++++++++++++++------------ commit-graph.h | 3 ++ fuzz-commit-graph.c | 16 ++++++++++ t/t5318-commit-graph.sh | 16 ++++++++-- 6 files changed, 83 insertions(+), 23 deletions(-) create mode 100644 fuzz-commit-graph.c
Range-diff against v5:
1: 0b57ecbe1b ! 1: c4ec3fc3fc commit-graph, fuzz: Add fuzzer for commit-graph
@@ -2,11 +2,11 @@
commit-graph, fuzz: Add fuzzer for commit-graph
- Breaks load_commit_graph_one() into a new function,
- parse_commit_graph(). The latter function operates on arbitrary buffers,
- which makes it suitable as a fuzzing target. Since parse_commit_graph()
- is only called by load_commit_graph_one() (and the fuzzer described
- below), we omit error messages that would be duplicated by the caller.
+ Break load_commit_graph_one() into a new function, parse_commit_graph().
+ The latter function operates on arbitrary buffers, which makes it
+ suitable as a fuzzing target. Since parse_commit_graph() is only called
+ by load_commit_graph_one() (and the fuzzer described below), we omit
+ error messages that would be duplicated by the caller.
Adds fuzz-commit-graph.c, which provides a fuzzing entry point compatible with libFuzzer (and possibly other fuzzing engines).
2: a3b5d33c4b = 2: d7b137650f commit-graph: fix buffer read-overflow 3: 350ea5f7c9 ! 3: c06e0667fa Makefile: correct example fuzz build
@@ -2,6 +2,15 @@
Makefile: correct example fuzz build
+ The comment explaining how to build the fuzzers was broken in
+ 927c77e7d4d ("Makefile: use FUZZ_CXXFLAGS for linking fuzzers",
+ 2018-11-14).
+
+ When building fuzzers, all .c files must be compiled with coverage
+ tracing enabled. This is not possible when using only FUZZ_CXXFLAGS, as
+ that flag is only applied to the fuzzers themselves. Switching back to
+ CFLAGS fixes the issue.
+
diff --git a/Makefile b/Makefile
Signature · 1 line
2.20.1.97.g81188d93c3-goog
PATCH v6 1/3 commit-graph, fuzz: Add fuzzer for commit-graph
Break load_commit_graph_one() into a new function, parse_commit_graph(). The latter function operates on arbitrary buffers, which makes it suitable as a fuzzing target. Since parse_commit_graph() is only called by load_commit_graph_one() (and the fuzzer described below), we omit error messages that would be duplicated by the caller.
Adds fuzz-commit-graph.c, which provides a fuzzing entry point compatible with libFuzzer (and possibly other fuzzing engines).
--- .gitignore | 1 + Makefile | 1 + commit-graph.c | 53 ++++++++++++++++++++++++++++++--------------- commit-graph.h | 3 +++ fuzz-commit-graph.c | 16 ++++++++++++++ 5 files changed, 57 insertions(+), 17 deletions(-) create mode 100644 fuzz-commit-graph.c
.gitignore +1 -0
@@ -1,3 +1,4 @@ +/fuzz-commit-graph /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx
Makefile +1 -0
@@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \ ETAGS_TARGET = TAGS +FUZZ_OBJS += fuzz-commit-graph.o FUZZ_OBJS += fuzz-pack-headers.o FUZZ_OBJS += fuzz-pack-idx.o
commit-graph.c +36 -17
104 lines not on this page · raw message
commit-graph.h +3 -0
@@ -54,6 +54,9 @@ struct commit_graph {struct commit_graph *load_commit_graph_one (const char *graph_file); +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); +/* * Return 1 if and only if the repository has a commit-graph * file and generation numbers are computed in that file.
fuzz-commit-graph.c new file +16 -0
@@ -0,0 +1,16 @@ +#include "commit-graph.h" + +struct commit_graph *parse_commit_graph (void *graph_map,int fd, +size_t graph_size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size); + +int LLVMFuzzerTestOneInput (const uint8_t *data,size_t size) +{ +struct commit_graph *g; + + g =parse_commit_graph ((void *)data,-1 , size); +free (g); + +return 0 ; +}
Signature · 1 line
2.20.1.97.g81188d93c3-goog
PATCH v6 2/3 commit-graph: fix buffer read-overflow
In reply to Josh Steadmonfuzz-commit-graph identified a case where Git will read past the end of a buffer containing a commit graph if the graph's header has an incorrect chunk count. A simple bounds check in parse_commit_graph() prevents this.
--- commit-graph.c | 14 ++++++++++++-- t/t5318-commit-graph.sh | 16 +++++++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-)
commit-graph.c +12 -2
@@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd, last_chunk_offset =8 ; chunk_lookup = data +8 ;for (i =0 ; i < graph->num_chunks; i++) { -uint32_t chunk_id =get_be32 (chunk_lookup +0 ); -uint64_t chunk_offset =get_be64 (chunk_lookup +4 ); +uint32_t chunk_id; +uint64_t chunk_offset;int chunk_repeated =0 ; +if (data + graph_size - chunk_lookup < +GRAPH_CHUNKLOOKUP_WIDTH ) { +error (_ ("chunk lookup table entry missing; graph file may be incomplete" )); +free (graph); +return NULL ; + } + + chunk_id =get_be32 (chunk_lookup +0 ); + chunk_offset =get_be64 (chunk_lookup +4 ); + chunk_lookup +=GRAPH_CHUNKLOOKUP_WIDTH ;if (chunk_offset > graph_size -GIT_MAX_RAWSZ ) {
t/t5318-commit-graph.sh +13 -3
@@ -366,9 +366,10 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES)) -# usage: corrupt_graph_and_verify <position> <data> <string> +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] # Manipulates the commit-graph file at the position -# by inserting the data, then runs 'git commit-graph verify' +# by inserting the data, optionally zeroing the file +# starting at <zero_pos>, then runs 'git commit-graph verify' # and places the output in the file 'err'. Test 'err' for # the given string. corrupt_graph_and_verify() {
@@ -376,11 +377,15 @@ corrupt_graph_and_verify() { data="${2:-\0}" grepstr=$3 cd "$TRASH_DIRECTORY/full" && + orig_size=$(wc -c < $objdir/info/commit-graph) && + zero_pos=${4:-${orig_size}} && test_when_finished mv commit-graph-backup $objdir/info/commit-graph && cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 && + dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) && test_must_fail git commit-graph verify 2>test_err && - grep -v "^+" test_err >err + grep -v "^+" test_err >err && test_i18ngrep "$grepstr" err }
@@ -484,6 +489,11 @@ test_expect_success 'detect invalid checksum hash' ' "incorrect checksum" ' +test_expect_success 'detect incorrect chunk count' ' + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\377" \ + "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' + test_expect_success 'git fsck (checks commit-graph)' ' cd "$TRASH_DIRECTORY/full" && git fsck &&
Signature · 1 line
2.20.1.97.g81188d93c3-goog
Re: [PATCH v6 2/3] commit-graph: fix buffer read-overflow
Quoted 4 lines · Josh Steadmon
fuzz-commit-graph identified a case where Git will read past the end of a buffer containing a commit graph if the graph's header has an incorrect chunk count. A simple bounds check in
parse_commit_graph()prevents this.
This has a 2.21 regression where the test fails on NetBSD:
https://gitlab.com/git-vcs/git-ci/-/jobs/164224275
Quoted 51 lines
Signed-off-by Josh Steadmon <[email protected]>--- commit-graph.c | 14 ++++++++++++-- t/t5318-commit-graph.sh | 16 +++++++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-)commit-graph.c +12
-2@@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd, last_chunk_offset =8 ; chunk_lookup = data +8 ;for (i =0 ; i < graph->num_chunks; i++) { -uint32_t chunk_id =get_be32 (chunk_lookup +0 ); -uint64_t chunk_offset =get_be64 (chunk_lookup +4 ); +uint32_t chunk_id; +uint64_t chunk_offset;int chunk_repeated =0 ; +if (data + graph_size - chunk_lookup < +GRAPH_CHUNKLOOKUP_WIDTH ) { +error (_ ("chunk lookup table entry missing; graph file may be incomplete" )); +free (graph); +return NULL ; + } + + chunk_id =get_be32 (chunk_lookup +0 ); + chunk_offset =get_be64 (chunk_lookup +4 ); + chunk_lookup +=GRAPH_CHUNKLOOKUP_WIDTH ;if (chunk_offset > graph_size -GIT_MAX_RAWSZ ) {t/t5318-commit-graph.sh +3
-2@@ -366,9 +366,10 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES)) -# usage: corrupt_graph_and_verify <position> <data> <string> +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] # Manipulates the commit-graph file at the position -# by inserting the data, then runs 'git commit-graph verify' +# by inserting the data, optionally zeroing the file +# starting at <zero_pos>, then runs 'git commit-graph verify' # and places the output in the file 'err'. Test 'err' for # the given string. corrupt_graph_and_verify() {
t/t5318-commit-graph.sh corrupt_graph_and_verify() { · line 386
Quoted 10 lines
t/t5318-commit-graph.sh +4
-0@@ -376,11 +377,15 @@ corrupt_graph_and_verify() { data="${2:-\0}" grepstr=$3 cd "$TRASH_DIRECTORY/full" && + orig_size=$(wc -c < $objdir/info/commit-graph) && + zero_pos=${4:-${orig_size}} && test_when_finished mv commit-graph-backup $objdir/info/commit-graph && cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 && + dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) &&
In the limited time I had to dig it starts failing at test 46, when count=0 is given. dd on NetBSD exits with 127 when given count=0 it seems.
Quoted 7 lines
t/t5318-commit-graph.sh +1
-1test_must_fail git commit-graph verify 2>test_err && - grep -v "^+" test_err >err + grep -v "^+" test_err >err && test_i18ngrep "$grepstr" err }
t/t5318-commit-graph.sh test_expect_success 'detect invalid checksum hash' ' · line 496
Quoted 8 lines
t/t5318-commit-graph.sh +5
-0@@ -484,6 +489,11 @@ test_expect_success 'detect invalid checksum hash' ' "incorrect checksum" ' +test_expect_success 'detect incorrect chunk count' ' + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\377" \ + "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' +
Hacking around the above (e.g. "dd ... || :") makes all the failing tests pass except this new one, which I didn't dig into.
Re: [PATCH v6 2/3] commit-graph: fix buffer read-overflow
Quoted 7 lines · Ævar Arnfjörð Bjarmason
Quoted 7 lines
t/t5318-commit-graph.sh +2
-0@@ -376,11 +377,15 @@ corrupt_graph_and_verify() { data="${2:-\0}" grepstr=$3 cd "$TRASH_DIRECTORY/full" && + orig_size=$(wc -c < $objdir/info/commit-graph) && + zero_pos=${4:-${orig_size}} && test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
Quoted 8 lines
t/t5318-commit-graph.sh corrupt_graph_and_verify() { · line 386
Quoted 4 lines
t/t5318-commit-graph.sh +2
-0cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 && + dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) &&In the limited time I had to dig it starts failing at test 46, when count=0 is given. dd on NetBSD exits with 127 when given count=0 it seems.
So the first 'dd' is supposed to truncate the commit-graph file at $zero_pos. I don't think we need 'count=0' for that: in the absence of the 'if=...' operand, 'dd' reads from standard input, which is redirected from /dev/null in our test scripts, i.e. there is nothing to read, and, consequently, there is nothing to write, either.
Though not strictly necessary, I would feel more comfortable if 'if=/dev/null' would be explicitly specified, and even more so with a "# truncate at $zero_pos" comment above that command.
As to the second 'dd', I think we should not run it at all when count would be zero, i.e. when $orig_size = $zero_pos, because in combination with 'if=/dev/zero' it's asking for trouble. According to POSIX [1]:
count=n
Copy only n input blocks. If n is zero, it is unspecified whether no blocks or all blocks are copied.
Imagine a 'dd' that implements the second option: there are infinite blocks in /dev/zero to copy! OTOH, if an implementation chooses the first option (e.g. the usual Linux 'dd' from coreutils), then both of these 'dd' invocations will leave the commit-graph file as-is, so it doesn't matter whether we run them or not.
[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/dd.html
PATCH v6 3/3 Makefile: correct example fuzz build
In reply to Josh SteadmonThe comment explaining how to build the fuzzers was broken in 927c77e7d4d ("Makefile: use FUZZ_CXXFLAGS for linking fuzzers", 2018-11-14).
When building fuzzers, all .c files must be compiled with coverage tracing enabled. This is not possible when using only FUZZ_CXXFLAGS, as that flag is only applied to the fuzzers themselves. Switching back to CFLAGS fixes the issue.
--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
@@ -3104,7 +3104,7 @@ cover_db_html: cover_db # An example command to build against libFuzzer from LLVM 4.0.0: # # make CC=clang CXX=clang++ \ -# FUZZ_CXXFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ +# CFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \ # fuzz-all #
Signature · 1 line
2.20.1.97.g81188d93c3-goog