{"thread":{"id":"63511","subject":"[PATCH] Fix buffer underflow in xdl_build_script","startedAt":"2025-05-23T20:51:45Z","lastAt":"2025-05-24T13:53:36Z","messageCount":5,"participants":["Alex via GitGitGadget","René Scharfe","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518804","messageId":"pull.1976.git.git.1748033500935.gitgitgadget@gmail.com","threadId":"63511","inReplyTo":null,"subject":"[PATCH] Fix buffer underflow in xdl_build_script","fromName":"Alex via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-23T20:51:40Z","receivedAt":"2025-05-23T20:51:45Z","isPatch":true,"sender":{"key":"ajb44.geo@yahoo.com","avatar":null},"body":"From: jinyaoguo <guo846@purdue.edu>\n\nThe loop in xdl_build_script used `i1 >= 0 || i2 >= 0`, causing\n`i1` (or `i2`) to reach 0 and then access `rchg1[i1-1]` (or\n`rchg2[i2-1]`), which underflows the buffer.\nThis commit adds explicit `i1 > 0` and `i2 > 0` checks around\nthose array accesses to prevent invalid negative indexing.\n\nSigned-off-by: Alex Guo <alexguo1023@gmail.com>\n---\n    Fix buffer underflow in xdl_build_script\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1976%2Fmugitya03%2Fbuf-1-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1976/mugitya03/buf-1-v1\nPull-Request: https://github.com/git/git/pull/1976\n\n xdiff/xdiffi.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 5a96e36dfbe..2e983965328 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -951,9 +951,10 @@ int xdl_build_script(xdfenv_t *xe, xdchange_t **xscr) {\n \t * Trivial. Collects \"groups\" of changes and creates an edit script.\n \t */\n \tfor (i1 = xe->xdf1.nrec, i2 = xe->xdf2.nrec; i1 >= 0 || i2 >= 0; i1--, i2--)\n-\t\tif (rchg1[i1 - 1] || rchg2[i2 - 1]) {\n-\t\t\tfor (l1 = i1; rchg1[i1 - 1]; i1--);\n-\t\t\tfor (l2 = i2; rchg2[i2 - 1]; i2--);\n+\t\tif ((i1 > 0 && rchg1[i1 - 1]) ||\n+\t\t\t(i2 > 0 && rchg2[i2 - 1])) {\n+\t\t\tfor (l1 = i1; i1 > 0 && rchg1[i1 - 1]; i1--);\n+            for (l2 = i2; i2 > 0 && rchg2[i2 - 1]; i2--);\n \n \t\t\tif (!(xch = xdl_add_change(cscr, i1, i2, l1 - i1, l2 - i2))) {\n \t\t\t\txdl_free_script(cscr);\n\nbase-commit: 8613c2bb6cd16ef530dc5dd74d3b818a1ccbf1c0\n-- \ngitgitgadget\n"},{"id":"518828","messageId":"037b4612-ea92-45bf-bacb-ba66204f941e@web.de","threadId":"63511","inReplyTo":"pull.1976.git.git.1748033500935.gitgitgadget@gmail.com","subject":"Re: [PATCH] Fix buffer underflow in xdl_build_script","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-05-24T05:57:27Z","receivedAt":"2025-05-24T05:57:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.05.25 um 22:51 schrieb Alex via GitGitGadget:\n> From: jinyaoguo <guo846@purdue.edu>\n> \n> The loop in xdl_build_script used `i1 >= 0 || i2 >= 0`, causing\n> `i1` (or `i2`) to reach 0 and then access `rchg1[i1-1]` (or\n> `rchg2[i2-1]`), which underflows the buffer.\n> This commit adds explicit `i1 > 0` and `i2 > 0` checks around\n> those array accesses to prevent invalid negative indexing.\n\nxdl_prepare_ctx() in xdiff/xprepare.c allocates an extra entry at both\nends for rchg arrays, so an index of -1 should be within the bounds.  \n\ni1 and i2 are decreased in lockstep, though, so one of them can become\nsmaller than -1 if nrec is different between the files.  And that's how\nthis code run can indeed run off into the weeds.\n\nCuriously, AddressSanitizer doesn't report anything, but if I add the\nfollowing line after the outer for, I can trigger it to report a\nheap-buffer-overflow with e.g., git show 8613c2bb6c:\n\n\tif (i1 < 0 || i2 < 0) fprintf(stderr, \"Oops: %ld %ld\\n\", i1, i2);\n\n> \n> Signed-off-by: Alex Guo <alexguo1023@gmail.com>\n> ---\n>     Fix buffer underflow in xdl_build_script\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1976%2Fmugitya03%2Fbuf-1-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1976/mugitya03/buf-1-v1\n> Pull-Request: https://github.com/git/git/pull/1976\n> \n>  xdiff/xdiffi.c | 7 ++++---\n>  1 file changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n> index 5a96e36dfbe..2e983965328 100644\n> --- a/xdiff/xdiffi.c\n> +++ b/xdiff/xdiffi.c\n> @@ -951,9 +951,10 @@ int xdl_build_script(xdfenv_t *xe, xdchange_t **xscr) {\n>  \t * Trivial. Collects \"groups\" of changes and creates an edit script.\n>  \t */\n>  \tfor (i1 = xe->xdf1.nrec, i2 = xe->xdf2.nrec; i1 >= 0 || i2 >= 0; i1--, i2--)\n\nShould the || be a && instead?  From a birds-eye view I would assume we\ncan stop scanning for changes when we exhaust (reach the top) of either\nside.  We just have to make sure everything from the other side is\naccounted for in the last added change.\n\n> -\t\tif (rchg1[i1 - 1] || rchg2[i2 - 1]) {\n> -\t\t\tfor (l1 = i1; rchg1[i1 - 1]; i1--);\n> -\t\t\tfor (l2 = i2; rchg2[i2 - 1]; i2--);\n> +\t\tif ((i1 > 0 && rchg1[i1 - 1]) ||\n> +\t\t\t(i2 > 0 && rchg2[i2 - 1])) {\n> +\t\t\tfor (l1 = i1; i1 > 0 && rchg1[i1 - 1]; i1--);\n> +            for (l2 = i2; i2 > 0 && rchg2[i2 - 1]; i2--);\n\nNit: The indentation of that line is off.\n\n>  \n>  \t\t\tif (!(xch = xdl_add_change(cscr, i1, i2, l1 - i1, l2 - i2))) {\n>  \t\t\t\txdl_free_script(cscr);\n> \n> base-commit: 8613c2bb6cd16ef530dc5dd74d3b818a1ccbf1c0\n\n"},{"id":"518840","messageId":"a58bf04a-d9fc-4ecf-96a5-82d31ada510d@web.de","threadId":"63511","inReplyTo":"037b4612-ea92-45bf-bacb-ba66204f941e@web.de","subject":"Re: [PATCH] Fix buffer underflow in xdl_build_script","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-05-24T09:08:42Z","receivedAt":"2025-05-24T09:08:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.05.25 um 07:57 schrieb René Scharfe:\n> Am 23.05.25 um 22:51 schrieb Alex via GitGitGadget:\n>> From: jinyaoguo <guo846@purdue.edu>\n>>\n>> The loop in xdl_build_script used `i1 >= 0 || i2 >= 0`, causing\n>> `i1` (or `i2`) to reach 0 and then access `rchg1[i1-1]` (or\n>> `rchg2[i2-1]`), which underflows the buffer.\n>> This commit adds explicit `i1 > 0` and `i2 > 0` checks around\n>> those array accesses to prevent invalid negative indexing.\n> \n> xdl_prepare_ctx() in xdiff/xprepare.c allocates an extra entry at both\n> ends for rchg arrays, so an index of -1 should be within the bounds.  \n> \n> i1 and i2 are decreased in lockstep, though, so one of them can become\n> smaller than -1 if nrec is different between the files.  And that's how\n> this code run can indeed run off into the weeds.\n\nActually no, i1 can't seem to reach 0 without i2 also being 0 and vice\nversa.  Or can it?  It makes sense that we reach the start of both\nbuffers at the same time if we walk backwards from the end, don't\nmisstep and have consistent rchg array contents, but I'm not sure.\n\nAre you able to demonstrate any out-of-bounds access with e.g.,\nValgrind, AddressSanitizer or an assertion?\n\n> Curiously, AddressSanitizer doesn't report anything, but if I add the\n> following line after the outer for, I can trigger it to report a\n> heap-buffer-overflow with e.g., git show 8613c2bb6c:\n> \n> \tif (i1 < 0 || i2 < 0) fprintf(stderr, \"Oops: %ld %ld\\n\", i1, i2);\n\nThat's because I forgot to add braces.  D'oh!  I can't trigger any\nout-of-bounds access or that Oops with them properly in place.  So I\nlet myself get fooled by a daring coding style. :-|\n\n> \n>>\n>> Signed-off-by: Alex Guo <alexguo1023@gmail.com>\n>> ---\n>>     Fix buffer underflow in xdl_build_script\n>>\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1976%2Fmugitya03%2Fbuf-1-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1976/mugitya03/buf-1-v1\n>> Pull-Request: https://github.com/git/git/pull/1976\n>>\n>>  xdiff/xdiffi.c | 7 ++++---\n>>  1 file changed, 4 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n>> index 5a96e36dfbe..2e983965328 100644\n>> --- a/xdiff/xdiffi.c\n>> +++ b/xdiff/xdiffi.c\n>> @@ -951,9 +951,10 @@ int xdl_build_script(xdfenv_t *xe, xdchange_t **xscr) {\n>>  \t * Trivial. Collects \"groups\" of changes and creates an edit script.\n\nTrivial for Davide perhaps (libxdiff author), but not my mushy brain..\n\n>>  \t */\n>>  \tfor (i1 = xe->xdf1.nrec, i2 = xe->xdf2.nrec; i1 >= 0 || i2 >= 0; i1--, i2--)\n> \n> Should the || be a && instead?  From a birds-eye view I would assume we\n> can stop scanning for changes when we exhaust (reach the top) of either\n> side.  We just have to make sure everything from the other side is\n> accounted for in the last added change.\n> \n>> -\t\tif (rchg1[i1 - 1] || rchg2[i2 - 1]) {\n>> -\t\t\tfor (l1 = i1; rchg1[i1 - 1]; i1--);\n>> -\t\t\tfor (l2 = i2; rchg2[i2 - 1]; i2--);\n>> +\t\tif ((i1 > 0 && rchg1[i1 - 1]) ||\n>> +\t\t\t(i2 > 0 && rchg2[i2 - 1])) {\n>> +\t\t\tfor (l1 = i1; i1 > 0 && rchg1[i1 - 1]; i1--);\n>> +            for (l2 = i2; i2 > 0 && rchg2[i2 - 1]; i2--);\n> \n> Nit: The indentation of that line is off.\n> \n>>  \n>>  \t\t\tif (!(xch = xdl_add_change(cscr, i1, i2, l1 - i1, l2 - i2))) {\n>>  \t\t\t\txdl_free_script(cscr);\n>>\n>> base-commit: 8613c2bb6cd16ef530dc5dd74d3b818a1ccbf1c0\n> \n> \n\n"},{"id":"518843","messageId":"2b9c5e91-67ab-4e46-93c4-15c8b79841be@gmail.com","threadId":"63511","inReplyTo":"a58bf04a-d9fc-4ecf-96a5-82d31ada510d@web.de","subject":"Re: [PATCH] Fix buffer underflow in xdl_build_script","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-24T13:38:26Z","receivedAt":"2025-05-24T13:38:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/05/2025 10:08, René Scharfe wrote:\n> Am 24.05.25 um 07:57 schrieb René Scharfe:\n>> Am 23.05.25 um 22:51 schrieb Alex via GitGitGadget:\n>>> From: jinyaoguo <guo846@purdue.edu>\n>>>\n>>> The loop in xdl_build_script used `i1 >= 0 || i2 >= 0`, causing\n>>> `i1` (or `i2`) to reach 0 and then access `rchg1[i1-1]` (or\n>>> `rchg2[i2-1]`), which underflows the buffer.\n>>> This commit adds explicit `i1 > 0` and `i2 > 0` checks around\n>>> those array accesses to prevent invalid negative indexing.\n>>\n>> xdl_prepare_ctx() in xdiff/xprepare.c allocates an extra entry at both\n>> ends for rchg arrays, so an index of -1 should be within the bounds.\n\nand rchg[-1] == 0 so i1 and i2 can never drop below -1\n\n>> i1 and i2 are decreased in lockstep, though, so one of them can become\n>> smaller than -1 if nrec is different between the files.  And that's how\n>> this code run can indeed run off into the weeds.\n> \n> Actually no, i1 can't seem to reach 0 without i2 also being 0 and vice\n> versa.  Or can it?  It makes sense that we reach the start of both\n> buffers at the same time if we walk backwards from the end, don't\n> misstep and have consistent rchg array contents, but I'm not sure.\nThe code looks like\n\n\tfor (i1 = xe->xdf1.nrec, i2 = xe->xdf2.nrec; i1 >= 0 || i2 >= 0; i1--, \ni2--)\n\t\tif (rchg1[i1 - 1] || rchg2[i2 - 1]) {\n\t\t\tfor (l1 = i1; rchg1[i1 - 1]; i1--);\n\t\t\tfor (l2 = i2; rchg2[i2 - 1]; i2--);\n\nI think I've convinced myself that it is safe assuming there are an \nequal number of unchanged lines in rchg1 and rchg2 and rchg1[-1] == \nrchg2[-1] == 0. Each iteration consumes any changed lines in the \npreimage and the postimage plus a single context line from each (apart \nfrom the final iteration when the context lines may have been \nexhausted). At the start of the last iteration there are three possibilities\n\n  - i1 == -1 && i2 >= 0 => there are insertions as the start of the file.\n    As the context lines in the preimage have been exhausted all the\n    remaining rchg2 elements represent added lines and are consumed by\n    for (l2 = i2; rchg2[i2 - 1]; i2--) so at the end of the loop body\n    i2 == 0 and the outer loop will exit.\n\n  - i1 >= 0 && i2 == -1 => there are deletions at the start of the file.\n    As the context lines in the postimage have been exhausted all the\n    remaining rchg1 elements represent deleted lines and are consumed by\n    for (l1 = i1; rchg1[i1 - 1]; i1--) so at the end of the loop body\n    i1 == 0 and the outer loop will exit.\n\n  - i1 >= 0 && i2 >= 0 => the first line is unchanged or there are\n    insertions and deletions at the beginning of the file. At the end of\n    the loop body i1 == 0 && i2 == 0 and the outer loop will exit.\n\nWe could add\n\n     if (i1 < -1 || i2 < -1)\n         BUG(\"mismatched context line count\");\n\nbefore \"if (rchg1[i1 - 1] || rchg2[i2 - 1])\" inside the loop if we're \nworried about bugs that break the assumption that there are equal \nnumbers of context lines on each side. Any such bug would generate \ninvalid diffs. I don't know how likely that is to happen in practice.\n\nBest Wishes\n\nPhillip\n\n> \n> Are you able to demonstrate any out-of-bounds access with e.g.,\n> Valgrind, AddressSanitizer or an assertion?\n> \n>> Curiously, AddressSanitizer doesn't report anything, but if I add the\n>> following line after the outer for, I can trigger it to report a\n>> heap-buffer-overflow with e.g., git show 8613c2bb6c:\n>>\n>> \tif (i1 < 0 || i2 < 0) fprintf(stderr, \"Oops: %ld %ld\\n\", i1, i2);\n> \n> That's because I forgot to add braces.  D'oh!  I can't trigger any\n> out-of-bounds access or that Oops with them properly in place.  So I\n> let myself get fooled by a daring coding style. :-|\n> \n>>\n>>>\n>>> Signed-off-by: Alex Guo <alexguo1023@gmail.com>\n>>> ---\n>>>      Fix buffer underflow in xdl_build_script\n>>>\n>>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1976%2Fmugitya03%2Fbuf-1-v1\n>>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1976/mugitya03/buf-1-v1\n>>> Pull-Request: https://github.com/git/git/pull/1976\n>>>\n>>>   xdiff/xdiffi.c | 7 ++++---\n>>>   1 file changed, 4 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n>>> index 5a96e36dfbe..2e983965328 100644\n>>> --- a/xdiff/xdiffi.c\n>>> +++ b/xdiff/xdiffi.c\n>>> @@ -951,9 +951,10 @@ int xdl_build_script(xdfenv_t *xe, xdchange_t **xscr) {\n>>>   \t * Trivial. Collects \"groups\" of changes and creates an edit script.\n> \n> Trivial for Davide perhaps (libxdiff author), but not my mushy brain..\n> \n>>>   \t */\n>>>   \tfor (i1 = xe->xdf1.nrec, i2 = xe->xdf2.nrec; i1 >= 0 || i2 >= 0; i1--, i2--)\n>>\n>> Should the || be a && instead?  From a birds-eye view I would assume we\n>> can stop scanning for changes when we exhaust (reach the top) of either\n>> side.  We just have to make sure everything from the other side is\n>> accounted for in the last added change.\n>>\n>>> -\t\tif (rchg1[i1 - 1] || rchg2[i2 - 1]) {\n>>> -\t\t\tfor (l1 = i1; rchg1[i1 - 1]; i1--);\n>>> -\t\t\tfor (l2 = i2; rchg2[i2 - 1]; i2--);\n>>> +\t\tif ((i1 > 0 && rchg1[i1 - 1]) ||\n>>> +\t\t\t(i2 > 0 && rchg2[i2 - 1])) {\n>>> +\t\t\tfor (l1 = i1; i1 > 0 && rchg1[i1 - 1]; i1--);\n>>> +            for (l2 = i2; i2 > 0 && rchg2[i2 - 1]; i2--);\n>>\n>> Nit: The indentation of that line is off.\n>>\n>>>   \n>>>   \t\t\tif (!(xch = xdl_add_change(cscr, i1, i2, l1 - i1, l2 - i2))) {\n>>>   \t\t\t\txdl_free_script(cscr);\n>>>\n>>> base-commit: 8613c2bb6cd16ef530dc5dd74d3b818a1ccbf1c0\n"},{"id":"518844","messageId":"953dc362-f546-4f49-b580-b48a4a1370e8@gmail.com","threadId":"63511","inReplyTo":"2b9c5e91-67ab-4e46-93c4-15c8b79841be@gmail.com","subject":"Re: [PATCH] Fix buffer underflow in xdl_build_script","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-24T13:53:23Z","receivedAt":"2025-05-24T13:53:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/05/2025 14:38, Phillip Wood wrote:\n> On 24/05/2025 10:08, René Scharfe wrote:\n>> Am 24.05.25 um 07:57 schrieb René Scharfe:\n>>> Am 23.05.25 um 22:51 schrieb Alex via GitGitGadget:\n>>>> From: jinyaoguo <guo846@purdue.edu>\n>>>>\n>>>> The loop in xdl_build_script used `i1 >= 0 || i2 >= 0`, causing\n>>>> `i1` (or `i2`) to reach 0 and then access `rchg1[i1-1]` (or\n>>>> `rchg2[i2-1]`), which underflows the buffer.\n>>>> This commit adds explicit `i1 > 0` and `i2 > 0` checks around\n>>>> those array accesses to prevent invalid negative indexing.\n>>>\n>>> xdl_prepare_ctx() in xdiff/xprepare.c allocates an extra entry at both\n>>> ends for rchg arrays, so an index of -1 should be within the bounds.\n> \n> and rchg[-1] == 0 so i1 and i2 can never drop below -1\n> \n>>> i1 and i2 are decreased in lockstep, though, so one of them can become\n>>> smaller than -1 if nrec is different between the files.  And that's how\n>>> this code run can indeed run off into the weeds.\n>>\n>> Actually no, i1 can't seem to reach 0 without i2 also being 0 and vice\n>> versa.  Or can it?  It makes sense that we reach the start of both\n>> buffers at the same time if we walk backwards from the end, don't\n>> misstep and have consistent rchg array contents, but I'm not sure.\n> The code looks like\n> \n>      for (i1 = xe->xdf1.nrec, i2 = xe->xdf2.nrec; i1 >= 0 || i2 >= 0; \n> i1--, i2--)\n>          if (rchg1[i1 - 1] || rchg2[i2 - 1]) {\n>              for (l1 = i1; rchg1[i1 - 1]; i1--);\n>              for (l2 = i2; rchg2[i2 - 1]; i2--);\n> \n> I think I've convinced myself that it is safe assuming there are an \n> equal number of unchanged lines in rchg1 and rchg2 and rchg1[-1] == \n> rchg2[-1] == 0. Each iteration consumes any changed lines in the \n> preimage and the postimage plus a single context line from each (apart \n> from the final iteration when the context lines may have been \n> exhausted). At the start of the last iteration there are three \n> possibilities\n> \n>   - i1 == -1 && i2 >= 0 => there are insertions as the start of the file.\n\nSorry that should be i1 == 0 && i2 >= 0\n\n>     As the context lines in the preimage have been exhausted all the\n>     remaining rchg2 elements represent added lines and are consumed by\n>     for (l2 = i2; rchg2[i2 - 1]; i2--) so at the end of the loop body\n>     i2 == 0 and the outer loop will exit.\n> \n>   - i1 >= 0 && i2 == -1 => there are deletions at the start of the file.\n\nThis one should be i1 >= 0 && i2 == 0\n\n>     As the context lines in the postimage have been exhausted all the\n>     remaining rchg1 elements represent deleted lines and are consumed by\n>     for (l1 = i1; rchg1[i1 - 1]; i1--) so at the end of the loop body\n>     i1 == 0 and the outer loop will exit.\n> \n>   - i1 >= 0 && i2 >= 0 => the first line is unchanged or there are\n>     insertions and deletions at the beginning of the file. At the end of\n>     the loop body i1 == 0 && i2 == 0 and the outer loop will exit.\n> \n> We could add\n> \n>      if (i1 < -1 || i2 < -1)\n>          BUG(\"mismatched context line count\");\n> \n> before \"if (rchg1[i1 - 1] || rchg2[i2 - 1])\" inside the loop if we're \n> worried about bugs that break the assumption that there are equal \n> numbers of context lines on each side. Any such bug would generate \n> invalid diffs. I don't know how likely that is to happen in practice.\n> \n> Best Wishes\n> \n> Phillip\n> \n>>\n>> Are you able to demonstrate any out-of-bounds access with e.g.,\n>> Valgrind, AddressSanitizer or an assertion?\n>>\n>>> Curiously, AddressSanitizer doesn't report anything, but if I add the\n>>> following line after the outer for, I can trigger it to report a\n>>> heap-buffer-overflow with e.g., git show 8613c2bb6c:\n>>>\n>>>     if (i1 < 0 || i2 < 0) fprintf(stderr, \"Oops: %ld %ld\\n\", i1, i2);\n>>\n>> That's because I forgot to add braces.  D'oh!  I can't trigger any\n>> out-of-bounds access or that Oops with them properly in place.  So I\n>> let myself get fooled by a daring coding style. :-|\n>>\n>>>\n>>>>\n>>>> Signed-off-by: Alex Guo <alexguo1023@gmail.com>\n>>>> ---\n>>>>      Fix buffer underflow in xdl_build_script\n>>>>\n>>>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr- \n>>>> git-1976%2Fmugitya03%2Fbuf-1-v1\n>>>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr- \n>>>> git-1976/mugitya03/buf-1-v1\n>>>> Pull-Request: https://github.com/git/git/pull/1976\n>>>>\n>>>>   xdiff/xdiffi.c | 7 ++++---\n>>>>   1 file changed, 4 insertions(+), 3 deletions(-)\n>>>>\n>>>> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n>>>> index 5a96e36dfbe..2e983965328 100644\n>>>> --- a/xdiff/xdiffi.c\n>>>> +++ b/xdiff/xdiffi.c\n>>>> @@ -951,9 +951,10 @@ int xdl_build_script(xdfenv_t *xe, xdchange_t \n>>>> **xscr) {\n>>>>        * Trivial. Collects \"groups\" of changes and creates an edit \n>>>> script.\n>>\n>> Trivial for Davide perhaps (libxdiff author), but not my mushy brain..\n>>\n>>>>        */\n>>>>       for (i1 = xe->xdf1.nrec, i2 = xe->xdf2.nrec; i1 >= 0 || i2 >= \n>>>> 0; i1--, i2--)\n>>>\n>>> Should the || be a && instead?  From a birds-eye view I would assume we\n>>> can stop scanning for changes when we exhaust (reach the top) of either\n>>> side.  We just have to make sure everything from the other side is\n>>> accounted for in the last added change.\n>>>\n>>>> -        if (rchg1[i1 - 1] || rchg2[i2 - 1]) {\n>>>> -            for (l1 = i1; rchg1[i1 - 1]; i1--);\n>>>> -            for (l2 = i2; rchg2[i2 - 1]; i2--);\n>>>> +        if ((i1 > 0 && rchg1[i1 - 1]) ||\n>>>> +            (i2 > 0 && rchg2[i2 - 1])) {\n>>>> +            for (l1 = i1; i1 > 0 && rchg1[i1 - 1]; i1--);\n>>>> +            for (l2 = i2; i2 > 0 && rchg2[i2 - 1]; i2--);\n>>>\n>>> Nit: The indentation of that line is off.\n>>>\n>>>>               if (!(xch = xdl_add_change(cscr, i1, i2, l1 - i1, l2 - \n>>>> i2))) {\n>>>>                   xdl_free_script(cscr);\n>>>>\n>>>> base-commit: 8613c2bb6cd16ef530dc5dd74d3b818a1ccbf1c0\n> \n\n"}]}