Re: [PATCH] xdiff: avoid arithmetic overflow in xdl_get_hunk()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 14, 2025, 22:28 UTC
- Message-ID
- <xmqqikobdz7l.fsf@gitster.g>
- In-Reply-To
- <4e9b6b4c-aaa1-4c6f-93f4-7bb04607e843@web.de>
René Scharfe <l.s.r@web.de> writes:
Show 22 quoted lines
> xdl_get_hunk() calculates the maximum number of common lines between two > changes that would fit into the same hunk for the given context options. > It involves doubling and addition and thus can overflow if the terms are > huge. > > The type of ctxlen and interhunkctxlen in xdemitconf_t is long, while > the type of the corresponding context and interhunkcontext in struct > diff_options is int. On many platforms longs are bigger that ints, > which prevents the overflow. On Windows they have the same range and > the overflow manifests as hunks that are split erroneously and lines > being repeated between them. > > Fix the overflow by checking and not going beyond LONG_MAX. This allows > specifying a huge context line count and getting all lines of a changed > files in a single hunk, as expected. > > Reported-by: Jason Cho <jason11choca@proton.me> > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > 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" ;-)
Show 23 quoted lines
> diff --git a/xdiff/xemit.c b/xdiff/xemit.c
> index f8e3f25b03..1d40c9cb40 100644
> --- a/xdiff/xemit.c
> +++ b/xdiff/xemit.c
> @@ -43,6 +43,10 @@ static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *
> return 0;
> }
>
> +static long saturating_add(long a, long b)
> +{
> + return signed_add_overflows(a, b) ? LONG_MAX : a + b;
> +}
>
> /*
> * Starting at the passed change atom, find the latest change atom to be included
> @@ -52,7 +56,9 @@ static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *
> xdchange_t *xdl_get_hunk(xdchange_t **xscr, xdemitconf_t const *xecfg)
> {
> xdchange_t *xch, *xchp, *lxch;
> - long max_common = 2 * xecfg->ctxlen + xecfg->interhunkctxlen;
> + long max_common = saturating_add(saturating_add(xecfg->ctxlen,
> + xecfg->ctxlen),
> + xecfg->interhunkctxlen);Looking good.
Thanks. Will queue.