From: Derrick Stolee <stolee@gmail.com>
Date: Mon, 04 Jun 2018 13:08:59 GMT
Subject: Re: [PATCH v3 15/20] commit-graph: test for corrupted octopus edge
Message-ID: <7665886b-1343-ca9d-aa41-db7a5b7d2735@gmail.com>
In-Reply-To: <86tvqltla9.fsf@gmail.com>

On 6/2/2018 8:39 AM, Jakub Narebski wrote:
> Derrick Stolee <dstolee@microsoft.com> writes:
>
>> The commit-graph file has an extra chunk to store the parent int-ids for
>> parents beyond the first parent for octopus merges. Our test repo has a
>> single octopus merge that we can manipulate to demonstrate the 'verify'
>> subcommand detects incorrect values in that chunk.
> If I understand it correctly the above means that our _reading_ code
> checks for validity (which then 'git commit-graph verify' uses), just
> there were not any tests for that.
>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>   t/t5318-commit-graph.sh | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
>> index 58adb8246d..240aef6add 100755
>> --- a/t/t5318-commit-graph.sh
>> +++ b/t/t5318-commit-graph.sh
>> @@ -248,6 +248,7 @@ test_expect_success 'git commit-graph verify' '
>>   '
>>   
>>   NUM_COMMITS=9
>> +NUM_OCTOPUS_EDGES=2
>>   HASH_LEN=20
>>   GRAPH_BYTE_VERSION=4
>>   GRAPH_BYTE_HASH=5
>> @@ -274,6 +275,10 @@ GRAPH_BYTE_COMMIT_EXTRA_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 4`
>>   GRAPH_BYTE_COMMIT_WRONG_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 3`
>>   GRAPH_BYTE_COMMIT_GENERATION=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 8`
>>   GRAPH_BYTE_COMMIT_DATE=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 12`
>> +GRAPH_COMMIT_DATA_WIDTH=`expr $HASH_LEN + 16`
>> +GRAPH_OCTOPUS_DATA_OFFSET=`expr $GRAPH_COMMIT_DATA_OFFSET + \
>> +				$GRAPH_COMMIT_DATA_WIDTH \* $NUM_COMMITS`
>> +GRAPH_BYTE_OCTOPUS=`expr $GRAPH_OCTOPUS_DATA_OFFSET + 4`
>>   
>>   # usage: corrupt_graph_and_verify <position> <data> <string>
>>   # Manipulates the commit-graph file at the position
>> @@ -378,4 +383,9 @@ test_expect_success 'detect incorrect commit date' '
>>   		"commit date"
>>   '
>>   
>> +test_expect_success 'detect incorrect parent for octopus merge' '
>> +	corrupt_graph_and_verify $GRAPH_BYTE_OCTOPUS "\01" \
>> +		"invalid parent"
>> +'
> So we change the int-id to non-existing commit, and check that
> commit-graph code checks for that.
>
> What about the case when there are octopus merges, but no EDGE chunk
> (which I think we can emulate by changing / corrupting number of
> chunks)?
>
> What about the case where int-id of edge in EDGE chunk is correct, that
> is points to a valid commit, but does not agree with what is in the
> object database (what parents octopus merge has in reality)?
>
> Do we detect the situation where the second parent value in the commit
> data stores an array position within a Large Edge chunk, but we do not
> reach a value with the most-significant bit on when reaching the end of
> Large Edge chunk?

There are a few holes like this, but I think they are better suited to a 
follow-up series, as this series is already quite large.

-Stolee