Re: [PATCH] xdiff: avoid arithmetic overflow in xdl_get_hunk()
- From
- Jason Cho <jason11choca@proton.me>
- Date
- Mar 14, 2025, 23:14 UTC
- Message-ID
- <gq3mW6C-_CzvRWe7vlXuDni4d2hDajYwmVqqjBC9h-2HhGU9qiyvmzxveXEVwIhBE4X4vkVKsHHYWayWLnQ9gso8HHYbjXi2TxeZaaMnf0g=@proton.me>
- In-Reply-To
- <4e9b6b4c-aaa1-4c6f-93f4-7bb04607e843@web.de>
Dear René,
What a thorough analysis. I didn't know this bug is only on Windows.
Thank you for submitting the fix. I am excited to see it in a new release.
For now I will cherrypick your patch to my fork of git.
Best regards, Jason Cho
On Friday, March 14th, 2025 at 11:00 PM, René Scharfe <l.s.r@web.de> wrote:
Show 79 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(-)
>
> diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
> index f7ff234cf9..ec2804eea6 100755
> --- a/t/t4055-diff-context.sh
> +++ b/t/t4055-diff-context.sh
> @@ -89,4 +89,14 @@ test_expect_success '-U0 is valid, so is diff.context=0' '
> grep "^+MODIFIED" output
> '
>
> +test_expect_success '-U2147483647 works' '
> + echo APPENDED >>x &&
>
> + test_line_count = 16 x &&
> + git diff -U2147483647 >output &&
>
> + test_line_count = 22 output &&
> + grep "^-ADDED" output &&
> + grep "^+MODIFIED" output &&
> + grep "^+APPENDED" output
> +'
> +
> test_done
> 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);
>
> long max_ignorable = xecfg->ctxlen;
>
> long ignored = 0; /* number of ignored blank lines */
>
> --
> 2.48.1