From: Derrick Stolee <stolee@gmail.com>
Date: Fri, 07 Dec 2018 13:33:34 GMT
Subject: Re: [PATCH v2 2/3] commit-graph: fix buffer read-overflow
Message-ID: <7d3693c7-3926-b09d-fa17-faf5aa1dcde9@gmail.com>
In-Reply-To: <af45c2337fbe2a59ac95aff3ce90a69d8c30416f.1544127439.git.steadmon@google.com>

On 12/6/2018 3:20 PM, Josh Steadmon wrote:
> +
> +# 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?

> +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