[PATCH] blame: prevent a segv when -L given start > EOF

[PATCH] blame: prevent a segv when -L given start > EOF

[Jay Soffian]

blame would segv if given -L <lineno> with <lineno> past the end of the file.
While we're fixing the bug, add test cases for an invalid <start> when called
as -L <start>,<end> or -L<start>.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
bottom is start and top is end, which seems backwards to me, but alas, it is
what it is. :-)

 builtin-blame.c  |    2 +-
 t/t8003-blame.sh |    8 ++++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index 6408ec8..10f7eac 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2433,7 +2433,7 @@ parse_done:
 	if (top < 1)
 		top = lno;
 	bottom--;
-	if (lno < top)
+	if (lno < top || lno < bottom)
 		die("file %s has only %lu lines", path, lno);
 
 	ent = xcalloc(1, sizeof(*ent));
diff --git a/t/t8003-blame.sh b/t/t8003-blame.sh
index ad834f2..4a8db74 100755
--- a/t/t8003-blame.sh
+++ b/t/t8003-blame.sh
@@ -157,4 +157,12 @@ EOF
   git --no-pager blame $COMMIT -- uno >/dev/null
 '
 
+test_expect_success 'blame -L with invalid start' '
+	test_must_fail git blame -L5 tres 2>&1 | grep "has only 2 lines"
+'
+
+test_expect_success 'blame -L with invalid end' '
+	git blame -L1,5 tres 2>&1 | grep "has only 2 lines"
+'
+
 test_done
-- 
1.7.0.rc1.200.g9c1f9

Re: [PATCH] blame: prevent a segv when -L given start > EOF

[Jay Soffian]

On Mon, Feb 8, 2010 at 10:48 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> +test_expect_success 'blame -L with invalid start' '
> +       test_must_fail git blame -L5 tres 2>&1 | grep "has only 2 lines"
> +'

Sorry, please remove test_must_fail after applying. Otherwise correct.

j.

Re: [PATCH] blame: prevent a segv when -L given start > EOF

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> -	if (lno < top)
> +	if (lno < top || lno < bottom)
>  		die("file %s has only %lu lines", path, lno);

Thanks; I think we make sure that "bottom < top" always hold true before
we reach this point, so checking with bottom alone should suffice, no?

Re: [PATCH] blame: prevent a segv when -L given start > EOF

[Jay Soffian]

On Mon, Feb 8, 2010 at 10:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> -     if (lno < top)
>> +     if (lno < top || lno < bottom)
>>               die("file %s has only %lu lines", path, lno);
>
> Thanks; I think we make sure that "bottom < top" always hold true before
> we reach this point,

We swap them if they're reversed.

> so checking with bottom alone should suffice, no?

Ah true. You can squash that in? :-)

j.

Re: [PATCH] blame: prevent a segv when -L given start > EOF

[Jay Soffian]

On Mon, Feb 8, 2010 at 11:00 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
>> so checking with bottom alone should suffice, no?
>
> Ah true. You can squash that in? :-)

Actually, no, that doesn't work, but my head can't keep bottom=start,
top=end straight so I'm not even sure why not.

j.

Re: [PATCH] blame: prevent a segv when -L given start > EOF

[Jay Soffian]

On Mon, Feb 8, 2010 at 10:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> -     if (lno < top)
>> +     if (lno < top || lno < bottom)
>>               die("file %s has only %lu lines", path, lno);
>
> Thanks; I think we make sure that "bottom < top" always hold true before
> we reach this point, so checking with bottom alone should suffice, no?

Right, so given "-L 10" at the point of that check:

bottom = 0
top = 10

Whereas given "-L 10,100" at the point of that check:

bottom = 9
top = 100

So the code needs to check both.  Previously it made sure neither was
< 1, and swapped bottom/top if they were reversed.

Sorry for the confusion.

j.

Re: [PATCH] blame: prevent a segv when -L given start > EOF

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> -	if (lno < top)
>> +	if (lno < top || lno < bottom)
>>  		die("file %s has only %lu lines", path, lno);
>
> Thanks; I think we make sure that "bottom < top" always hold true before
> we reach this point, so checking with bottom alone should suffice, no?

I am ... stupid.  If lno < bottom, then lno < top, but we need to check
both anyway.

Re: [PATCH] blame: prevent a segv when -L given start > EOF

[Johannes Sixt]

Jay Soffian schrieb:
> +test_expect_success 'blame -L with invalid start' '
> +	test_must_fail git blame -L5 tres 2>&1 | grep "has only 2 lines"

