[PATCH] apply: reallocate the postimage buffer when needed

[PATCH] apply: reallocate the postimage buffer when needed

[Carlos Martín Nieto]

The buffer in the postimage may become too small when whitespace fixes
are applied to the patch and update_pre_post_images might write past
the end of the buffer.

Teach the code to reallocate the buffer if needed. When it comes time
to free the buffer, do it directly on postimage.buf instead of the
newlines strbuf.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---

This was reported on IRC. Reproduction steps are at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=663338 and it
involves applying a patch whilst fixing whitespace changes.

Blame says Junio and Giuseppe were the last ones to touch this part of
the code, so there you go.

While this seems like a reasonable fix to me, it's the first time I've
looked at this part of the code, so there might be a better way of
growing the buffer to its final size. I considered adding a loop at
the beginning to determine the final size, but I'm unsure about which
lines actually get skipped.

 builtin/apply.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 389898f..8899b09 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2003,10 +2003,12 @@ static void update_pre_post_images(struct image *preimage,
 	 * in place (postlen==0) or not.
 	 */
 	old = postimage->buf;
-	if (postlen)
+	if (postlen) {
 		new = postimage->buf = xmalloc(postlen);
-	else
+		postimage->alloc = postlen;
+	} else {
 		new = old;
+	}
 	fixed = preimage->buf;
 	for (i = ctx = 0; i < postimage->nr; i++) {
 		size_t len = postimage->line[i].len;
@@ -2032,6 +2034,13 @@ static void update_pre_post_images(struct image *preimage,
 
 		/* and copy it in, while fixing the line length */
 		len = preimage->line[ctx].len;
+		if (postimage->alloc < (new - postimage->buf) + len) {
+			size_t post_len = new - postimage->buf;
+			postimage->buf = xrealloc(postimage->buf, post_len + len);
+			postimage->alloc = post_len + len;
+			new = postimage->buf + post_len;
+		}
+
 		memcpy(new, fixed, len);
 		new += len;
 		fixed += len;
@@ -2594,6 +2603,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
 	preimage.len = old - oldlines;
 	postimage.buf = newlines.buf;
 	postimage.len = newlines.len;
+	postimage.alloc = newlines.alloc;
 	preimage.line = preimage.line_allocated;
 	postimage.line = postimage.line_allocated;
 
@@ -2679,7 +2689,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
 	}
 
 	free(oldlines);
-	strbuf_release(&newlines);
+	free(postimage.buf);
 	free(preimage.line_allocated);
 	free(postimage.line_allocated);
 
-- 
1.7.10.rc0.17.g74595

Re: [PATCH] apply: reallocate the postimage buffer when needed

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> Blame says Junio and Giuseppe were the last ones to touch this part of
> the code, so there you go.

Whatever you do in your fix, this comment block needs to be updated:

	/*
	 * Adjust the common context lines in postimage. This can be
	 * done in-place when we are just doing whitespace fixing,
	 * which does not make the string grow, but needs a new buffer
	 * when ignoring whitespace causes the update, since in this case
	 * we could have e.g. tabs converted to multiple spaces.
	 * We trust the caller to tell us if the update can be done
	 * in place (postlen==0) or not.
	 */

The second sentence used to be true for a long time (if you indented
your line with too many spaces, we removed them and replaced with
fewer number of tabs; if you had spaces before a tab, we removed
them; if you added unnecessary whitespaces at the end, we removed
them), but ceased to be so when Python style "indent must be spaces"
was added.

So I think this either always needs to re-allocate, or the caller
has to tell it by other means than "!postlen" the need for
reallocation.

I wasn't involved in the "apply while ignoring whitespace
differences", so Giuseppe may be able to notice other mode of
beakages in this and fuzzy_matchlines() function.  The commit to be
stared at is 86c91f9 (git apply: option to ignore whitespace
differences, 2009-08-04).

Thanks.

Re: [PATCH] apply: reallocate the postimage buffer when needed

[Junio C Hamano]

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

> Carlos Martín Nieto <cmn@elego.de> writes:
>
>> Blame says Junio and Giuseppe were the last ones to touch this part of
>> the code, so there you go.
>
> Whatever you do in your fix, this comment block needs to be updated:
>
> 	/*
> 	 * Adjust the common context lines in postimage. This can be
> 	 * done in-place when we are just doing whitespace fixing,
> 	 * which does not make the string grow, but needs a new buffer
> 	 * when ignoring whitespace causes the update, since in this case
> 	 * we could have e.g. tabs converted to multiple spaces.
> 	 * We trust the caller to tell us if the update can be done
> 	 * in place (postlen==0) or not.
> 	 */
>
> The second sentence used to be true for a long time (if you indented
> your line with too many spaces, we removed them and replaced with
> fewer number of tabs; if you had spaces before a tab, we removed
> them; if you added unnecessary whitespaces at the end, we removed
> them), but ceased to be so when Python style "indent must be spaces"
> was added.
>
> So I think this either always needs to re-allocate, or the caller
> has to tell it by other means than "!postlen" the need for
> reallocation.
>
> I wasn't involved in the "apply while ignoring whitespace
> differences", so Giuseppe may be able to notice other mode of
> beakages in this and fuzzy_matchlines() function.  The commit to be
> stared at is 86c91f9 (git apply: option to ignore whitespace
> differences, 2009-08-04).

Looking at the call site of update_pre_post_images() again, I think
the call made from this part may also need to be updated for "indent
must be spaces" that was added by 3e3ec2a (whitespace: add
tab-in-indent error class, 2010-04-03) and 4e35c51 (whitespace: add
tab-in-indent support for --whitespace=fix, 2010-04-03):

	... code to look at preimage and img, checking if the lines
        ... match if the whitespace breakages in the preimage line
        ... were fixed is here.
	/*
	 * Yes, the preimage is based on an older version that still
	 * has whitespace breakages unfixed, and fixing them makes the
	 * hunk match.  Update the context lines in the postimage.
	 */
	fixed_buf = strbuf_detach(&fixed, &fixed_len);
	update_pre_post_images(preimage, postimage,
			       fixed_buf, fixed_len, 0);

The logic to match the preimage and img is correct and does not
overflow anything; the fixing of preimage and img is done with
ws_fix_copy() that uses strbuf.

But fixed_buf and fixed_len may be longer than the original length
of preimage buffer when "indent must be spaces" is in effect, and
that would mean context lines in the postimage may have to also
grow. The call to update_pre_post_images() must be telling how big
the postimage with fixed context lines will be, not passing 0 to say
"I know it will not grow", because that is no longer true these
days.

Re: [PATCH] apply: reallocate the postimage buffer when needed

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
> ...
> But fixed_buf and fixed_len may be longer than the original length
> of preimage buffer when "indent must be spaces" is in effect, and
> that would mean context lines in the postimage may have to also
> grow. The call to update_pre_post_images() must be telling how big
> the postimage with fixed context lines will be, not passing 0 to say
> "I know it will not grow", because that is no longer true these
> days.

Just in case you haven't noticed it on your machine, t4105 shows
that this patch breaks "git apply".

...
ok 5 - big offset

expecting success: 
		git checkout-index -f -q -u file &&
		git apply -C2 F0.diff &&
		test_cmp expect file
	
Context reduced to (2/2) to apply fragment at 6
*** glibc detected *** /git.git/git: free(): invalid pointer: 0x00000000020c09e2 ***
...
./test-lib.sh: line 492: 10555 Aborted                 git apply -C2 F0.diff
not ok - 6 fuzz with no offset
#	
#			git checkout-index -f -q -u file &&
#			git apply -C2 F0.diff &&
#			test_cmp expect file
#		

With --valgrind the same test dies like so:

==18735== Invalid free() / delete / delete[]
==18735==    at 0x4C240FD: free (vg_replace_malloc.c:366)
==18735==    by 0x40918A: apply_data (apply.c:2692)
==18735==    by 0x40A1C2: check_patch (apply.c:3163)
==18735==    by 0x40C9A4: apply_patch (apply.c:3178)
==18735==    by 0x40D9A1: cmd_apply (apply.c:3959)
==18735==    by 0x404DD6: handle_internal_command (git.c:308)
==18735==    by 0x404FFC: main (git.c:513)
==18735==  Address 0x55c7c62 is 2 bytes inside a block of size 52 alloc'd
==18735==    at 0x4C244E8: malloc (vg_replace_malloc.c:236)
==18735==    by 0x4C24562: realloc (vg_replace_malloc.c:525)
==18735==    by 0x4E19AD: xrealloc (wrapper.c:82)
==18735==    by 0x4CFD7A: strbuf_grow (strbuf.c:72)
==18735==    by 0x4082B6: apply_data (apply.c:2470)
==18735==    by 0x40A1C2: check_patch (apply.c:3163)
==18735==    by 0x40C9A4: apply_patch (apply.c:3178)
==18735==    by 0x40D9A1: cmd_apply (apply.c:3959)
==18735==    by 0x404DD6: handle_internal_command (git.c:308)
==18735==    by 0x404FFC: main (git.c:513)


where "apply_data (apply.c:2692)" refers to 

	free(postimage.buf);