From: Kyle J. McKay Date: Mon, 19 Jan 2015 03:54:45 GMT Subject: Re: [PATCH] test: add git apply whitespace expansion tests Message-ID: In-Reply-To: On Jan 18, 2015, at 14:11, Junio C Hamano wrote: > On Sun, Jan 18, 2015 at 2:49 AM, Kyle J. McKay > wrote: >> * Here's some tests. With "apply: make update_pre_post_images() >> sanity >> check the given postlen" but not "apply: count the size of postimage >> correctly" test 1/4 and 4/4 trigger the 'die("BUG: postlen...' but >> test 2/4 and 3/4 do not although they fail because git apply >> generates >> garbage. > > Do the failing cases that do not trigger the new postlen check fail > because the original (mis)counting thinks (incorrectly) that the > rewritten result _should_ fit within the original postlen (hence we > allow an in-place rewrite by passing postlen=0 to the helper), but > in fact after rewriting postimage->len ends up being longer due to > the miscounting? I'm not 100%, but I think so because: Before 250b3c6c (apply --whitespace=fix: avoid running over the postimage buffer, 2013-03-22), tests 1 and 4 tend to easily cause seg faults whereas 2 and 3 just give garbage. After 250b3c6c (apply --whitespace=fix: avoid running over the postimage buffer, 2013-03-22), tests 1 and 4 may pass without seg faulting although clearly there's some memory corruption going on because after "apply: make update_pre_post_images() sanity check the given postlen" they always die with the BUG message. I created the tests after reading your description in "apply: count the size of postimage correctly". I made a guess about what would trigger the problem -- I do not have a deep understanding of the builtin/apply.c code though. Tests 2 and 3 were attempts to make the discrepancy between counted and needed (assuming the "apply: count the size of postimage correctly" fix has not been applied) progressively worse and instead I ended up with a different kind of failure. Test 4 was then an alternate attempt to create a very large discrepancy and it ended up with BUG: values not that dissimilar from test 1. FYI, without the counting fix, test 1 causes "BUG: postlen = 390, used = 585" and test 4 causes "BUG: postlen = 396, used = 591". So while I did manage to increase the discrepancy a bit from the values you reported for clojure (postlen = 262, used = 273), I was actually aiming for a difference big enough to pretty much always guarantee a core dump. The garbage tests 2 and 3 produce without the counting fix is reminiscent of what you get when you use memcpy instead of memmove for an overlapping memory copy operation. A slightly modified version of these 4 tests can be run as far back a v1.7.4 (when apply --whitespace=fix started fixing tab-in-indent errors) and you get core dumps or garbage there too for all 4. So since I've not been able to get test 2 or 3 to core dump (even before 250b3c6c) I tend to believe you are correct in that the code thinks (incorrectly) that the result should fit within the buffer. I say buffer because the test 3 patch inserts 100 lines into a 6 line file and yet it never seems to cause a core dump (even in v1.7.4), so the buffer size must be based on the patch, not the original -- I'm sure that would make sense if I understood what's going on in the apply code. I did manage to create a test 5 that causes "BUG: postlen = 3036, used = 3542", but its verbose output has unfriendly long lines and it's fixed by the same counting fix as the others so it doesn't seem worthwhile to include it. -Kyle