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