Re: [PATCH v4] reflog-walk: don't segfault on non-commit sha1's in the reflog

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

 



On Wed, Jan 6, 2016 at 4:13 PM, Dennis Kaarsemaker
<dennis@xxxxxxxxxxxxxxx> wrote:
> On di, 2016-01-05 at 20:52 -0500, Eric Sunshine wrote:
>> On Tue, Jan 5, 2016 at 8:28 PM, Eric Sunshine <
>> sunshine@xxxxxxxxxxxxxx> wrote:
>> > On Tue, Jan 5, 2016 at 8:20 PM, Dennis Kaarsemaker
>> > <dennis@xxxxxxxxxxxxxxx> wrote:
>> > > On di, 2016-01-05 at 20:05 -0500, Eric Sunshine wrote:
>> > > > Hmm, this test is successful for me on OS X even without the
>> > > > reflog-walk.c changes applied.
>> > > >
>> > > > And this test actually fails (inversely) because it's expecting
>> > > > a
>> > > > failure, but doesn't get one since the command produces the
>> > > > expected
>> > > > output.
>> > >
>> > > That's... surprising to say the least. What's the content of
>> > > 'actual',
>> > > and which git.git commit are you on?
>> >
>> > % cat t/trash\ directory.t1410-reflog/actual
>> > b60a214 refs/tests/tree-in-reflog@{0}: Restoring to commit
>> > 140c527 refs/tests/tree-in-reflog@{1}: Forcing tree
>> > b60a214 refs/tests/tree-in-reflog@{2}: Creating ref
>> > %
>> >
>> > This is with only the t/t1410-reflog.sh changes from your patch
>> > applied atop current 'master' (SHA1 7548842).
>>
>> By the way, the segfault does occur for me on Linux and FreeBSD.
>>
>> And, in all cases, on all tested platforms, with the full patch
>> applied, both tests behave sanely (in the expected fashion). So, even
>> though the crash doesn't manifest everywhere, the fact that the tests
>> are meaningfully testing it on the "affected" platforms may mean that
>> it's not worth worrying about why it doesn't segfault on OS X.
>>
>> (Of course, practicality aside, one might want to satisfy one's
>> intellectual curiosity about why it behaves differently on OS X.)
>
> The only explanation I can think of (and that's with practically no
> knowledge of OS X internals) is that OS X's memory allocation strategy
> is unlucky. Git is definitely writing to a location it should not write
> to. On linux and freebsd this is unallocated memory, so you get a
> segfault. On OS X, it happens to be memory actually allocated by git,
> resulting not in a segfault but in silent corruption of other in-memory
> data. I would argue that this is a much worse result, even though in
> this small test that corruption seems to not trigger a crash.

Agreed. For a guaranteed crash, put assert(c->object.type ==
OBJ_COMMIT); in the macro function "slabname## _at_peek" in
commit-slab.h. That is if I analyzed the crash correctly. I'm not
suggesting to put the assert() in the code permanently though because
I don't know how often this function is called.
-- 
Duy
--
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]