{"thread":{"id":"29910","subject":"[PATCH] apply: reallocate the postimage buffer when needed","startedAt":"2012-03-11T14:24:17Z","lastAt":"2012-03-12T06:23:47Z","messageCount":4,"participants":["Carlos Martín Nieto","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"186653","messageId":"1331475857-15169-1-git-send-email-cmn@elego.de","threadId":"29910","inReplyTo":null,"subject":"[PATCH] apply: reallocate the postimage buffer when needed","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-03-11T14:24:17Z","receivedAt":"2012-03-11T14:24:17Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"The buffer in the postimage may become too small when whitespace fixes\nare applied to the patch and update_pre_post_images might write past\nthe end of the buffer.\n\nTeach the code to reallocate the buffer if needed. When it comes time\nto free the buffer, do it directly on postimage.buf instead of the\nnewlines strbuf.\n\nSigned-off-by: Carlos Martín Nieto <cmn@elego.de>\n---\n\nThis was reported on IRC. Reproduction steps are at\nhttp://bugs.debian.org/cgi-bin/bugreport.cgi?bug=663338 and it\ninvolves applying a patch whilst fixing whitespace changes.\n\nBlame says Junio and Giuseppe were the last ones to touch this part of\nthe code, so there you go.\n\nWhile this seems like a reasonable fix to me, it's the first time I've\nlooked at this part of the code, so there might be a better way of\ngrowing the buffer to its final size. I considered adding a loop at\nthe beginning to determine the final size, but I'm unsure about which\nlines actually get skipped.\n\n builtin/apply.c |   16 +++++++++++++---\n 1 file changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 389898f..8899b09 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -2003,10 +2003,12 @@ static void update_pre_post_images(struct image *preimage,\n \t * in place (postlen==0) or not.\n \t */\n \told = postimage->buf;\n-\tif (postlen)\n+\tif (postlen) {\n \t\tnew = postimage->buf = xmalloc(postlen);\n-\telse\n+\t\tpostimage->alloc = postlen;\n+\t} else {\n \t\tnew = old;\n+\t}\n \tfixed = preimage->buf;\n \tfor (i = ctx = 0; i < postimage->nr; i++) {\n \t\tsize_t len = postimage->line[i].len;\n@@ -2032,6 +2034,13 @@ static void update_pre_post_images(struct image *preimage,\n \n \t\t/* and copy it in, while fixing the line length */\n \t\tlen = preimage->line[ctx].len;\n+\t\tif (postimage->alloc < (new - postimage->buf) + len) {\n+\t\t\tsize_t post_len = new - postimage->buf;\n+\t\t\tpostimage->buf = xrealloc(postimage->buf, post_len + len);\n+\t\t\tpostimage->alloc = post_len + len;\n+\t\t\tnew = postimage->buf + post_len;\n+\t\t}\n+\n \t\tmemcpy(new, fixed, len);\n \t\tnew += len;\n \t\tfixed += len;\n@@ -2594,6 +2603,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n \tpreimage.len = old - oldlines;\n \tpostimage.buf = newlines.buf;\n \tpostimage.len = newlines.len;\n+\tpostimage.alloc = newlines.alloc;\n \tpreimage.line = preimage.line_allocated;\n \tpostimage.line = postimage.line_allocated;\n \n@@ -2679,7 +2689,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n \t}\n \n \tfree(oldlines);\n-\tstrbuf_release(&newlines);\n+\tfree(postimage.buf);\n \tfree(preimage.line_allocated);\n \tfree(postimage.line_allocated);\n \n-- \n1.7.10.rc0.17.g74595\n"},{"id":"186661","messageId":"7v8vj7x9jr.fsf@alter.siamese.dyndns.org","threadId":"29910","inReplyTo":"1331475857-15169-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] apply: reallocate the postimage buffer when needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-11T18:43:52Z","receivedAt":"2012-03-11T18:43:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto <cmn@elego.de> writes:\n\n> Blame says Junio and Giuseppe were the last ones to touch this part of\n> the code, so there you go.\n\nWhatever you do in your fix, this comment block needs to be updated:\n\n\t/*\n\t * Adjust the common context lines in postimage. This can be\n\t * done in-place when we are just doing whitespace fixing,\n\t * which does not make the string grow, but needs a new buffer\n\t * when ignoring whitespace causes the update, since in this case\n\t * we could have e.g. tabs converted to multiple spaces.\n\t * We trust the caller to tell us if the update can be done\n\t * in place (postlen==0) or not.\n\t */\n\nThe second sentence used to be true for a long time (if you indented\nyour line with too many spaces, we removed them and replaced with\nfewer number of tabs; if you had spaces before a tab, we removed\nthem; if you added unnecessary whitespaces at the end, we removed\nthem), but ceased to be so when Python style \"indent must be spaces\"\nwas added.\n\nSo I think this either always needs to re-allocate, or the caller\nhas to tell it by other means than \"!postlen\" the need for\nreallocation.\n\nI wasn't involved in the \"apply while ignoring whitespace\ndifferences\", so Giuseppe may be able to notice other mode of\nbeakages in this and fuzzy_matchlines() function.  The commit to be\nstared at is 86c91f9 (git apply: option to ignore whitespace\ndifferences, 2009-08-04).\n\nThanks.\n"},{"id":"186668","messageId":"7vipiax3il.fsf@alter.siamese.dyndns.org","threadId":"29910","inReplyTo":"7v8vj7x9jr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] apply: reallocate the postimage buffer when needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-11T20:54:10Z","receivedAt":"2012-03-11T20:54:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Carlos Martín Nieto <cmn@elego.de> writes:\n>\n>> Blame says Junio and Giuseppe were the last ones to touch this part of\n>> the code, so there you go.\n>\n> Whatever you do in your fix, this comment block needs to be updated:\n>\n> \t/*\n> \t * Adjust the common context lines in postimage. This can be\n> \t * done in-place when we are just doing whitespace fixing,\n> \t * which does not make the string grow, but needs a new buffer\n> \t * when ignoring whitespace causes the update, since in this case\n> \t * we could have e.g. tabs converted to multiple spaces.\n> \t * We trust the caller to tell us if the update can be done\n> \t * in place (postlen==0) or not.\n> \t */\n>\n> The second sentence used to be true for a long time (if you indented\n> your line with too many spaces, we removed them and replaced with\n> fewer number of tabs; if you had spaces before a tab, we removed\n> them; if you added unnecessary whitespaces at the end, we removed\n> them), but ceased to be so when Python style \"indent must be spaces\"\n> was added.\n>\n> So I think this either always needs to re-allocate, or the caller\n> has to tell it by other means than \"!postlen\" the need for\n> reallocation.\n>\n> I wasn't involved in the \"apply while ignoring whitespace\n> differences\", so Giuseppe may be able to notice other mode of\n> beakages in this and fuzzy_matchlines() function.  The commit to be\n> stared at is 86c91f9 (git apply: option to ignore whitespace\n> differences, 2009-08-04).\n\nLooking at the call site of update_pre_post_images() again, I think\nthe call made from this part may also need to be updated for \"indent\nmust be spaces\" that was added by 3e3ec2a (whitespace: add\ntab-in-indent error class, 2010-04-03) and 4e35c51 (whitespace: add\ntab-in-indent support for --whitespace=fix, 2010-04-03):\n\n\t... code to look at preimage and img, checking if the lines\n        ... match if the whitespace breakages in the preimage line\n        ... were fixed is here.\n\t/*\n\t * Yes, the preimage is based on an older version that still\n\t * has whitespace breakages unfixed, and fixing them makes the\n\t * hunk match.  Update the context lines in the postimage.\n\t */\n\tfixed_buf = strbuf_detach(&fixed, &fixed_len);\n\tupdate_pre_post_images(preimage, postimage,\n\t\t\t       fixed_buf, fixed_len, 0);\n\nThe logic to match the preimage and img is correct and does not\noverflow anything; the fixing of preimage and img is done with\nws_fix_copy() that uses strbuf.\n\nBut fixed_buf and fixed_len may be longer than the original length\nof preimage buffer when \"indent must be spaces\" is in effect, and\nthat would mean context lines in the postimage may have to also\ngrow. The call to update_pre_post_images() must be telling how big\nthe postimage with fixed context lines will be, not passing 0 to say\n\"I know it will not grow\", because that is no longer true these\ndays.\n"},{"id":"186681","messageId":"7v1uoyuyks.fsf@alter.siamese.dyndns.org","threadId":"29910","inReplyTo":"7vipiax3il.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] apply: reallocate the postimage buffer when needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-12T06:23:47Z","receivedAt":"2012-03-12T06:23:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> ...\n> But fixed_buf and fixed_len may be longer than the original length\n> of preimage buffer when \"indent must be spaces\" is in effect, and\n> that would mean context lines in the postimage may have to also\n> grow. The call to update_pre_post_images() must be telling how big\n> the postimage with fixed context lines will be, not passing 0 to say\n> \"I know it will not grow\", because that is no longer true these\n> days.\n\nJust in case you haven't noticed it on your machine, t4105 shows\nthat this patch breaks \"git apply\".\n\n...\nok 5 - big offset\n\nexpecting success: \n\t\tgit checkout-index -f -q -u file &&\n\t\tgit apply -C2 F0.diff &&\n\t\ttest_cmp expect file\n\t\nContext reduced to (2/2) to apply fragment at 6\n*** glibc detected *** /git.git/git: free(): invalid pointer: 0x00000000020c09e2 ***\n...\n./test-lib.sh: line 492: 10555 Aborted                 git apply -C2 F0.diff\nnot ok - 6 fuzz with no offset\n#\t\n#\t\t\tgit checkout-index -f -q -u file &&\n#\t\t\tgit apply -C2 F0.diff &&\n#\t\t\ttest_cmp expect file\n#\t\t\n\nWith --valgrind the same test dies like so:\n\n==18735== Invalid free() / delete / delete[]\n==18735==    at 0x4C240FD: free (vg_replace_malloc.c:366)\n==18735==    by 0x40918A: apply_data (apply.c:2692)\n==18735==    by 0x40A1C2: check_patch (apply.c:3163)\n==18735==    by 0x40C9A4: apply_patch (apply.c:3178)\n==18735==    by 0x40D9A1: cmd_apply (apply.c:3959)\n==18735==    by 0x404DD6: handle_internal_command (git.c:308)\n==18735==    by 0x404FFC: main (git.c:513)\n==18735==  Address 0x55c7c62 is 2 bytes inside a block of size 52 alloc'd\n==18735==    at 0x4C244E8: malloc (vg_replace_malloc.c:236)\n==18735==    by 0x4C24562: realloc (vg_replace_malloc.c:525)\n==18735==    by 0x4E19AD: xrealloc (wrapper.c:82)\n==18735==    by 0x4CFD7A: strbuf_grow (strbuf.c:72)\n==18735==    by 0x4082B6: apply_data (apply.c:2470)\n==18735==    by 0x40A1C2: check_patch (apply.c:3163)\n==18735==    by 0x40C9A4: apply_patch (apply.c:3178)\n==18735==    by 0x40D9A1: cmd_apply (apply.c:3959)\n==18735==    by 0x404DD6: handle_internal_command (git.c:308)\n==18735==    by 0x404FFC: main (git.c:513)\n\n\nwhere \"apply_data (apply.c:2692)\" refers to \n\n\tfree(postimage.buf);\n"}]}