Re: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Aug 5, 2013 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> A bounds checking bug allows the X in -LX to extend one line past the
>> end of file. For example, given a file with 5 lines, -L6 is accepted as
>> valid. Demonstrate this problem.
>>
>> While here, also add tests to check that the remaining cases of X and Y
>> in -LX,Y are handled correctly at and in the vicinity of end-of-file.
>>
>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>> ---
>>  t/annotate-tests.sh | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
>> index 3524eaf..02fbbf1 100644
>> --- a/t/annotate-tests.sh
>> +++ b/t/annotate-tests.sh
>> @@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' '
>>       check_count -L/99/,-3 B 1 B2 1 D 1
>>  '
>>
>> +# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than
>> +# git-blame sees, hence the last line is actually $(wc...)+1.
>> +test_expect_success 'blame -L X (X == nlines)' '
>> +     n=$(expr $(wc -l <file) + 1) &&
>> +     check_count -L$n C 1
>> +'
>> +
>> +test_expect_failure 'blame -L X (X == nlines + 1)' '
>> +     n=$(expr $(wc -l <file) + 2) &&
>> +     test_must_fail $PROG -L$n file
>> +'
>> +
>>  test_expect_success 'blame -L X (X > nlines)' '
>>       test_must_fail $PROG -L12345 file
>>  '
>> +test_expect_success 'blame -L ,Y (Y == nlines)' '
>> +     n=$(expr $(wc -l <file) + 1) &&
>> +     check_count -L,$n A 1 B 1 B1 1 B2 1 "A U Thor" 1 C 1 D 1 E 1
>> +'
>> +
>> +test_expect_success 'blame -L ,Y (Y == nlines + 1)' '
>> +     n=$(expr $(wc -l <file) + 2) &&
>> +     test_must_fail $PROG -L,$n file
>> +'
>> +
>
> This is somewhat curious.
>
> Does the problem triggers only on a file that ends with an
> incomplete line, or the test was inserted at this location only
> because it was convenient and the problem exists with or without the
> incomplete final line?
>
> I am guessing that it is the latter.

The problem exists with and without the incomplete line. The comment
mentioning "incomplete line" and "wc" was inserted to explain why it
was necessary to add one to the result of wc, which might not
otherwise be obvious.

The tests were inserted at this location because they are semantically
related to the "blame -L ,Y (Y > nlines)" test which already exists in
the file (quote just below this response).

>>  test_expect_success 'blame -L ,Y (Y > nlines)' '
>>       test_must_fail $PROG -L,12345 file
>>  '