{"thread":{"id":"49317","subject":"Git 2.19 Segmentation fault 11 on macOS","startedAt":"2018-09-11T15:25:44Z","lastAt":"2018-09-17T18:48:43Z","messageCount":19,"participants":["ryenus","Derrick Stolee","Elijah Newren","Thomas Gummerer","Junio C Hamano","Johannes Schindelin","Eric Sunshine","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"357863","messageId":"CAKkAvay6crMOJ0Vm2C9Z0ktBj9n4+RkOAiP+zuG=Sm+PVBgQ+Q@mail.gmail.com","threadId":"49317","inReplyTo":null,"subject":"Git 2.19 Segmentation fault 11 on macOS","fromName":"ryenus","fromEmail":"ryenus@gmail.com","sentAt":"2018-09-11T15:25:30Z","receivedAt":"2018-09-11T15:25:44Z","isPatch":false,"sender":{"key":"ryenus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/610161?v=4"},"body":"I just updated to 2.19 via Homebrew, git range-diff seems cool, but I\nonly got a Segmentation fault: 11\n\n    $ git version; git range-diff origin/master  HEAD@{2} HEAD\n    git version 2.19.0\n    Segmentation fault: 11\n\nBoth origin/master and my local branch each got two new commits of their own,\nplease correct me if this is not the expected way to use git range-diff.\n\nFYI, I've created a sample repo here:\nhttps://github.com/ryenus/range-diff-segfault/\n"},{"id":"357864","messageId":"1b8a35be-4234-7f71-c0be-41736bbe60cf@gmail.com","threadId":"49317","inReplyTo":"CAKkAvay6crMOJ0Vm2C9Z0ktBj9n4+RkOAiP+zuG=Sm+PVBgQ+Q@mail.gmail.com","subject":"Re: Git 2.19 Segmentation fault 11 on macOS","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-09-11T15:38:53Z","receivedAt":"2018-09-11T15:38:58Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2018 11:25 AM, ryenus wrote:\n> I just updated to 2.19 via Homebrew, git range-diff seems cool, but I\n> only got a Segmentation fault: 11\n>\n>      $ git version; git range-diff origin/master  HEAD@{2} HEAD\n>      git version 2.19.0\n>      Segmentation fault: 11\n>\n> Both origin/master and my local branch each got two new commits of their own,\n> please correct me if this is not the expected way to use git range-diff.\n>\n> FYI, I've created a sample repo here:\n> https://github.com/ryenus/range-diff-segfault/\n\nHi Ryenus,\n\nThanks for the report!\n\nI ran something similar using Git for Windows 2.19.0-rc2. I had to run \n`git commit --amend --no-edit` on the tip commit to make my local master \ndisagree with origin/master. I then ran the following:\n\n$ git range-diff origin/master HEAD~1 HEAD\n-:  ------- > 1:  5009c62 aaa\n\nWith this, the command succeeded for me. There is another way to get a \nsimilar result, could you try it?\n\n$ git range-diff origin/master~1..origin/master HEAD~1..HEAD\n1:  f14d571 = 1:  5009c62 aaa\n\nOtherwise, we can now get started trying to repro this on a Mac. Thanks!\n\n-Stolee\n"},{"id":"357865","messageId":"CABPp-BFUoTYSuTrtJt7girB50CGaEwg=Hgbuii45juBbTx0w0A@mail.gmail.com","threadId":"49317","inReplyTo":"CAKkAvay6crMOJ0Vm2C9Z0ktBj9n4+RkOAiP+zuG=Sm+PVBgQ+Q@mail.gmail.com","subject":"Re: Git 2.19 Segmentation fault 11 on macOS","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-11T15:47:40Z","receivedAt":"2018-09-11T15:47:56Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Sep 11, 2018 at 8:27 AM ryenus <ryenus@gmail.com> wrote:\n>\n> I just updated to 2.19 via Homebrew, git range-diff seems cool, but I\n> only got a Segmentation fault: 11\n>\n>     $ git version; git range-diff origin/master  HEAD@{2} HEAD\n>     git version 2.19.0\n>     Segmentation fault: 11\n>\n> Both origin/master and my local branch each got two new commits of their own,\n> please correct me if this is not the expected way to use git range-diff.\n>\n> FYI, I've created a sample repo here:\n> https://github.com/ryenus/range-diff-segfault/\n\nThanks for the report and coming up with a sample repo.  However,\nreflogs don't transfer with clones, and your origin/master may well\npoint somewhere different than ours.  Could you run\n   git rev-parse origin/master HEAD@{2} HEAD\nin the range-diff-segfault repo where you can reproduce so we know\nwhat commits to pass to trigger the bug?\n"},{"id":"357866","messageId":"20180911154906.GA4865@hank.intra.tgummerer.com","threadId":"49317","inReplyTo":"CAKkAvay6crMOJ0Vm2C9Z0ktBj9n4+RkOAiP+zuG=Sm+PVBgQ+Q@mail.gmail.com","subject":"Re: Git 2.19 Segmentation fault 11 on macOS","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-09-11T15:49:06Z","receivedAt":"2018-09-11T15:49:11Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Hi,\n\nthanks for your bug report!\n\nOn 09/11, ryenus wrote:\n> I just updated to 2.19 via Homebrew, git range-diff seems cool, but I\n> only got a Segmentation fault: 11\n> \n>     $ git version; git range-diff origin/master  HEAD@{2} HEAD\n\nUnfortunately the HEAD@{2} syntax needs your reflog, which is not\navailable when just cloning the repository (the reflog is only local\nand not pushed to the remote repository).  Would it be possible to\ncreate a short script to create the repository where you're\nexperiencing the behaviour, or replacing 'origin/master', 'HEAD@{2}'\nand 'HEAD' with the actual commit ids?\n\nI tried with various values, but unfortunately failed to reproduce\nthis so far (although admittedly I tried it on linux, not Mac OS).\n\n>     git version 2.19.0\n>     Segmentation fault: 11\n> \n> Both origin/master and my local branch each got two new commits of their own,\n> please correct me if this is not the expected way to use git range-diff.\n> \n> FYI, I've created a sample repo here:\n> https://github.com/ryenus/range-diff-segfault/\n"},{"id":"357868","messageId":"CAKkAvaw4QTMzKXpkpAaMhZaz68=OdS_AmrMXyu6C9td2P+XmTg@mail.gmail.com","threadId":"49317","inReplyTo":"20180911154906.GA4865@hank.intra.tgummerer.com","subject":"Re: Git 2.19 Segmentation fault 11 on macOS","fromName":"ryenus","fromEmail":"ryenus@gmail.com","sentAt":"2018-09-11T16:03:35Z","receivedAt":"2018-09-11T16:03:49Z","isPatch":false,"sender":{"key":"ryenus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/610161?v=4"},"body":"On Tue, 11 Sep 2018 at 23:49, Thomas Gummerer <t.gummerer@gmail.com> wrote:\n>\n> Hi,\n>\n> thanks for your bug report!\n>\n> On 09/11, ryenus wrote:\n> > I just updated to 2.19 via Homebrew, git range-diff seems cool, but I\n> > only got a Segmentation fault: 11\n> >\n> >     $ git version; git range-diff origin/master  HEAD@{2} HEAD\n>\n> Unfortunately the HEAD@{2} syntax needs your reflog, which is not\n> available when just cloning the repository (the reflog is only local\n> and not pushed to the remote repository).  Would it be possible to\n> create a short script to create the repository where you're\n> experiencing the behaviour, or replacing 'origin/master', 'HEAD@{2}'\n> and 'HEAD' with the actual commit ids?\n\nso `HEAD~2` should be used instead of `HEAD@{2}`, right?\nI just tried the following and got same error:\n\n    $ git range-diff master patch~2 patch\n    Segmentation fault: 11\n\n>\n> I tried with various values, but unfortunately failed to reproduce\n> this so far (although admittedly I tried it on linux, not Mac OS).\n>\n> >     git version 2.19.0\n> >     Segmentation fault: 11\n> >\n> > Both origin/master and my local branch each got two new commits of their own,\n> > please correct me if this is not the expected way to use git range-diff.\n> >\n> > FYI, I've created a sample repo here:\n> > https://github.com/ryenus/range-diff-segfault/\n"},{"id":"357869","messageId":"844da493-b1c1-b295-0094-beafd48f3b50@gmail.com","threadId":"49317","inReplyTo":"1b8a35be-4234-7f71-c0be-41736bbe60cf@gmail.com","subject":"Re: Git 2.19 Segmentation fault 11 on macOS","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-09-11T16:04:20Z","receivedAt":"2018-09-11T16:04:26Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2018 11:38 AM, Derrick Stolee wrote:\n> On 9/11/2018 11:25 AM, ryenus wrote:\n>> I just updated to 2.19 via Homebrew, git range-diff seems cool, but I\n>> only got a Segmentation fault: 11\n>>\n>>      $ git version; git range-diff origin/master  HEAD@{2} HEAD\n>>      git version 2.19.0\n>>      Segmentation fault: 11\n>>\n>> Both origin/master and my local branch each got two new commits of \n>> their own,\n>> please correct me if this is not the expected way to use git range-diff.\n>>\n>> FYI, I've created a sample repo here:\n>> https://github.com/ryenus/range-diff-segfault/\n>\n> Hi Ryenus,\n>\n> Thanks for the report!\n>\n> I ran something similar using Git for Windows 2.19.0-rc2. I had to run \n> `git commit --amend --no-edit` on the tip commit to make my local \n> master disagree with origin/master. I then ran the following:\n>\n> $ git range-diff origin/master HEAD~1 HEAD\n> -:  ------- > 1:  5009c62 aaa\n>\n> With this, the command succeeded for me. There is another way to get a \n> similar result, could you try it?\n>\n> $ git range-diff origin/master~1..origin/master HEAD~1..HEAD\n> 1:  f14d571 = 1:  5009c62 aaa\n>\n> Otherwise, we can now get started trying to repro this on a Mac. Thanks!\n\nThe patch below includes a test that fails on Mac OSX with a segfault.\n\nGitGitGadget PR: https://github.com/gitgitgadget/git/pull/36\nFailed Build: \nhttps://git-for-windows.visualstudio.com/git/_build/results?buildId=18616&view=logs\n\n-->8--\n\n From 3ee470d09d54b9ad7ab950f17051d625db0c8654 Mon Sep 17 00:00:00 2001\nFrom: Derrick Stolee <dstolee@microsoft.com>\nDate: Tue, 11 Sep 2018 11:42:03 -0400\nSubject: [PATCH] range-diff: attempt to create test that fails on OSX\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n  t/t3206-range-diff.sh | 5 +++++\n  1 file changed, 5 insertions(+)\n\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 2237c7f4af..02744b07a8 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n         test_cmp expected actual\n  '\n\n+test_expect_success 'amend and check' '\n+       git commit --amend -m \"new message\" &&\n+       git range-diff changed-message HEAD@{2} HEAD\n+'\n+\n  test_done\n--\n2.19.0.rc2.windows.1\n\n"},{"id":"357872","messageId":"fd241679-2283-4e01-315b-db27be8a794c@gmail.com","threadId":"49317","inReplyTo":"844da493-b1c1-b295-0094-beafd48f3b50@gmail.com","subject":"Re: Git 2.19 Segmentation fault 11 on macOS","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-09-11T16:13:42Z","receivedAt":"2018-09-11T16:13:45Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2018 12:04 PM, Derrick Stolee wrote:\n> On 9/11/2018 11:38 AM, Derrick Stolee wrote:\n>> On 9/11/2018 11:25 AM, ryenus wrote:\n>>> I just updated to 2.19 via Homebrew, git range-diff seems cool, but I\n>>> only got a Segmentation fault: 11\n>>>\n>>>      $ git version; git range-diff origin/master  HEAD@{2} HEAD\n>>>      git version 2.19.0\n>>>      Segmentation fault: 11\n>>>\n>>> Both origin/master and my local branch each got two new commits of \n>>> their own,\n>>> please correct me if this is not the expected way to use git \n>>> range-diff.\n>>>\n>>> FYI, I've created a sample repo here:\n>>> https://github.com/ryenus/range-diff-segfault/\n>>\n>> Hi Ryenus,\n>>\n>> Thanks for the report!\n>>\n>> I ran something similar using Git for Windows 2.19.0-rc2. I had to \n>> run `git commit --amend --no-edit` on the tip commit to make my local \n>> master disagree with origin/master. I then ran the following:\n>>\n>> $ git range-diff origin/master HEAD~1 HEAD\n>> -:  ------- > 1:  5009c62 aaa\n>>\n>> With this, the command succeeded for me. There is another way to get \n>> a similar result, could you try it?\n>>\n>> $ git range-diff origin/master~1..origin/master HEAD~1..HEAD\n>> 1:  f14d571 = 1:  5009c62 aaa\n>>\n>> Otherwise, we can now get started trying to repro this on a Mac. Thanks!\n>\n> The patch below includes a test that fails on Mac OSX with a segfault.\n>\n> GitGitGadget PR: https://github.com/gitgitgadget/git/pull/36\n> Failed Build: \n> https://git-for-windows.visualstudio.com/git/_build/results?buildId=18616&view=logs\n>\n> -->8--\n>\n> From 3ee470d09d54b9ad7ab950f17051d625db0c8654 Mon Sep 17 00:00:00 2001\n> From: Derrick Stolee <dstolee@microsoft.com>\n> Date: Tue, 11 Sep 2018 11:42:03 -0400\n> Subject: [PATCH] range-diff: attempt to create test that fails on OSX\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  t/t3206-range-diff.sh | 5 +++++\n>  1 file changed, 5 insertions(+)\n>\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index 2237c7f4af..02744b07a8 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n>         test_cmp expected actual\n>  '\n>\n> +test_expect_success 'amend and check' '\n> +       git commit --amend -m \"new message\" &&\n> +       git range-diff changed-message HEAD@{2} HEAD\n> +'\n> +\n>  test_done\n> -- \n> 2.19.0.rc2.windows.1\n\n\nSorry, nevermind. The test failed for a different reason:\n\n2018-09-11T16:02:20.2680990Z ++ git range-diff changed-message \n'HEAD@{2}' HEAD\n2018-09-11T16:02:20.2779250Z fatal: Log for 'HEAD' only has 2 entries.\n2018-09-11T16:02:20.2802520Z error: could not parse log for \n'changed-message..HEAD@{2}'\n2018-09-11T16:02:20.2817470Z error: last command exited with $?=255\n2018-09-11T16:02:20.2832300Z not ok 12 - amend and check\n\nRyenus, it would help if you could create and push the following \nbranches based on your local repro:\n\n     git branch base HEAD@{2}\n\n     git branch topic HEAD\n\n     git push origin base topic\n\nAlso, does the following command fail, after creating the branches?\n\n     git range-diff origin/master base topic\n\n\nThanks,\n\n-Stolee\n\n"},{"id":"357873","messageId":"20180911163419.GB4865@hank.intra.tgummerer.com","threadId":"49317","inReplyTo":"fd241679-2283-4e01-315b-db27be8a794c@gmail.com","subject":"Re: Git 2.19 Segmentation fault 11 on macOS","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-09-11T16:34:19Z","receivedAt":"2018-09-11T16:34:24Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 09/11, Derrick Stolee wrote:\n> On 9/11/2018 12:04 PM, Derrick Stolee wrote:\n> > On 9/11/2018 11:38 AM, Derrick Stolee wrote:\n> > The patch below includes a test that fails on Mac OSX with a segfault.\n> > \n> > GitGitGadget PR: https://github.com/gitgitgadget/git/pull/36\n> > Failed Build: https://git-for-windows.visualstudio.com/git/_build/results?buildId=18616&view=logs\n> > \n> > -->8--\n> > \n> > From 3ee470d09d54b9ad7ab950f17051d625db0c8654 Mon Sep 17 00:00:00 2001\n> > From: Derrick Stolee <dstolee@microsoft.com>\n> > Date: Tue, 11 Sep 2018 11:42:03 -0400\n> > Subject: [PATCH] range-diff: attempt to create test that fails on OSX\n> > \n> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> > ---\n> >  t/t3206-range-diff.sh | 5 +++++\n> >  1 file changed, 5 insertions(+)\n> > \n> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> > index 2237c7f4af..02744b07a8 100755\n> > --- a/t/t3206-range-diff.sh\n> > +++ b/t/t3206-range-diff.sh\n> > @@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n> >         test_cmp expected actual\n> >  '\n> > \n> > +test_expect_success 'amend and check' '\n> > +       git commit --amend -m \"new message\" &&\n> > +       git range-diff changed-message HEAD@{2} HEAD\n> > +'\n> > +\n> >  test_done\n> > -- \n> > 2.19.0.rc2.windows.1\n> \n> \n> Sorry, nevermind. The test failed for a different reason:\n\nI think you're on the right track here.  I can not test this on Mac\nOS, but on Linux, the following fails when running the test under\nvalgrind:\n\n    diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n    index 2237c7f4af..a8b0ef8c1d 100755\n    --- a/t/t3206-range-diff.sh\n    +++ b/t/t3206-range-diff.sh\n    @@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n            test_cmp expected actual\n     '\n     \n    +test_expect_success 'amend and check' '\n    +       git commit --amend -m \"new message\" &&\n    +       git range-diff master HEAD@{1} HEAD\n    +'\n    +\n     test_done\n\nvalgrind gives me the following:\n\n==18232== Invalid read of size 4\n==18232==    at 0x34D7B5: compute_assignment (linear-assignment.c:54)\n==18232==    by 0x2A4253: get_correspondences (range-diff.c:245)\n==18232==    by 0x2A4BFB: show_range_diff (range-diff.c:427)\n==18232==    by 0x19D453: cmd_range_diff (range-diff.c:108)\n==18232==    by 0x122698: run_builtin (git.c:418)\n==18232==    by 0x1229D8: handle_builtin (git.c:637)\n==18232==    by 0x122BCC: run_argv (git.c:689)\n==18232==    by 0x122D90: cmd_main (git.c:766)\n==18232==    by 0x1D55A3: main (common-main.c:45)\n==18232==  Address 0x4f4d844 is 0 bytes after a block of size 4 alloc'd\n==18232==    at 0x483777F: malloc (vg_replace_malloc.c:299)\n==18232==    by 0x3381B0: do_xmalloc (wrapper.c:60)\n==18232==    by 0x338283: xmalloc (wrapper.c:87)\n==18232==    by 0x2A3F8C: get_correspondences (range-diff.c:207)\n==18232==    by 0x2A4BFB: show_range_diff (range-diff.c:427)\n==18232==    by 0x19D453: cmd_range_diff (range-diff.c:108)\n==18232==    by 0x122698: run_builtin (git.c:418)\n==18232==    by 0x1229D8: handle_builtin (git.c:637)\n==18232==    by 0x122BCC: run_argv (git.c:689)\n==18232==    by 0x122D90: cmd_main (git.c:766)\n==18232==    by 0x1D55A3: main (common-main.c:45)\n==18232== \n\nI'm looking into why that fails.  Also adding Dscho to Cc here as the\nauthor of this code.\n"},{"id":"357874","messageId":"CABPp-BG7YJF=m0Z82wp_JVXn3fiHW4FeL6WK2FdOOkdFx9LguA@mail.gmail.com","threadId":"49317","inReplyTo":"CAKkAvaw4QTMzKXpkpAaMhZaz68=OdS_AmrMXyu6C9td2P+XmTg@mail.gmail.com","subject":"Re: Git 2.19 Segmentation fault 11 on macOS","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-11T16:35:03Z","receivedAt":"2018-09-11T16:35:18Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Sep 11, 2018 at 9:07 AM ryenus <ryenus@gmail.com> wrote:\n> On Tue, 11 Sep 2018 at 23:49, Thomas Gummerer <t.gummerer@gmail.com> wrote:\n> >\n> > Hi,\n> >\n> > thanks for your bug report!\n> >\n> > On 09/11, ryenus wrote:\n> > > I just updated to 2.19 via Homebrew, git range-diff seems cool, but I\n> > > only got a Segmentation fault: 11\n> > >\n> > >     $ git version; git range-diff origin/master  HEAD@{2} HEAD\n> >\n> > Unfortunately the HEAD@{2} syntax needs your reflog, which is not\n> > available when just cloning the repository (the reflog is only local\n> > and not pushed to the remote repository).  Would it be possible to\n> > create a short script to create the repository where you're\n> > experiencing the behaviour, or replacing 'origin/master', 'HEAD@{2}'\n> > and 'HEAD' with the actual commit ids?\n>\n> so `HEAD~2` should be used instead of `HEAD@{2}`, right?\n> I just tried the following and got same error:\n>\n>     $ git range-diff master patch~2 patch\n>     Segmentation fault: 11\n\nAfter cloning the repo and running\n$ git range-diff master origin/patch~2 origin/patch\n\nI cannot reproduce on either Linux or Mac OS X.  On Mac OS X, I tried\nboth with a locally built git-2.19 from sources, as well as an\nhomebrew-installed version of git-2.19.0.\n\nFor reference here's what I get running git rev-parse on those arguments:\n$ git rev-parse master origin/patch~2 origin/patch\nf14d571887c1b98fd22c60bc21c11700456162fa\n5c7e07ebfbc7de5304deab6a04b476e6fa082d0e\nad8a185de38bfe546dd64fe37ae566de260d73c2\n\nIs there any chance I'm misunderstanding or the repo doesn't have the\ncommits you were actually using to reproduce the bug?\n"},{"id":"357875","messageId":"xmqqa7ooq8to.fsf@gitster-ct.c.googlers.com","threadId":"49317","inReplyTo":"fd241679-2283-4e01-315b-db27be8a794c@gmail.com","subject":"Re: Git 2.19 Segmentation fault 11 on macOS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-11T16:48:19Z","receivedAt":"2018-09-11T16:48:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 9/11/2018 12:04 PM, Derrick Stolee wrote:\n>\n>> The patch below includes a test that fails on Mac OSX with a segfault.\n> ...\n> Sorry, nevermind. The test failed for a different reason:\n\nEven if it is for a different reason, segfaulting is not acceptable,\nbut it seems it is failing quite normally.\n\nShucks.  It sounded too easy to get a reproduction like so X-<.\n\n> 2018-09-11T16:02:20.2680990Z ++ git range-diff changed-message\n> 'HEAD@{2}' HEAD\n> 2018-09-11T16:02:20.2779250Z fatal: Log for 'HEAD' only has 2 entries.\n> 2018-09-11T16:02:20.2802520Z error: could not parse log for\n> 'changed-message..HEAD@{2}'\n> 2018-09-11T16:02:20.2817470Z error: last command exited with $?=255\n> 2018-09-11T16:02:20.2832300Z not ok 12 - amend and check\n>\n> Ryenus, it would help if you could create and push the following\n> branches based on your local repro:\n>\n>     git branch base HEAD@{2}\n>\n>     git branch topic HEAD\n>\n>     git push origin base topic\n>\n> Also, does the following command fail, after creating the branches?\n>\n>     git range-diff origin/master base topic\n\nYup, that is a very sensible way to get a reliable reproduction.\n\nThanks for helping.\n\n"},{"id":"357881","messageId":"20180911172903.GC4865@hank.intra.tgummerer.com","threadId":"49317","inReplyTo":"20180911163419.GB4865@hank.intra.tgummerer.com","subject":"Re: Git 2.19 Segmentation fault 11 on macOS","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-09-11T17:29:04Z","receivedAt":"2018-09-11T17:29:08Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 09/11, Thomas Gummerer wrote:\n> I think you're on the right track here.  I can not test this on Mac\n> OS, but on Linux, the following fails when running the test under\n> valgrind:\n> \n>     diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n>     index 2237c7f4af..a8b0ef8c1d 100755\n>     --- a/t/t3206-range-diff.sh\n>     +++ b/t/t3206-range-diff.sh\n>     @@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n>             test_cmp expected actual\n>      '\n>      \n>     +test_expect_success 'amend and check' '\n>     +       git commit --amend -m \"new message\" &&\n>     +       git range-diff master HEAD@{1} HEAD\n>     +'\n>     +\n>      test_done\n> \n> valgrind gives me the following:\n> \n> ==18232== Invalid read of size 4\n> ==18232==    at 0x34D7B5: compute_assignment (linear-assignment.c:54)\n> ==18232==    by 0x2A4253: get_correspondences (range-diff.c:245)\n> ==18232==    by 0x2A4BFB: show_range_diff (range-diff.c:427)\n> ==18232==    by 0x19D453: cmd_range_diff (range-diff.c:108)\n> ==18232==    by 0x122698: run_builtin (git.c:418)\n> ==18232==    by 0x1229D8: handle_builtin (git.c:637)\n> ==18232==    by 0x122BCC: run_argv (git.c:689)\n> ==18232==    by 0x122D90: cmd_main (git.c:766)\n> ==18232==    by 0x1D55A3: main (common-main.c:45)\n> ==18232==  Address 0x4f4d844 is 0 bytes after a block of size 4 alloc'd\n> ==18232==    at 0x483777F: malloc (vg_replace_malloc.c:299)\n> ==18232==    by 0x3381B0: do_xmalloc (wrapper.c:60)\n> ==18232==    by 0x338283: xmalloc (wrapper.c:87)\n> ==18232==    by 0x2A3F8C: get_correspondences (range-diff.c:207)\n> ==18232==    by 0x2A4BFB: show_range_diff (range-diff.c:427)\n> ==18232==    by 0x19D453: cmd_range_diff (range-diff.c:108)\n> ==18232==    by 0x122698: run_builtin (git.c:418)\n> ==18232==    by 0x1229D8: handle_builtin (git.c:637)\n> ==18232==    by 0x122BCC: run_argv (git.c:689)\n> ==18232==    by 0x122D90: cmd_main (git.c:766)\n> ==18232==    by 0x1D55A3: main (common-main.c:45)\n> ==18232== \n> \n> I'm looking into why that fails.  Also adding Dscho to Cc here as the\n> author of this code.\n\nThe diff below seems to fix it.  Not submitting this as a proper\npatch, as I don't quite understand what the original code tried to do\nhere.  However this does pass all tests we currently have and fixes\nthe out of bounds memory read that's caught by valgrind (and that I\nimagine could cause the segfault on Mac OS).\n\nThis matches how the initial minimum for the reduction transfer is\ncalculated in [1].\n\nI'll try to convince myself of the right solution, but should someone\nmore familiar with the linear-assignment algorithm have an idea, feel\nfree to take this over :)\n\n[1]: https://github.com/src-d/lapjv/blob/master/lap.h#L276\n\n--- >8 ---\n\ndiff --git a/linear-assignment.c b/linear-assignment.c\nindex 9b3e56e283..ab0aa5fd41 100644\n--- a/linear-assignment.c\n+++ b/linear-assignment.c\n@@ -51,7 +51,7 @@ void compute_assignment(int column_count, int row_count, int *cost,\n \t\telse if (j1 < -1)\n \t\t\trow2column[i] = -2 - j1;\n \t\telse {\n-\t\t\tint min = COST(!j1, i) - v[!j1];\n+\t\t\tint min = INT_MAX;\n \t\t\tfor (j = 1; j < column_count; j++)\n \t\t\t\tif (j != j1 && min > COST(j, i) - v[j])\n \t\t\t\t\tmin = COST(j, i) - v[j];\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 2237c7f4af..a8b0ef8c1d 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'amend and check' '\n+\tgit commit --amend -m \"new message\" &&\n+\tgit range-diff master HEAD@{1} HEAD\n+'\n+\n test_done\n\n--- >8 ---\n"},{"id":"357994","messageId":"20180912190108.GE4865@hank.intra.tgummerer.com","threadId":"49317","inReplyTo":"20180911172903.GC4865@hank.intra.tgummerer.com","subject":"[PATCH] linear-assignment: fix potential out of bounds memory access (was: Re: Git 2.19 Segmentation fault 11 on macOS)","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-09-12T19:01:08Z","receivedAt":"2018-09-12T19:01:14Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 09/11, Thomas Gummerer wrote:\n> On 09/11, Thomas Gummerer wrote:\n> > I think you're on the right track here.  I can not test this on Mac\n> > OS, but on Linux, the following fails when running the test under\n> > valgrind:\n> > \n> >     diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> >     index 2237c7f4af..a8b0ef8c1d 100755\n> >     --- a/t/t3206-range-diff.sh\n> >     +++ b/t/t3206-range-diff.sh\n> >     @@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n> >             test_cmp expected actual\n> >      '\n> >      \n> >     +test_expect_success 'amend and check' '\n> >     +       git commit --amend -m \"new message\" &&\n> >     +       git range-diff master HEAD@{1} HEAD\n> >     +'\n> >     +\n> >      test_done\n> > \n> > valgrind gives me the following:\n> > \n> > ==18232== Invalid read of size 4\n> > ==18232==    at 0x34D7B5: compute_assignment (linear-assignment.c:54)\n> > ==18232==    by 0x2A4253: get_correspondences (range-diff.c:245)\n> > ==18232==    by 0x2A4BFB: show_range_diff (range-diff.c:427)\n> > ==18232==    by 0x19D453: cmd_range_diff (range-diff.c:108)\n> > ==18232==    by 0x122698: run_builtin (git.c:418)\n> > ==18232==    by 0x1229D8: handle_builtin (git.c:637)\n> > ==18232==    by 0x122BCC: run_argv (git.c:689)\n> > ==18232==    by 0x122D90: cmd_main (git.c:766)\n> > ==18232==    by 0x1D55A3: main (common-main.c:45)\n> > ==18232==  Address 0x4f4d844 is 0 bytes after a block of size 4 alloc'd\n> > ==18232==    at 0x483777F: malloc (vg_replace_malloc.c:299)\n> > ==18232==    by 0x3381B0: do_xmalloc (wrapper.c:60)\n> > ==18232==    by 0x338283: xmalloc (wrapper.c:87)\n> > ==18232==    by 0x2A3F8C: get_correspondences (range-diff.c:207)\n> > ==18232==    by 0x2A4BFB: show_range_diff (range-diff.c:427)\n> > ==18232==    by 0x19D453: cmd_range_diff (range-diff.c:108)\n> > ==18232==    by 0x122698: run_builtin (git.c:418)\n> > ==18232==    by 0x1229D8: handle_builtin (git.c:637)\n> > ==18232==    by 0x122BCC: run_argv (git.c:689)\n> > ==18232==    by 0x122D90: cmd_main (git.c:766)\n> > ==18232==    by 0x1D55A3: main (common-main.c:45)\n> > ==18232== \n> > \n> > I'm looking into why that fails.  Also adding Dscho to Cc here as the\n> > author of this code.\n> \n> The diff below seems to fix it.  Not submitting this as a proper\n> patch [...]\n\nI found the time to actually have a look at the paper, so here's a\nproper patch:\n\nI'm still not entirely sure what the initial code tried to do here,\nbut I think staying as close as possible to the original is probably\nour best option here, also for future readers of this code.\n\n--- >8 ---\n\nSubject: [PATCH] linear-assignment: fix potential out of bounds memory access\n\nCurrently the 'compute_assignment()' function can may read memory out\nof bounds, even if used correctly.  Namely this happens when we only\nhave one column.  In that case we try to calculate the initial\nminimum cost using '!j1' as column in the reduction transfer code.\nThat in turn causes us to try and get the cost from column 1 in the\ncost matrix, which does not exist, and thus results in an out of\nbounds memory read.\n\nInstead of trying to intialize the minimum cost from another column,\njust set it to INT_MAX.  This also matches what the example code in the\noriginal paper for the algorithm [1] does (it initializes the value to\ninf, for which INT_MAX is the closest match in C).\n\nNote that the test only fails under valgrind on Linux, but the same\ncommand has been reported to segfault on Mac OS.\n\nAlso start from 0 in the loop, which matches what the example code in\nthe original paper does as well.  Starting from 1 means we'd ignore\nthe first column during the reduction transfer phase.  Note that in\nthe original paper the loop does start from 1, but the implementation\nis in Pascal, where arrays are 1 indexed.\n\n[1]: Jonker, R., & Volgenant, A. (1987). A shortest augmenting path\n     algorithm for dense and sparse linear assignment\n     problems. Computing, 38(4), 325–340.\n\nReported-by: ryenus <ryenus@gmail.com>\nHelped-by: Derrick Stolee <stolee@gmail.com>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n linear-assignment.c   | 4 ++--\n t/t3206-range-diff.sh | 5 +++++\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/linear-assignment.c b/linear-assignment.c\nindex 9b3e56e283..7700b80eeb 100644\n--- a/linear-assignment.c\n+++ b/linear-assignment.c\n@@ -51,8 +51,8 @@ void compute_assignment(int column_count, int row_count, int *cost,\n \t\telse if (j1 < -1)\n \t\t\trow2column[i] = -2 - j1;\n \t\telse {\n-\t\t\tint min = COST(!j1, i) - v[!j1];\n-\t\t\tfor (j = 1; j < column_count; j++)\n+\t\t\tint min = INT_MAX;\n+\t\t\tfor (j = 0; j < column_count; j++)\n \t\t\t\tif (j != j1 && min > COST(j, i) - v[j])\n \t\t\t\t\tmin = COST(j, i) - v[j];\n \t\t\tv[j1] -= min;\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 2237c7f4af..fb4c13a84a 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'no commits on one side' '\n+\tgit commit --amend -m \"new message\" &&\n+\tgit range-diff master HEAD@{1} HEAD\n+'\n+\n test_done\n-- \n2.19.0.397.gdd90340f6a\n\n"},{"id":"358005","messageId":"xmqqy3c6jx1m.fsf@gitster-ct.c.googlers.com","threadId":"49317","inReplyTo":"20180912190108.GE4865@hank.intra.tgummerer.com","subject":"Re: [PATCH] linear-assignment: fix potential out of bounds memory access","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-12T20:11:33Z","receivedAt":"2018-09-12T20:11:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n>> > I'm looking into why that fails.  Also adding Dscho to Cc here as the\n>> > author of this code.\n>> \n>> The diff below seems to fix it.  Not submitting this as a proper\n>> patch [...]\n>\n> I found the time to actually have a look at the paper, so here's a\n> proper patch:\n>\n> I'm still not entirely sure what the initial code tried to do here,\n\nIt looks to me an attempt to optimize (instead of starting from a\nvalue that is too big to be minimum, pick the first value and start\nfrom there, and all the \"found even smaller one, so let's replace\"\nlater would work the same way) that went wrong (just that the \"first\none\" was written incorrectly), but it is not absolutely necessary to\nfind out why the code was written in a particular way that happened\nto be buggy.\n\n> but I think staying as close as possible to the original is probably\n> our best option here, also for future readers of this code.\n\nThanks for digging.\n\n> --- >8 ---\n>\n> Subject: [PATCH] linear-assignment: fix potential out of bounds memory access\n>\n> Currently the 'compute_assignment()' function can may read memory out\n> of bounds, even if used correctly.  Namely this happens when we only\n> have one column.  In that case we try to calculate the initial\n> minimum cost using '!j1' as column in the reduction transfer code.\n> That in turn causes us to try and get the cost from column 1 in the\n> cost matrix, which does not exist, and thus results in an out of\n> bounds memory read.\n\nThis nicely explains what goes wrong.\n\n> Instead of trying to intialize the minimum cost from another column,\n> just set it to INT_MAX.  This also matches what the example code in the\n> original paper for the algorithm [1] does (it initializes the value to\n> inf, for which INT_MAX is the closest match in C).\n\nYeah, if we really want to avoid INT_MAX we could use another \"have\nwe found any value yet?\" boolean variable, but the caller in\nget_correspondences() does not even worry about integer overflows\nwhen stuffing diffsize to the cost[] array, and the other possible\nvalue that can go to cost[] array is COST_MAX that is mere 65k, so\nit would be OK to use INT_MAX as sentinel here, I guess.\n\n> Note that the test only fails under valgrind on Linux, but the same\n> command has been reported to segfault on Mac OS.\n>\n> Also start from 0 in the loop, which matches what the example code in\n> the original paper does as well.  Starting from 1 means we'd ignore\n> the first column during the reduction transfer phase.  Note that in\n> the original paper the loop does start from 1, but the implementation\n> is in Pascal, where arrays are 1 indexed.\n>\n> [1]: Jonker, R., & Volgenant, A. (1987). A shortest augmenting path\n>      algorithm for dense and sparse linear assignment\n>      problems. Computing, 38(4), 325–340.\n>\n> Reported-by: ryenus <ryenus@gmail.com>\n> Helped-by: Derrick Stolee <stolee@gmail.com>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n>  linear-assignment.c   | 4 ++--\n>  t/t3206-range-diff.sh | 5 +++++\n>  2 files changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/linear-assignment.c b/linear-assignment.c\n> index 9b3e56e283..7700b80eeb 100644\n> --- a/linear-assignment.c\n> +++ b/linear-assignment.c\n> @@ -51,8 +51,8 @@ void compute_assignment(int column_count, int row_count, int *cost,\n>  \t\telse if (j1 < -1)\n>  \t\t\trow2column[i] = -2 - j1;\n>  \t\telse {\n> -\t\t\tint min = COST(!j1, i) - v[!j1];\n> -\t\t\tfor (j = 1; j < column_count; j++)\n> +\t\t\tint min = INT_MAX;\n> +\t\t\tfor (j = 0; j < column_count; j++)\n>  \t\t\t\tif (j != j1 && min > COST(j, i) - v[j])\n>  \t\t\t\t\tmin = COST(j, i) - v[j];\n>  \t\t\tv[j1] -= min;\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index 2237c7f4af..fb4c13a84a 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success 'no commits on one side' '\n> +\tgit commit --amend -m \"new message\" &&\n> +\tgit range-diff master HEAD@{1} HEAD\n> +'\n> +\n>  test_done\n"},{"id":"358020","messageId":"20180912224410.GB1719@hank.intra.tgummerer.com","threadId":"49317","inReplyTo":"xmqqy3c6jx1m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] linear-assignment: fix potential out of bounds memory access","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-09-12T22:44:10Z","receivedAt":"2018-09-12T22:44:15Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 09/12, Junio C Hamano wrote:\n> Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> > --- >8 ---\n> >\n> > Subject: [PATCH] linear-assignment: fix potential out of bounds memory access\n> >\n> > Currently the 'compute_assignment()' function can may read memory out\n> > of bounds, even if used correctly.  Namely this happens when we only\n> > have one column.  In that case we try to calculate the initial\n> > minimum cost using '!j1' as column in the reduction transfer code.\n> > That in turn causes us to try and get the cost from column 1 in the\n> > cost matrix, which does not exist, and thus results in an out of\n> > bounds memory read.\n> \n> This nicely explains what goes wrong.\n> \n> > Instead of trying to intialize the minimum cost from another column,\n> > just set it to INT_MAX.  This also matches what the example code in the\n> > original paper for the algorithm [1] does (it initializes the value to\n> > inf, for which INT_MAX is the closest match in C).\n> \n> Yeah, if we really want to avoid INT_MAX we could use another \"have\n> we found any value yet?\" boolean variable, but the caller in\n> get_correspondences() does not even worry about integer overflows\n> when stuffing diffsize to the cost[] array, and the other possible\n> value that can go to cost[] array is COST_MAX that is mere 65k, so\n> it would be OK to use INT_MAX as sentinel here, I guess.\n\nRight, I'm not sure it would be worth introducing another boolean\nvariable here.  In the normal case we'll always enter the if condition\ninside the loop, and set a reasonable 'min' value.\n\nThat does not happen if we only have one column, and the 'min' will\nremain 'INT_MAX'.  Now in that case it doesn't matter much, as having\nonly one column means there's no possibility to assign anything, so\nthe actual values shouldn't matter (at least that's my understanding\nof the algorithm so far).\n\nAnother improvement we may be able to make here is to not even try to\ncompute the assignment if there's only one column for that reason, but\nI'm out of time today and the rest of my week looks a bit busy, so I\nprobably won't get to do anything before the beginning of next week.\n\n> > Note that the test only fails under valgrind on Linux, but the same\n> > command has been reported to segfault on Mac OS.\n> >\n> > Also start from 0 in the loop, which matches what the example code in\n> > the original paper does as well.  Starting from 1 means we'd ignore\n> > the first column during the reduction transfer phase.  Note that in\n> > the original paper the loop does start from 1, but the implementation\n> > is in Pascal, where arrays are 1 indexed.\n> >\n> > [1]: Jonker, R., & Volgenant, A. (1987). A shortest augmenting path\n> >      algorithm for dense and sparse linear assignment\n> >      problems. Computing, 38(4), 325–340.\n> >\n> > Reported-by: ryenus <ryenus@gmail.com>\n> > Helped-by: Derrick Stolee <stolee@gmail.com>\n> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> > ---\n> >  linear-assignment.c   | 4 ++--\n> >  t/t3206-range-diff.sh | 5 +++++\n> >  2 files changed, 7 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/linear-assignment.c b/linear-assignment.c\n> > index 9b3e56e283..7700b80eeb 100644\n> > --- a/linear-assignment.c\n> > +++ b/linear-assignment.c\n> > @@ -51,8 +51,8 @@ void compute_assignment(int column_count, int row_count, int *cost,\n> >  \t\telse if (j1 < -1)\n> >  \t\t\trow2column[i] = -2 - j1;\n> >  \t\telse {\n> > -\t\t\tint min = COST(!j1, i) - v[!j1];\n> > -\t\t\tfor (j = 1; j < column_count; j++)\n> > +\t\t\tint min = INT_MAX;\n> > +\t\t\tfor (j = 0; j < column_count; j++)\n> >  \t\t\t\tif (j != j1 && min > COST(j, i) - v[j])\n> >  \t\t\t\t\tmin = COST(j, i) - v[j];\n> >  \t\t\tv[j1] -= min;\n> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> > index 2237c7f4af..fb4c13a84a 100755\n> > --- a/t/t3206-range-diff.sh\n> > +++ b/t/t3206-range-diff.sh\n> > @@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n> >  \ttest_cmp expected actual\n> >  '\n> >  \n> > +test_expect_success 'no commits on one side' '\n> > +\tgit commit --amend -m \"new message\" &&\n> > +\tgit range-diff master HEAD@{1} HEAD\n> > +'\n> > +\n> >  test_done\n"},{"id":"358024","messageId":"nycvar.QRO.7.76.6.1809122136020.73@tvgsbejvaqbjf.bet","threadId":"49317","inReplyTo":"20180912190108.GE4865@hank.intra.tgummerer.com","subject":"Re: [PATCH] linear-assignment: fix potential out of bounds memory access (was: Re: Git 2.19 Segmentation fault 11 on macOS)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-09-13T02:38:34Z","receivedAt":"2018-09-13T02:38:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\n[quickly, as I will go back to a proper vacation after this]\n\nOn Wed, 12 Sep 2018, Thomas Gummerer wrote:\n\n> diff --git a/linear-assignment.c b/linear-assignment.c\n> index 9b3e56e283..7700b80eeb 100644\n> --- a/linear-assignment.c\n> +++ b/linear-assignment.c\n> @@ -51,8 +51,8 @@ void compute_assignment(int column_count, int row_count, int *cost,\n>  \t\telse if (j1 < -1)\n>  \t\t\trow2column[i] = -2 - j1;\n>  \t\telse {\n> -\t\t\tint min = COST(!j1, i) - v[!j1];\n> -\t\t\tfor (j = 1; j < column_count; j++)\n> +\t\t\tint min = INT_MAX;\n\nI am worried about this, as I tried very hard to avoid integer overruns.\n\nWouldn't it be possible to replace the `else {` by an appropriate `else if\n(...) { ... } else {`? E.g. `else if (column_count < 2)` or some such?\n\nCiao,\nDscho\n\n> +\t\t\tfor (j = 0; j < column_count; j++)\n>  \t\t\t\tif (j != j1 && min > COST(j, i) - v[j])\n>  \t\t\t\t\tmin = COST(j, i) - v[j];\n>  \t\t\tv[j1] -= min;\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index 2237c7f4af..fb4c13a84a 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success 'no commits on one side' '\n> +\tgit commit --amend -m \"new message\" &&\n> +\tgit range-diff master HEAD@{1} HEAD\n> +'\n> +\n>  test_done\n> -- \n> 2.19.0.397.gdd90340f6a\n> \n> \n"},{"id":"358041","messageId":"CAPig+cRZm6iznBzBF1pwj1v12XX=Q_jzLxSLjWpEMja73r_juw@mail.gmail.com","threadId":"49317","inReplyTo":"20180912190108.GE4865@hank.intra.tgummerer.com","subject":"Re: [PATCH] linear-assignment: fix potential out of bounds memory access (was: Re: Git 2.19 Segmentation fault 11 on macOS)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-09-13T10:14:12Z","receivedAt":"2018-09-13T10:14:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 12, 2018 at 3:01 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:\n> Subject: [PATCH] linear-assignment: fix potential out of bounds memory access\n>\n> Currently the 'compute_assignment()' function can may read memory out\n\n\"can may\"?\n\n> of bounds, even if used correctly.  Namely this happens when we only\n> have one column.  In that case we try to calculate the initial\n> minimum cost using '!j1' as column in the reduction transfer code.\n> That in turn causes us to try and get the cost from column 1 in the\n> cost matrix, which does not exist, and thus results in an out of\n> bounds memory read.\n"},{"id":"358098","messageId":"20180913221318.GE1719@hank.intra.tgummerer.com","threadId":"49317","inReplyTo":"nycvar.QRO.7.76.6.1809122136020.73@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] linear-assignment: fix potential out of bounds memory access (was: Re: Git 2.19 Segmentation fault 11 on macOS)","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-09-13T22:13:18Z","receivedAt":"2018-09-13T22:13:22Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 09/12, Johannes Schindelin wrote:\n> Hi Thomas,\n> \n> [quickly, as I will go back to a proper vacation after this]\n\nSorry about interrupting your vacation, enjoy wherever you are! :)\n\n> On Wed, 12 Sep 2018, Thomas Gummerer wrote:\n> \n> > diff --git a/linear-assignment.c b/linear-assignment.c\n> > index 9b3e56e283..7700b80eeb 100644\n> > --- a/linear-assignment.c\n> > +++ b/linear-assignment.c\n> > @@ -51,8 +51,8 @@ void compute_assignment(int column_count, int row_count, int *cost,\n> >  \t\telse if (j1 < -1)\n> >  \t\t\trow2column[i] = -2 - j1;\n> >  \t\telse {\n> > -\t\t\tint min = COST(!j1, i) - v[!j1];\n> > -\t\t\tfor (j = 1; j < column_count; j++)\n> > +\t\t\tint min = INT_MAX;\n> \n> I am worried about this, as I tried very hard to avoid integer overruns.\n\nAh fair enough, now I think I understand where the calculation of the\ninitial value of min comes from, thanks!\n\n> Wouldn't it be possible to replace the `else {` by an appropriate `else if\n> (...) { ... } else {`? E.g. `else if (column_count < 2)` or some such?\n\nYes, I think that would be possible.  However if we're already special\ncasing \"column_count < 2\", I think we might as well just exit early\nbefore running through the whole algorithm in that case.  If there's\nonly one column, there are no commits that can be assigned to\neachother, as there is only the one.\n\nWe could also just not run call 'compute_assignment' in the first\nplace if column_count == 1, however I'd rather make the function safer\nto call, just in case we find it useful for something else in the\nfuture.\n\nWill send an updated patch in a bit.\n\n> Ciao,\n> Dscho\n> \n> > +\t\t\tfor (j = 0; j < column_count; j++)\n> >  \t\t\t\tif (j != j1 && min > COST(j, i) - v[j])\n> >  \t\t\t\t\tmin = COST(j, i) - v[j];\n> >  \t\t\tv[j1] -= min;\n> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> > index 2237c7f4af..fb4c13a84a 100755\n> > --- a/t/t3206-range-diff.sh\n> > +++ b/t/t3206-range-diff.sh\n> > @@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n> >  \ttest_cmp expected actual\n> >  '\n> >  \n> > +test_expect_success 'no commits on one side' '\n> > +\tgit commit --amend -m \"new message\" &&\n> > +\tgit range-diff master HEAD@{1} HEAD\n> > +'\n> > +\n> >  test_done\n> > -- \n> > 2.19.0.397.gdd90340f6a\n> > \n> > \n"},{"id":"358099","messageId":"20180913223834.GF1719@hank.intra.tgummerer.com","threadId":"49317","inReplyTo":"20180912190108.GE4865@hank.intra.tgummerer.com","subject":"[PATCH v2] linear-assignment: fix potential out of bounds memory access","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-09-13T22:38:34Z","receivedAt":"2018-09-13T22:38:40Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Currently the 'compute_assignment()' function may read memory out\nof bounds, even if used correctly.  Namely this happens when we only\nhave one column.  In that case we try to calculate the initial\nminimum cost using '!j1' as column in the reduction transfer code.\nThat in turn causes us to try and get the cost from column 1 in the\ncost matrix, which does not exist, and thus results in an out of\nbounds memory read.\n\nIn the original paper [1], the example code initializes that minimum\ncost to \"infinite\".  We could emulate something similar by setting the\nminimum cost to INT_MAX, which would result in the same minimum cost\nas the current algorithm, as we'd always go into the if condition at\nleast once, except when we only have one column, and column_count thus\nequals 1.\n\nIf column_count does equal 1, the condition in the loop would always\nbe false, and we'd end up with a minimum of INT_MAX, which may lead to\ninteger overflows later in the algorithm.\n\nFor a column count of 1, we however do not even really need to go\nthrough the whole algorithm.  A column count of 1 means that there's\nno possible assignments, and we can just zero out the column2row and\nrow2column arrays, and return early from the function, while keeping\nthe reduction transfer part of the function the same as it is\ncurrently.\n\nAnother solution would be to just not call the 'compute_assignment()'\nfunction from the range diff code in this case, however it's better to\nmake the compute_assignment function more robust, so future callers\ndon't run into this potential problem.\n\nNote that the test only fails under valgrind on Linux, but the same\ncommand has been reported to segfault on Mac OS.\n\n[1]: Jonker, R., & Volgenant, A. (1987). A shortest augmenting path\n     algorithm for dense and sparse linear assignment\n     problems. Computing, 38(4), 325–340.\n\nReported-by: ryenus <ryenus@gmail.com>\nHelped-by: Derrick Stolee <stolee@gmail.com>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n linear-assignment.c   | 6 ++++++\n t/t3206-range-diff.sh | 5 +++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/linear-assignment.c b/linear-assignment.c\nindex 9b3e56e283..ecffc09be6 100644\n--- a/linear-assignment.c\n+++ b/linear-assignment.c\n@@ -19,6 +19,12 @@ void compute_assignment(int column_count, int row_count, int *cost,\n \tint *free_row, free_count = 0, saved_free_count, *pred, *col;\n \tint i, j, phase;\n \n+\tif (column_count < 2) {\n+\t\tmemset(column2row, 0, sizeof(int) * column_count);\n+\t\tmemset(row2column, 0, sizeof(int) * row_count);\n+\t\treturn;\n+\t}\n+\n \tmemset(column2row, -1, sizeof(int) * column_count);\n \tmemset(row2column, -1, sizeof(int) * row_count);\n \tALLOC_ARRAY(v, column_count);\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 2237c7f4af..fb4c13a84a 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -142,4 +142,9 @@ test_expect_success 'changed message' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'no commits on one side' '\n+\tgit commit --amend -m \"new message\" &&\n+\tgit range-diff master HEAD@{1} HEAD\n+'\n+\n test_done\n-- \n2.19.0.397.gdd90340f6a\n\n"},{"id":"358315","messageId":"20180917184838.GE140909@aiede.svl.corp.google.com","threadId":"49317","inReplyTo":"20180913223834.GF1719@hank.intra.tgummerer.com","subject":"Re: [PATCH v2] linear-assignment: fix potential out of bounds memory access","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-17T18:48:38Z","receivedAt":"2018-09-17T18:48:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Gummerer wrote:\n\n> Currently the 'compute_assignment()' function may read memory out\n> of bounds, even if used correctly.  Namely this happens when we only\n> have one column.\n[...]\n> Reported-by: ryenus <ryenus@gmail.com>\n> Helped-by: Derrick Stolee <stolee@gmail.com>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n>  linear-assignment.c   | 6 ++++++\n>  t/t3206-range-diff.sh | 5 +++++\n>  2 files changed, 11 insertions(+)\n\nHere's a bit of a non-review, but hopefully it pokes others into\ndoing a proper review.\n\nI haven't carefully examined the checks you're adding here.  Your\ngoals seem to be right.  I've been running with this patch since last\nThursday, with no problems yet.  Thanks for writing it.\n\nSincerely,\nJonathan\n"}]}