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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

On Sun, Jan 18, 2015 at 2:49 AM, Kyle J. McKay <mackyle@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]