Re: [PATCH 1/1] untracked cache: fix off-by-one

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

Jeff King <peff@peff.net> writes:

> Yeah, every struct on a platform where FLEX_ARRAY is 1 ends up
> over-allocated by 1 byte. We could account for that in all of our flex
> allocations, but I don't it affects enough platforms to be worth the
> bother.

It was an explicit design decision to declare that the
overallocation was a non-problem back when we invented FLEX_ARRAY
macro.  We could probably have instead decided to pass number of
bytes to be stored in the flex-array (including NUL if that is a
character array) then subtract the value of FLEX_ARRAY if FLEX_ARRAY
were limited to only 0 or 1, but that was not workable as it is the
most natural to define FLEX_ARRAY to an empty string, i.e. making an
array field decl to "type field[];", for the more recent compilers.


>> As there is no obvious way to trigger this bug reliably, on all
>> platforms supported by Git, and as the bug is obvious enough, this patch
>> comes without a regression test.
>
> Makes sense. This code path should be well-covered by the existing tests
> anyway, so even if we could get those tools to trigger, I don't think
> there would be much point in adding a new test.

Yeah, it is already super that this obscure bug was spotted in the
first place.  It is true that this may regress without a test, but I
do not think it is reasonable to expect that it is possible to write
a reliable crash-detecting test for this one.

> The right thing is probably something like:
>
>   eos = memchr(data, '\0', end - data);
>   if (!eos)
> 	return error("malformed untracked cache extension");
>   len = eos - data;
>
> I wouldn't be at all surprised if other bits of the index code have the
> same issue, though. And at any rate, thinking about that should
> definitely not hold up your fix.

True, true.  I wonder if folks intereseted in libFuzzer can chime in
with something useful here, but that is totally independent.