Re: [External] Re: [PATCH v2 1/1] diffcore-break: prevent dangling pointer

[Junio C Hamano <gitster@pobox.com>]

Han Young <hanyang.tony@bytedance.com> writes:

>> Your "do not leave q->queue[] dangling, as other people may still
>> look at them" fix certainly is a good hygiene, but I have to wonder
>> why we are doing break detection in this case in the first place.
>> For the internal "Let's figure out which path have changed, so that
>> we re-read only those changed paths" invocation of diff machinery,
>> we should not be doing so.  A break detection is to see if the
>> change in the contents of a single path is a total rewrite, and
>> regardless of the answer, the fact that the path was modified does
>> not change, update_index_from_diff() would work on the path anyway.
>> I also suspect that, if we are doing rename detection in this call
>> to do_diff_cache(), it is a totally wasted effort.  We may want to
>> take a deeper look at it, possibly outside the theme of this more
>> focused fix.
>
> I'm not familiar with reset and diff machinery; I encountered this bug
> during a real world mixed reset. The segmentfault calling stack is
> cmd_reset -> read_from_tree -> diffcore_std -> diffcore_break
> It looks like rename detection is indeed pointless.
>
>> By the way, I find it highly curious that with the following patch
>> to revert the fix with a bit of extra output sprinkled to your
>> tests, the problem does not reproduce reliably, which may indicate
>> that your test may be flaky (i.e., timing dependent).  Am I doing
>> something bogus in the patch?
>
> It seems the problem does not reproduce reliably with or without your
> patch. I suspect that could be due to the freed memory on some
> occasions isn't reused by system, thus the access later on doesn't
> trigger a segment fault. On my macOS system, the test passes around
> 5% of the time. However, if I set q->queue[i] to a bogus memory
> location like 0x1 causes a Git segment fault every time.
> Is there a better way to write tests for this kind of situation?

If it is flaky because it depends on the way the system allocator
happens to reuse or not reuse a piece of memory, as long as we do
not get hit by false positives, it would be OK to leave it as-is if
we do not find a good solution, because running tests under asan
would catch such problems, I think.

Thanks.