Re: [PATCH v2 2/3] commit-graph: fix buffer read-overflow

From: Derrick Stolee <stolee@gmail.com>
Date: Dec 7, 2018, 13:33 UTC
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:

Show 7 quoted lines

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

Show 5 quoted lines

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