patchapply: reallocate the postimage buffer when needed

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(-)

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

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.

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

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.

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

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);