Re: [PATCH] xdiff: avoid arithmetic overflow in xdl_get_hunk()

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 16, 2025, 19:53 UTC
Message-ID: <xmqqiko8da63.fsf@gitster.g>
In-Reply-To: <8c9a3966-2746-4619-9f77-ca95797dcab8@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 12 quoted lines

> Am 14.03.25 um 23:28 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>>  t/t4055-diff-context.sh | 10 ++++++++++
>>>  xdiff/xemit.c           |  8 +++++++-
>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> Oh, I love a patch like this that is well thought out to carefully
>> check the bounds, instead of blindly say "ah, counting number of
>> things in size_t solves everything" ;-)
>
> Converting xdiff from long to size_t is still a good idea, I think, ...

Oh, no question about that, especially if the number of counted things are somehow proportionally related to the size of in-core memory regions in any way.

What I do *not* like is the recent trend in the patches I see. They stop thinking there once they blindly replace int or unsigned or whatever with size_t and the compiler stops warning. Even though compiler warnings can be a useful tool when there is very little false positives, they are merely tools to improve the code, but I see more and more confused patches that seem to think squelching warnings is the goal in itself, without thinking if the resulting code is actually improved.

And I didn't see that in this patch. The patch was actually written with real goal of improving the code in mind.

> but
> would be lot more effort and thus more risky.

Perhaps, perhaps not.

> Comparisons to upstream
> would become a lot more noisy as well.

I am not sure how much of that matters these days, though. Are they still active, or is the code perfect and pretty much done? I somehow had the impression it has been the latter for a long time...

Thanks.