Re: [PATCH] test: add git apply whitespace expansion tests

From: Kyle J. McKay <mackyle@gmail.com>
Date: Jan 19, 2015, 03:54 UTC
Message-ID: <AB9246EB-D720-4A4A-9AB7-4307613C19A3@gmail.com>
In-Reply-To: <CAPc5daXVk_ROUy7rmzS0aosWvE2wqw8tHZgomHHkay9CZjhbiw@mail.gmail.com>

On Jan 18, 2015, at 14:11, Junio C Hamano wrote:

Show 16 quoted lines

> On Sun, Jan 18, 2015 at 2:49 AM, Kyle J. McKay <mackyle@gmail.com>  
> 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