{"thread":{"id":"63102","subject":"Iffy output given git diff --unified=2147483647","startedAt":"2025-03-12T15:51:10Z","lastAt":"2025-03-17T16:50:27Z","messageCount":7,"participants":["Jason Cho","René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"514069","messageId":"NYMqsJ7uttDzFT2OOEg5LLsxCSoQhTzqBs16KrMHGEKC7LzOAiYnYTEZavRQWqGH41UgjdwScwer7MssNzI7AEDHnD8GTBWvoBIqJ2e7D6g=@proton.me","threadId":"63102","inReplyTo":"xXWgbH3mlNEvFcdGLqBHwcclZoeZNPoLg8Hr6YCipHXvS5eKaHeTppzFM-l_wyB46BB1R1T0j6g_jWRXIj7-GRJh1LPxi1ta3GkQ5t8F4-0=@proton.me","subject":"Iffy output given git diff --unified=2147483647","fromName":"Jason Cho","fromEmail":"jason11choca@proton.me","sentAt":"2025-03-12T15:51:02Z","receivedAt":"2025-03-12T15:51:10Z","isPatch":false,"sender":{"key":"jason11choca@proton.me","avatar":null},"body":"> $ git --versiongit version 2.47.0.windows.2\n> \n> $ git diff --unified=2147483647 1.txt 2.txt\n> diff --git a/1.txt b/2.txt\n> index e53eaa1..1130481 100644\n> --- a/1.txt\n> +++ b/2.txt\n> @@ -1,10 +1,10 @@\n>  1\n> -2\n> +a\n>  3\n>  4\n>  5\n>  6\n>  7\n>  8\n>  a\n>  0\n> @@ -1,10 +1,10 @@\n>  1\n>  a\n>  3\n>  4\n>  5\n>  6\n>  7\n>  8\n> -9\n> +a\n>  0\n> \n> $ git diff --unified=3 1.txt 2.txt\n> diff --git a/1.txt b/2.txt\n> index e53eaa1..1130481 100644\n> --- a/1.txt\n> +++ b/2.txt\n> @@ -1,10 +1,10 @@\n>  1\n> -2\n> +a\n>  3\n>  4\n>  5\n>  6\n>  7\n>  8\n> -9\n> +a\n>  0\n> \n> $ diff --version\n> diff (GNU diffutils) 3.10\n> Copyright (C) 2023 Free Software Foundation, Inc.\n> License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>.\n> This is free software: you are free to change and redistribute it.\n> There is NO WARRANTY, to the extent permitted by law.\n> \n> Written by Paul Eggert, Mike Haertel, David Hayes,\n> Richard Stallman, and Len Tower.\n> \n> $ diff  --unified=2147483647 1.txt 2.txt\n> --- 1.txt       2025-03-12 16:04:06.947099900 +0100\n> +++ 2.txt       2025-03-12 16:04:27.131732400 +0100\n> @@ -1,10 +1,10 @@\n>  1\n> -2\n> +a\n>  3\n>  4\n>  5\n>  6\n>  7\n>  8\n> -9\n> +a\n>  0\n\n\nPlease see the above command line output. I run this on Windows with git for windows, but the problem should apply for other platforms. The version of my git is 2.47.\n\nI prepare two files, I run GNU diff  --unified=2147483647 1.txt 2.txt, the output is correct. Then I run git diff with --unified=2147483647, the context of the second hunk is repeated, which is unexpected.\n\nI investigated it and found the repetition is due to an overflow issue in   xdiff/xemit.c.\n\n\n> xdchange_t *xdl_get_hunk(xdchange_t **xscr, xdemitconf_t const *xecfg){\n> xdchange_t *xch, *xchp, *lxch;\n> long max_common = 2 * xecfg->ctxlen + xecfg->interhunkctxlen;  <- ----\n> ...\n> }\n\n\nThe documentation https://git-scm.com/docs/git-diff doesn't say the range of --unified. Even if its max value is INT_MAX, 2147483647 is in the range.\n\nCan you guys clarify the correct range of --unified? If my value 2147483647 is in range, git diff should output a diff without the strange repetition. Please fix it.\n\n\n\n"},{"id":"514323","messageId":"4e9b6b4c-aaa1-4c6f-93f4-7bb04607e843@web.de","threadId":"63102","inReplyTo":"NYMqsJ7uttDzFT2OOEg5LLsxCSoQhTzqBs16KrMHGEKC7LzOAiYnYTEZavRQWqGH41UgjdwScwer7MssNzI7AEDHnD8GTBWvoBIqJ2e7D6g=@proton.me","subject":"[PATCH] xdiff: avoid arithmetic overflow in xdl_get_hunk()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-03-14T22:00:42Z","receivedAt":"2025-03-14T22:00:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"xdl_get_hunk() calculates the maximum number of common lines between two\nchanges that would fit into the same hunk for the given context options.\nIt involves doubling and addition and thus can overflow if the terms are\nhuge.\n\nThe type of ctxlen and interhunkctxlen in xdemitconf_t is long, while\nthe type of the corresponding context and interhunkcontext in struct\ndiff_options is int.  On many platforms longs are bigger that ints,\nwhich prevents the overflow.  On Windows they have the same range and\nthe overflow manifests as hunks that are split erroneously and lines\nbeing repeated between them.\n\nFix the overflow by checking and not going beyond LONG_MAX.  This allows\nspecifying a huge context line count and getting all lines of a changed\nfiles in a single hunk, as expected.\n\nReported-by: Jason Cho <jason11choca@proton.me>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/t4055-diff-context.sh | 10 ++++++++++\n xdiff/xemit.c           |  8 +++++++-\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh\nindex f7ff234cf9..ec2804eea6 100755\n--- a/t/t4055-diff-context.sh\n+++ b/t/t4055-diff-context.sh\n@@ -89,4 +89,14 @@ test_expect_success '-U0 is valid, so is diff.context=0' '\n \tgrep \"^+MODIFIED\" output\n '\n\n+test_expect_success '-U2147483647 works' '\n+\techo APPENDED >>x &&\n+\ttest_line_count = 16 x &&\n+\tgit diff -U2147483647 >output &&\n+\ttest_line_count = 22 output &&\n+\tgrep \"^-ADDED\" output &&\n+\tgrep \"^+MODIFIED\" output &&\n+\tgrep \"^+APPENDED\" output\n+'\n+\n test_done\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex f8e3f25b03..1d40c9cb40 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -43,6 +43,10 @@ static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *\n \treturn 0;\n }\n\n+static long saturating_add(long a, long b)\n+{\n+\treturn signed_add_overflows(a, b) ? LONG_MAX : a + b;\n+}\n\n /*\n  * Starting at the passed change atom, find the latest change atom to be included\n@@ -52,7 +56,9 @@ static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *\n xdchange_t *xdl_get_hunk(xdchange_t **xscr, xdemitconf_t const *xecfg)\n {\n \txdchange_t *xch, *xchp, *lxch;\n-\tlong max_common = 2 * xecfg->ctxlen + xecfg->interhunkctxlen;\n+\tlong max_common = saturating_add(saturating_add(xecfg->ctxlen,\n+\t\t\t\t\t\t\txecfg->ctxlen),\n+\t\t\t\t\t xecfg->interhunkctxlen);\n \tlong max_ignorable = xecfg->ctxlen;\n \tlong ignored = 0; /* number of ignored blank lines */\n\n--\n2.48.1\n"},{"id":"514327","messageId":"xmqqikobdz7l.fsf@gitster.g","threadId":"63102","inReplyTo":"4e9b6b4c-aaa1-4c6f-93f4-7bb04607e843@web.de","subject":"Re: [PATCH] xdiff: avoid arithmetic overflow in xdl_get_hunk()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-14T22:28:14Z","receivedAt":"2025-03-14T22:28:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> xdl_get_hunk() calculates the maximum number of common lines between two\n> changes that would fit into the same hunk for the given context options.\n> It involves doubling and addition and thus can overflow if the terms are\n> huge.\n>\n> The type of ctxlen and interhunkctxlen in xdemitconf_t is long, while\n> the type of the corresponding context and interhunkcontext in struct\n> diff_options is int.  On many platforms longs are bigger that ints,\n> which prevents the overflow.  On Windows they have the same range and\n> the overflow manifests as hunks that are split erroneously and lines\n> being repeated between them.\n>\n> Fix the overflow by checking and not going beyond LONG_MAX.  This allows\n> specifying a huge context line count and getting all lines of a changed\n> files in a single hunk, as expected.\n>\n> Reported-by: Jason Cho <jason11choca@proton.me>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  t/t4055-diff-context.sh | 10 ++++++++++\n>  xdiff/xemit.c           |  8 +++++++-\n>  2 files changed, 17 insertions(+), 1 deletion(-)\n\nOh, I love a patch like this that is well thought out to carefully\ncheck the bounds, instead of blindly say \"ah, counting number of\nthings in size_t solves everything\" ;-)\n\n> diff --git a/xdiff/xemit.c b/xdiff/xemit.c\n> index f8e3f25b03..1d40c9cb40 100644\n> --- a/xdiff/xemit.c\n> +++ b/xdiff/xemit.c\n> @@ -43,6 +43,10 @@ static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *\n>  \treturn 0;\n>  }\n>\n> +static long saturating_add(long a, long b)\n> +{\n> +\treturn signed_add_overflows(a, b) ? LONG_MAX : a + b;\n> +}\n>\n>  /*\n>   * Starting at the passed change atom, find the latest change atom to be included\n> @@ -52,7 +56,9 @@ static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *\n>  xdchange_t *xdl_get_hunk(xdchange_t **xscr, xdemitconf_t const *xecfg)\n>  {\n>  \txdchange_t *xch, *xchp, *lxch;\n> -\tlong max_common = 2 * xecfg->ctxlen + xecfg->interhunkctxlen;\n> +\tlong max_common = saturating_add(saturating_add(xecfg->ctxlen,\n> +\t\t\t\t\t\t\txecfg->ctxlen),\n> +\t\t\t\t\t xecfg->interhunkctxlen);\n\nLooking good.\n\nThanks.  Will queue.\n\n"},{"id":"514330","messageId":"gq3mW6C-_CzvRWe7vlXuDni4d2hDajYwmVqqjBC9h-2HhGU9qiyvmzxveXEVwIhBE4X4vkVKsHHYWayWLnQ9gso8HHYbjXi2TxeZaaMnf0g=@proton.me","threadId":"63102","inReplyTo":"4e9b6b4c-aaa1-4c6f-93f4-7bb04607e843@web.de","subject":"Re: [PATCH] xdiff: avoid arithmetic overflow in xdl_get_hunk()","fromName":"Jason Cho","fromEmail":"jason11choca@proton.me","sentAt":"2025-03-14T23:14:59Z","receivedAt":"2025-03-14T23:15:07Z","isPatch":true,"sender":{"key":"jason11choca@proton.me","avatar":null},"body":"Dear René,\n\nWhat a thorough analysis. I didn't know this bug is only on Windows.\n\nThank you for submitting the fix. I am excited to see it in a new release.\n\nFor now I will cherrypick your patch to my fork of git.\n\nBest regards,\nJason Cho\n\n\nOn Friday, March 14th, 2025 at 11:00 PM, René Scharfe <l.s.r@web.de> wrote:\n\n> xdl_get_hunk() calculates the maximum number of common lines between two\n> changes that would fit into the same hunk for the given context options.\n> It involves doubling and addition and thus can overflow if the terms are\n> huge.\n> \n> The type of ctxlen and interhunkctxlen in xdemitconf_t is long, while\n> the type of the corresponding context and interhunkcontext in struct\n> diff_options is int. On many platforms longs are bigger that ints,\n> which prevents the overflow. On Windows they have the same range and\n> the overflow manifests as hunks that are split erroneously and lines\n> being repeated between them.\n> \n> Fix the overflow by checking and not going beyond LONG_MAX. This allows\n> specifying a huge context line count and getting all lines of a changed\n> files in a single hunk, as expected.\n> \n> Reported-by: Jason Cho jason11choca@proton.me\n> \n> Signed-off-by: René Scharfe l.s.r@web.de\n> \n> ---\n> t/t4055-diff-context.sh | 10 ++++++++++\n> xdiff/xemit.c | 8 +++++++-\n> 2 files changed, 17 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh\n> index f7ff234cf9..ec2804eea6 100755\n> --- a/t/t4055-diff-context.sh\n> +++ b/t/t4055-diff-context.sh\n> @@ -89,4 +89,14 @@ test_expect_success '-U0 is valid, so is diff.context=0' '\n> grep \"^+MODIFIED\" output\n> '\n> \n> +test_expect_success '-U2147483647 works' '\n> + echo APPENDED >>x &&\n> \n> + test_line_count = 16 x &&\n> + git diff -U2147483647 >output &&\n> \n> + test_line_count = 22 output &&\n> + grep \"^-ADDED\" output &&\n> + grep \"^+MODIFIED\" output &&\n> + grep \"^+APPENDED\" output\n> +'\n> +\n> test_done\n> diff --git a/xdiff/xemit.c b/xdiff/xemit.c\n> index f8e3f25b03..1d40c9cb40 100644\n> --- a/xdiff/xemit.c\n> +++ b/xdiff/xemit.c\n> @@ -43,6 +43,10 @@ static int xdl_emit_record(xdfile_t *xdf, long ri, char const pre, xdemitcb_t *\n> return 0;\n> }\n> \n> +static long saturating_add(long a, long b)\n> +{\n> + return signed_add_overflows(a, b) ? LONG_MAX : a + b;\n> +}\n> \n> /\n> * Starting at the passed change atom, find the latest change atom to be included\n> @@ -52,7 +56,9 @@ static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *\n> xdchange_t *xdl_get_hunk(xdchange_t **xscr, xdemitconf_t const *xecfg)\n> {\n> xdchange_t *xch, *xchp, *lxch;\n> - long max_common = 2 * xecfg->ctxlen + xecfg->interhunkctxlen;\n> \n> + long max_common = saturating_add(saturating_add(xecfg->ctxlen,\n> \n> + xecfg->ctxlen),\n> \n> + xecfg->interhunkctxlen);\n> \n> long max_ignorable = xecfg->ctxlen;\n> \n> long ignored = 0; /* number of ignored blank lines */\n> \n> --\n> 2.48.1\n"},{"id":"514349","messageId":"8c9a3966-2746-4619-9f77-ca95797dcab8@web.de","threadId":"63102","inReplyTo":"xmqqikobdz7l.fsf@gitster.g","subject":"Re: [PATCH] xdiff: avoid arithmetic overflow in xdl_get_hunk()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-03-15T06:38:59Z","receivedAt":"2025-03-15T06:44:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.03.25 um 23:28 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>>  t/t4055-diff-context.sh | 10 ++++++++++\n>>  xdiff/xemit.c           |  8 +++++++-\n>>  2 files changed, 17 insertions(+), 1 deletion(-)\n>\n> Oh, I love a patch like this that is well thought out to carefully\n> check the bounds, instead of blindly say \"ah, counting number of\n> things in size_t solves everything\" ;-)\n\nConverting xdiff from long to size_t is still a good idea, I think, but\nwould be lot more effort and thus more risky.  Comparisons to upstream\nwould become a lot more noisy as well.\n\nRené\n\n"},{"id":"514399","messageId":"xmqqiko8da63.fsf@gitster.g","threadId":"63102","inReplyTo":"8c9a3966-2746-4619-9f77-ca95797dcab8@web.de","subject":"Re: [PATCH] xdiff: avoid arithmetic overflow in xdl_get_hunk()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-16T19:53:40Z","receivedAt":"2025-03-16T19:53:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Am 14.03.25 um 23:28 schrieb Junio C Hamano:\n>> René Scharfe <l.s.r@web.de> writes:\n>>\n>>>  t/t4055-diff-context.sh | 10 ++++++++++\n>>>  xdiff/xemit.c           |  8 +++++++-\n>>>  2 files changed, 17 insertions(+), 1 deletion(-)\n>>\n>> Oh, I love a patch like this that is well thought out to carefully\n>> check the bounds, instead of blindly say \"ah, counting number of\n>> things in size_t solves everything\" ;-)\n>\n> Converting xdiff from long to size_t is still a good idea, I think, ...\n\nOh, no question about that, especially if the number of counted\nthings are somehow proportionally related to the size of in-core\nmemory regions in any way.\n\nWhat I do *not* like is the recent trend in the patches I see.  They\nstop thinking there once they blindly replace int or unsigned or\nwhatever with size_t and the compiler stops warning.  Even though\ncompiler warnings can be a useful tool when there is very little\nfalse positives, they are merely tools to improve the code, but I\nsee more and more confused patches that seem to think squelching\nwarnings is the goal in itself, without thinking if the resulting\ncode is actually improved.\n\nAnd I didn't see that in this patch.  The patch was actually written\nwith real goal of improving the code in mind.\n\n> but\n> would be lot more effort and thus more risky.  \n\nPerhaps, perhaps not.\n\n> Comparisons to upstream\n> would become a lot more noisy as well.\n\nI am not sure how much of that matters these days, though.  Are they\nstill active, or is the code perfect and pretty much done?  I somehow\nhad the impression it has been the latter for a long time...\n\nThanks.\n"},{"id":"514415","messageId":"136bbdac-aca2-411c-8367-8de4472fa858@web.de","threadId":"63102","inReplyTo":"xmqqiko8da63.fsf@gitster.g","subject":"Re: [PATCH] xdiff: avoid arithmetic overflow in xdl_get_hunk()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-03-17T16:50:14Z","receivedAt":"2025-03-17T16:50:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.03.25 um 20:53 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> Comparisons to upstream\n>> would become a lot more noisy as well.\n>\n> I am not sure how much of that matters these days, though.  Are they\n> still active, or is the code perfect and pretty much done?  I somehow\n> had the impression it has been the latter for a long time...\n\nhttp://www.xmailserver.org/xdiff-lib.html offers libxdiff-0.23.tar.gz,\nwhose entries bear the timestamp 2008-11-12.  Solid.\n\nRené\n\n"}]}