threads / patch / 22578

patchblame: prevent a segv when -L given start > EOF

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

## tl;dr

11 messages between Feb 9, 2010 and Feb 9, 2010. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Jay Soffian· Feb 9, 2010, 03:48 UTC · lore

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(-)
Show changes to 2 files +9 −1

builtin-blame.c, t/t8003-blame.sh

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
Jay Soffian· Feb 9, 2010, 03:50 UTC · re: Jay Soffian · lore

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

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.
Junio C Hamano· Feb 9, 2010, 03:55 UTC · re: Jay Soffian · lore

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

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?

Jay Soffian· Feb 9, 2010, 04:00 UTC · re: Junio C Hamano · lore

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

On Mon, Feb 8, 2010 at 10:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> 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.
Jay Soffian· Feb 9, 2010, 04:02 UTC · re: Jay Soffian · lore

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

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.
Jay Soffian· Feb 9, 2010, 04:09 UTC · re: Junio C Hamano · lore

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

On Mon, Feb 8, 2010 at 10:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> 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.
Junio C Hamano· Feb 9, 2010, 05:55 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> 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.

Johannes Sixt· Feb 9, 2010, 07:43 UTC · re: Jay Soffian · lore

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

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
Jay Soffian· Feb 9, 2010, 17:55 UTC · re: Johannes Sixt · lore

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

On Tue, Feb 9, 2010 at 2:43 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 18 quoted lines
> 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.
Junio C Hamano· Feb 9, 2010, 18:06 UTC · re: Jay Soffian · lore

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

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(-)
Show changes to t/t8003-blame.sh +4 −2
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
Jay Soffian· Feb 9, 2010, 17:59 UTC · re: Johannes Sixt · lore

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

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(-)
Show changes to 2 files +11 −1

builtin-blame.c, t/t8003-blame.sh

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

← back to recent threads