threads / patch / 12046

patchapply: fix segfault

Subject: [PATCH] apply: fix segfault

## tl;dr

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

replies: 3people: 2as markdown or json

Johannes Schindelin· Feb 11, 2008, 22:28 UTC · lore

When the patch reports a line number that is larger than the number of lines in the current version of the file, git-apply used to segfault.

This fixes it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	This just happened to me.

 builtin-apply.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Show changes to builtin-apply.c +1 −1

diff --git a/builtin-apply.c b/builtin-apply.c
index 4578542..ca7096f 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1830,7 +1830,7 @@ static int find_pos(struct image *img,
 			return try_lno;
 
 	again:
-		if (backwards_lno == 0 && forwards_lno == img->nr)
+		if (backwards_lno == 0 && forwards_lno >= img->nr)
 			break;
 
 		if (i & 1) {

-- 
1.5.4.1264.gb53928

Junio C Hamano· Feb 11, 2008, 23:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH] apply: fix segfault

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> When the patch reports a line number that is larger than the number of
> lines in the current version of the file, git-apply used to segfault.

I have to wonder if the correct fix should be like this instead. Under that condition, I think computation of the initial "try" value already oversteps the line[] array for the original image.

Show changes to builtin-apply.c +3 −0

diff --git a/builtin-apply.c b/builtin-apply.c
index 2b8ba81..177f541 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1809,6 +1809,9 @@ static int find_pos(struct image *img,
 	else if (match_end)
 		line = img->nr - preimage->nr;
 
+	if (line > preimage->nr)
+		line = preimage->nr;
+
 	try = 0;
 	for (i = 0; i < line; i++)
 		try += img->line[i].len;

Junio C Hamano· Feb 11, 2008, 23:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] apply: fix segfault

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

Show 9 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> When the patch reports a line number that is larger than the number of
>> lines in the current version of the file, git-apply used to segfault.
>
> I have to wonder if the correct fix should be like this
> instead.  Under that condition, I think computation of the
> initial "try" value already oversteps the line[] array for the
> original image.

With tests...

 builtin-apply.c       |    3 ++
 t/t4105-apply-fuzz.sh |   60 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 0 deletions(-)

Show changes to 2 files +63 −0

builtin-apply.c, t/t4105-apply-fuzz.sh

diff --git a/builtin-apply.c b/builtin-apply.c
index 2b8ba81..177f541 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1809,6 +1809,9 @@ static int find_pos(struct image *img,
 	else if (match_end)
 		line = img->nr - preimage->nr;
 
+	if (line > preimage->nr)
+		line = preimage->nr;
+
 	try = 0;
 	for (i = 0; i < line; i++)
 		try += img->line[i].len;
diff --git a/t/t4105-apply-fuzz.sh b/t/t4105-apply-fuzz.sh
new file mode 100755
index 0000000..9c2a9f5
--- /dev/null
+++ b/t/t4105-apply-fuzz.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='apply with fuzz and offset'
+
+. ./test-lib.sh
+
+dotest () {
+	
+	name="$1" && shift &&
+
+	test_expect_success "$name" "
+		git checkout-index -f -q -u file &&
+		git apply $* &&
+		diff -u expect file
+	"
+
+}
+
+test_expect_success setup '
+
+	for i in 1 2 3 4 5 6 7 8 9 10 11 12
+	do
+		echo $i
+	done >file &&
+	git update-index --add file &&
+	for i in 1 2 3 4 5 6 7 a b c d e 8 9 10 11 12
+	do
+		echo $i
+	done >file &&
+	cat file >expect &&
+	git diff >O0.diff &&
+
+	sed -e "s/@@ -5,6 +5,11 @@/@@ -2,6 +2,11 @@/" >O1.diff O0.diff &&
+	sed -e "s/@@ -5,6 +5,11 @@/@@ -7,6 +7,11 @@/" >O2.diff O0.diff &&
+	sed -e "s/@@ -5,6 +5,11 @@/@@ -19,6 +19,11 @@/" >O3.diff O0.diff &&
+
+	sed -e "s/^ 5/ S/" >F0.diff O0.diff &&
+	sed -e "s/^ 5/ S/" >F1.diff O1.diff &&
+	sed -e "s/^ 5/ S/" >F2.diff O2.diff &&
+	sed -e "s/^ 5/ S/" >F3.diff O3.diff
+
+'
+
+dotest 'unmodified patch' O0.diff
+
+dotest 'minus offset' O1.diff
+
+dotest 'plus offset' O2.diff
+
+dotest 'big offset' O3.diff
+
+dotest 'fuzz with no offset' -C2 F0.diff
+
+dotest 'fuzz with minus offset' -C2 F1.diff
+
+dotest 'fuzz with plus offset' -C2 F2.diff
+
+dotest 'fuzz with big offset' -C2 F3.diff
+
+test_done

Junio C Hamano· Feb 11, 2008, 23:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] apply: fix segfault

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

Show 24 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> When the patch reports a line number that is larger than the number of
>> lines in the current version of the file, git-apply used to segfault.
>
> I have to wonder if the correct fix should be like this
> instead.  Under that condition, I think computation of the
> initial "try" value already oversteps the line[] array for the
> original image.
>
> diff --git a/builtin-apply.c b/builtin-apply.c
> index 2b8ba81..177f541 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -1809,6 +1809,9 @@ static int find_pos(struct image *img,
>  	else if (match_end)
>  		line = img->nr - preimage->nr;
>  
> +	if (line > preimage->nr)
> +		line = preimage->nr;
> +
>  	try = 0;
>  	for (i = 0; i < line; i++)
>  		try += img->line[i].len;

Sorry, obviously the check should be against img->nr not the preimage.