Re: [PATCH v2 03/12] commit-graph: test that 'verify' finds corruption

From: Jakub Narebski <jnareb@gmail.com>
Date: May 21, 2018, 18:53 UTC
Message-ID: <86wovwdemu.fsf@gmail.com>
In-Reply-To: <20180511211504.79877-4-dstolee@microsoft.com>

Derrick Stolee <dstolee@microsoft.com> writes:

Show 25 quoted lines

> Add test cases to t5318-commit-graph.sh that corrupt the commit-graph
> file and check that the 'git commit-graph verify' command fails. These
> tests verify the header and chunk information is checked carefully.
>
> Helped-by: Martin Ågren <martin.agren@gmail.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t5318-commit-graph.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 6ca451dfd2..0cb88232fa 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -240,4 +240,57 @@ test_expect_success 'git commit-graph verify' '
>  	git commit-graph verify >output
>  '
>  
> +# usage: corrupt_data <file> <pos> [<data>]
> +corrupt_data() {
> +	file=$1
> +	pos=$2
> +	data="${3:-\0}"
> +	printf "$data" | dd of="$file" bs=1 seek="$pos" conv=notrunc
> +}

First, if we do this that way (and not by adding a test helper), the use of this function should be, I think, protected using appropriate test prerequisite. Not everyone has 'dd' tool installed, for example on MS Windows.

Second, the commit-graph file format has H-byte HASH-checksum of all of the contents excluding checksum trailer. It feels like any corruption should have been caught by checksum test; thus to actually test that contents is verified we should adjust checksum too, e.g. with sha1sum if available or with test helper... oh, actually we have t/helper/test-sha1. Unfortulately, it looks like it has no docs (beside commit message).

> +
> +test_expect_success 'detect bad signature' '
> +	cd "$TRASH_DIRECTORY/full" &&

This 'cd' outside subshell and withou accompanying change back feels a bit strange to me.

> +	cp $objdir/info/commit-graph commit-graph-backup &&
> +	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +	corrupt_data $objdir/info/commit-graph 0 "\0" &&

So 'CGPH' signature is currupted into '\0GPH'.

> +	test_must_fail git commit-graph verify 2>err &&
> +	grep -v "^\+" err > verify-errors &&

Minor nit: redirection should be cuddled to the file, i.e.:

  +	grep -v "^\+" err >verify-errors &&

A question: why do you filter-out lines starting with "+" here?

> +	test_line_count = 1 verify-errors &&
> +	grep "graph signature" verify-errors

If messages from 'git commit-graph verify' can be localized (are translatable), then it should be i18n_grep, isn't it?

Show 7 quoted lines

> +'
> +
> +test_expect_success 'detect bad version number' '
> +	cd "$TRASH_DIRECTORY/full" &&
> +	cp $objdir/info/commit-graph commit-graph-backup &&
> +	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +	corrupt_data $objdir/info/commit-graph 4 "\02" &&

All right, so we replace commit-graph format version 1 ("\01") with version 2 ("\02"). First, why 2 and not 0? Second, is "\02" portable?

> +	test_must_fail git commit-graph verify 2>err &&
> +	grep -v "^\+" err > verify-errors &&
> +	test_line_count = 1 verify-errors &&

The above three lines is common across all test cases; I wonder if it would be possible to extract it into function, to avoid code duplication.

Show 8 quoted lines

> +	grep "graph version" verify-errors
> +'
> +
> +test_expect_success 'detect bad hash version' '
> +	cd "$TRASH_DIRECTORY/full" &&
> +	cp $objdir/info/commit-graph commit-graph-backup &&
> +	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +	corrupt_data $objdir/info/commit-graph 5 "\02" &&

All right, so we change / corrupt hash version from value of 1, which means SHA-1, to value of 2... which would soon meen NewHash. Why not "\777" (i.e. 0xff)?

Show 5 quoted lines

> +	test_must_fail git commit-graph verify 2>err &&
> +	grep -v "^\+" err > verify-errors &&
> +	test_line_count = 1 verify-errors &&
> +	grep "hash version" verify-errors
> +'

Note: all of the above tests check in load_commit_graph_one(), not the
one in verify_commit_graph().  Just FYI.

Show 11 quoted lines

> +
> +test_expect_success 'detect too small chunk-count' '
> +	cd "$TRASH_DIRECTORY/full" &&
> +	cp $objdir/info/commit-graph commit-graph-backup &&
> +	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +	corrupt_data $objdir/info/commit-graph 6 "\01" &&
> +	test_must_fail git commit-graph verify 2>err &&
> +	grep -v "^\+" err > verify-errors &&
> +	test_line_count = 2 verify-errors &&
> +	grep "missing the OID Lookup chunk" verify-errors &&
> +	grep "missing the Commit Data chunk" verify-errors

This feels too implementation specific. We should have at least two chunks missing (there are 3 required chunks, and number of chunks was changed to 1), but commit-graph format specification does not state that OID Fanout must be first, and thus it is two remaining required chunks that would be missing.

> +'
> +
>  test_done

One test that I would like to see that 'git commit-grph verify' correctly detects without crashing is if commit-graph file gets truncated at various lengths: shorter than smallest possible commit-graph file size, in the middle of fixed header, in the middle of chunk lookup part, in the middle of chunk, just the trailer chopped off.

Best regards,

-- 
Jakub Narębski