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

11 messages from 2010-02-09 to 2010-02-09. Participants: Jay Soffian, Junio C Hamano, Johannes Sixt.
Thread: https://gitlist.dev/t/22578

## Jay Soffian, 2010-02-09 03:48

Subject: [PATCH] blame: prevent a segv when -L given start > EOF
Message-ID: <1265687293-11168-1-git-send-email-jaysoffian@gmail.com>
URL: https://gitlist.dev/e/1265687293-11168-1-git-send-email-jaysoffian%40gmail.com

```
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

```

## Jay Soffian, 2010-02-09 03:50

Subject: Re: [PATCH] blame: prevent a segv when -L given start > EOF
Message-ID: <76718491002081950g52b3dae7ta22377dfdd3479df@mail.gmail.com>
URL: https://gitlist.dev/e/76718491002081950g52b3dae7ta22377dfdd3479df%40mail.gmail.com
In-Reply-To: <1265687293-11168-1-git-send-email-jaysoffian@gmail.com>

```
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, 2010-02-09 03:55

Subject: Re: [PATCH] blame: prevent a segv when -L given start > EOF
Message-ID: <7vtytrrrju.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vtytrrrju.fsf%40alter.siamese.dyndns.org
In-Reply-To: <1265687293-11168-1-git-send-email-jaysoffian@gmail.com>

```
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, 2010-02-09 04:00

Subject: Re: [PATCH] blame: prevent a segv when -L given start > EOF
Message-ID: <76718491002082000s7be5b32dob5b3dfe764b12da0@mail.gmail.com>
URL: https://gitlist.dev/e/76718491002082000s7be5b32dob5b3dfe764b12da0%40mail.gmail.com
In-Reply-To: <7vtytrrrju.fsf@alter.siamese.dyndns.org>

```
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.

```

## Jay Soffian, 2010-02-09 04:02

Subject: Re: [PATCH] blame: prevent a segv when -L given start > EOF
Message-ID: <76718491002082002n587c4b05x44e5f577cf17a8e1@mail.gmail.com>
URL: https://gitlist.dev/e/76718491002082002n587c4b05x44e5f577cf17a8e1%40mail.gmail.com
In-Reply-To: <76718491002082000s7be5b32dob5b3dfe764b12da0@mail.gmail.com>

```
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, 2010-02-09 04:09

Subject: Re: [PATCH] blame: prevent a segv when -L given start > EOF
Message-ID: <76718491002082009u6a94d628ncf060820d427df7c@mail.gmail.com>
URL: https://gitlist.dev/e/76718491002082009u6a94d628ncf060820d427df7c%40mail.gmail.com
In-Reply-To: <7vtytrrrju.fsf@alter.siamese.dyndns.org>

```
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.

```

## Junio C Hamano, 2010-02-09 05:55

Subject: Re: [PATCH] blame: prevent a segv when -L given start > EOF
Message-ID: <7vwrynq7fc.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vwrynq7fc.fsf%40alter.siamese.dyndns.org
In-Reply-To: <7vtytrrrju.fsf@alter.siamese.dyndns.org>

```
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.

```

## Johannes Sixt, 2010-02-09 07:43

Subject: Re: [PATCH] blame: prevent a segv when -L given start > EOF
Message-ID: <4B71122F.3040809@viscovery.net>
URL: https://gitlist.dev/e/4B71122F.3040809%40viscovery.net
In-Reply-To: <1265687293-11168-1-git-send-email-jaysoffian@gmail.com>

```
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, 2010-02-09 17:55

Subject: Re: [PATCH] blame: prevent a segv when -L given start > EOF
Message-ID: <76718491002090955g66eca03ar7438e57811616267@mail.gmail.com>
URL: https://gitlist.dev/e/76718491002090955g66eca03ar7438e57811616267%40mail.gmail.com
In-Reply-To: <4B71122F.3040809@viscovery.net>

```
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.

```

## Jay Soffian, 2010-02-09 17:59

Subject: [PATCH v2] blame: prevent a segv when -L given start > EOF
Message-ID: <1265738392-61503-1-git-send-email-jaysoffian@gmail.com>
URL: https://gitlist.dev/e/1265738392-61503-1-git-send-email-jaysoffian%40gmail.com
In-Reply-To: <4B71122F.3040809@viscovery.net>

```
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

```

## Junio C Hamano, 2010-02-09 18:06

Subject: Re: [PATCH] blame: prevent a segv when -L given start > EOF
Message-ID: <7vhbpqe11y.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vhbpqe11y.fsf%40alter.siamese.dyndns.org
In-Reply-To: <76718491002090955g66eca03ar7438e57811616267@mail.gmail.com>

```
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

```