Re: [PATCH 1/2] commit-graph, fuzz: Add fuzzer for commit-graph
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 6, 2018, 01:32 UTC
- Message-ID
- <xmqqmupja19b.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <53e62baaa8769bf8e90991e32e0d123cc6629559.1544048946.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
Show 5 quoted lines
> @@ -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).
Show 13 quoted lines
> + 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.
Show 16 quoted lines
> +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;
> +
> + /*
> + * 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.
> + 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.
> data = (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.
Show 33 quoted lines
> ...
> -
> -cleanup_fail:
> - munmap(graph_map, graph_size);
> - close(fd);
> - exit(1);
> }
>
> static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
> diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c
> new file mode 100644
> index 0000000000..420851d0d2
> --- /dev/null
> +++ b/fuzz-commit-graph.c
> @@ -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;
> +}