Please write this as

	test_must_fail git blame -L5 tres >output 2>&1 &&
	grep "has only 2 lines" output

> +'
> +
> +test_expect_success 'blame -L with invalid end' '
> +	git blame -L1,5 tres 2>&1 | grep "has only 2 lines"

	test_must_fail git blame -L1,5 tres >output 2>&1 &&
	grep "has only 2 lines" output

because shells look only at the exit code of the last command in a pipeline.

-- Hannes

Re: [PATCH] blame: prevent a segv when -L given start > EOF

[Jay Soffian]

On Tue, Feb 9, 2010 at 2:43 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Jay Soffian schrieb:
>> +test_expect_success 'blame -L with invalid start' '
>> +     test_must_fail git blame -L5 tres 2>&1 | grep "has only 2 lines"
>
> Please write this as
>
>        test_must_fail git blame -L5 tres >output 2>&1 &&
>        grep "has only 2 lines" output
>
>> +'
>> +
>> +test_expect_success 'blame -L with invalid end' '
>> +     git blame -L1,5 tres 2>&1 | grep "has only 2 lines"
>
>        test_must_fail git blame -L1,5 tres >output 2>&1 &&
>        grep "has only 2 lines" output
>
> because shells look only at the exit code of the last command in a pipeline.

Thanks, I knew that. I'd left in test_must_fail accidentally because
initially I wasn't bothering to grep the output. I then added the grep
and forgot to remove test_must_fail. Isn't this an adequate test:

  test_expect_success 'blame -L with invalid start' '
     git blame -L5 tres 2>&1 | grep "has only 2 lines"

As it seems unlikely git would crash and still output the message
correctly in this case.

?

j.

Re: [PATCH] blame: prevent a segv when -L given start > EOF

[Junio C Hamano]

Sorry for knowing but forgetting about the issue while applying.
I'll queue this fix-up.

-- >8 --
t8003: check exit code of command and error message separately

Shell reports exit status only from the most downstream command
in a pipeline.  In these tests, we want to make sure that the
command fails in a controlled way, and produces a correct error
message.

This issue was known by Jay who submitted the patch, and also was
pointed out by Hannes during the review process, but I forgot to
fix it up before applying.  Sorry about that.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t8003-blame.sh |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t8003-blame.sh b/t/t8003-blame.sh
index 4a8db74..3bbddd0 100755
--- a/t/t8003-blame.sh
+++ b/t/t8003-blame.sh
@@ -158,11 +158,13 @@ EOF
 '
 
 test_expect_success 'blame -L with invalid start' '
-	test_must_fail git blame -L5 tres 2>&1 | grep "has only 2 lines"
+	test_must_fail git blame -L5 tres 2>errors &&
+	grep "has only 2 lines" errors
 '
 
 test_expect_success 'blame -L with invalid end' '
-	git blame -L1,5 tres 2>&1 | grep "has only 2 lines"
+	test_must_fail git blame -L1,5 tres 2>errors &&
+	grep "has only 2 lines" errors
 '
 
 test_done

[PATCH v2] blame: prevent a segv when -L given start > EOF

[Jay Soffian]

blame would segv if given -L <lineno> with <lineno> past the end of the file.
While we're fixing the bug, add test cases for an invalid <start> when called
as -L <start>,<end> or -L<start>.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Modified the tests per Hannes recommendations.

 builtin-blame.c  |    2 +-
 t/t8003-blame.sh |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index 6408ec8..10f7eac 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2433,7 +2433,7 @@ parse_done:
 	if (top < 1)
 		top = lno;
 	bottom--;
-	if (lno < top)
+	if (lno < top || lno < bottom)
 		die("file %s has only %lu lines", path, lno);
 
 	ent = xcalloc(1, sizeof(*ent));
diff --git a/t/t8003-blame.sh b/t/t8003-blame.sh
index ad834f2..be648b2 100755
--- a/t/t8003-blame.sh
+++ b/t/t8003-blame.sh
@@ -157,4 +157,14 @@ EOF
   git --no-pager blame $COMMIT -- uno >/dev/null
 '
 
+test_expect_success 'blame -L with invalid start' '
+	test_must_fail git blame -L5 tres >output 2>&1 &&
+	grep "has only 2 lines" output
+'
+
+test_expect_success 'blame -L with invalid end' '
+	test_must_fail git blame -L1,5 tres >output 2>&1 &&
+	grep "has only 2 lines" output
+'
+
 test_done
-- 
1.7.0.rc1.200.g9c1f9