{"thread":{"id":"47528","subject":"[BUG] v2.16.0-rc0 seg faults when git bisect skip","startedAt":"2018-01-03T12:36:32Z","lastAt":"2018-01-09T11:09:12Z","messageCount":14,"participants":["Yasushi SHOJI","Ævar Arnfjörð Bjarmason","Martin Ågren","Junio C Hamano","Christian Couder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"335713","messageId":"CAELBRWKYZK2tVhGZSExXRXrRB94jFN-4N3d=iKARzEsqLu7k1g@mail.gmail.com","threadId":"47528","inReplyTo":null,"subject":"[BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Yasushi SHOJI","fromEmail":"yasushi.shoji@gmail.com","sentAt":"2018-01-03T12:36:21Z","receivedAt":"2018-01-03T12:36:32Z","isPatch":false,"sender":{"key":"yasushi.shoji@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10152?v=4"},"body":"Hi,\n\ngit version 2.16.0.rc0 seg faults on my machine when I\n\ngit bisect skip\n\nHere is a back trace:\n\n$ /opt/mygit/bin/git --version\ngit version 2.16.0.rc0\n\n$ /opt/mygit/bin/git bisect skip\nSegmentation fault (core dumped)\n\n$ gdb /opt/mygit/bin/git core\nGNU gdb (Debian 7.12-6+b1) 7.12.0.20161007-git\nCopyright (C) 2016 Free Software Foundation, Inc.\nLicense GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>\nThis is free software: you are free to change and redistribute it.\nThere is NO WARRANTY, to the extent permitted by law.  Type \"show copying\"\nand \"show warranty\" for details.\nThis GDB was configured as \"x86_64-linux-gnu\".\nType \"show configuration\" for configuration details.\nFor bug reporting instructions, please see:\n<http://www.gnu.org/software/gdb/bugs/>.\nFind the GDB manual and other documentation resources online at:\n<http://www.gnu.org/software/gdb/documentation/>.\nFor help, type \"help\".\nType \"apropos word\" to search for commands related to \"word\"...\nReading symbols from /opt/mygit/bin/git...done.\n[New LWP 5211]\n[Thread debugging using libthread_db enabled]\nUsing host libthread_db library \"/lib/x86_64-linux-gnu/libthread_db.so.1\".\nCore was generated by `git bisect--helper --next-all'.\nProgram terminated with signal SIGSEGV, Segmentation fault.\n#0  0x000055a73107f900 in best_bisection_sorted (list=0x0, nr=0) at bisect.c:232\n232 free_commit_list(p->next);\n(gdb) bt\n#0  0x000055a73107f900 in best_bisection_sorted (list=0x0, nr=0) at bisect.c:232\n#1  0x000055a73107fc0f in do_find_bisection (list=0x0, nr=0,\nweights=0x55a731b6ffd0, find_all=1) at bisect.c:361\n#2  0x000055a73107fcf4 in find_bisection (commit_list=0x7ffe8750d4d0,\nreaches=0x7ffe8750d4c4, all=0x7ffe8750d4c0, find_all=1) at\nbisect.c:400\n#3  0x000055a73108128d in bisect_next_all (prefix=0x0, no_checkout=0)\nat bisect.c:969\n#4  0x000055a730fd5238 in cmd_bisect__helper (argc=0,\nargv=0x7ffe8750e230, prefix=0x0) at builtin/bisect--helper.c:140\n#5  0x000055a730fcbc76 in run_builtin (p=0x55a73145c778\n<commands+120>, argc=2, argv=0x7ffe8750e230) at git.c:346\n#6  0x000055a730fcbf40 in handle_builtin (argc=2, argv=0x7ffe8750e230)\nat git.c:554\n#7  0x000055a730fcc0e8 in run_argv (argcp=0x7ffe8750e0ec,\nargv=0x7ffe8750e0e0) at git.c:606\n#8  0x000055a730fcc29b in cmd_main (argc=2, argv=0x7ffe8750e230) at git.c:683\n#9  0x000055a731068d9e in main (argc=3, argv=0x7ffe8750e228) at common-main.c:43\n(gdb) p p\n$1 = (struct commit_list *) 0x0\n\nAs you can see, the code dereferences to the 'next' while 'p' is NULL.\n\nI'm sure I did 'git bisect good' after git _found_ bad commit.  Then I\ntyped 'git bisect skip' on the commit 726804874 of guile repository.\nIf that matters at all.\n\nI haven't touched guile repo to preserve the current state.\n\nLet me know if you need any other info.\n-- \n          yashi\n"},{"id":"335716","messageId":"87fu7nc9a2.fsf@evledraar.gmail.com","threadId":"47528","inReplyTo":"CAELBRWKYZK2tVhGZSExXRXrRB94jFN-4N3d=iKARzEsqLu7k1g@mail.gmail.com","subject":"Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-03T14:21:57Z","receivedAt":"2018-01-03T14:22:07Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 03 2018, Yasushi SHOJI jotted:\n\n> Hi,\n>\n> git version 2.16.0.rc0 seg faults on my machine when I\n> [...]\n> Program terminated with signal SIGSEGV, Segmentation fault.\n> #0  0x000055a73107f900 in best_bisection_sorted (list=0x0, nr=0) at bisect.c:232\n> 232 free_commit_list(p->next);\n> (gdb) bt\n> #0  0x000055a73107f900 in best_bisection_sorted (list=0x0, nr=0) at bisect.c:232\n> #1  0x000055a73107fc0f in do_find_bisection (list=0x0, nr=0,\n> weights=0x55a731b6ffd0, find_all=1) at bisect.c:361\n> #2  0x000055a73107fcf4 in find_bisection (commit_list=0x7ffe8750d4d0,\n> reaches=0x7ffe8750d4c4, all=0x7ffe8750d4c0, find_all=1) at\n> bisect.c:400\n> #3  0x000055a73108128d in bisect_next_all (prefix=0x0, no_checkout=0)\n> at bisect.c:969\n> #4  0x000055a730fd5238 in cmd_bisect__helper (argc=0,\n> argv=0x7ffe8750e230, prefix=0x0) at builtin/bisect--helper.c:140\n> #5  0x000055a730fcbc76 in run_builtin (p=0x55a73145c778\n> <commands+120>, argc=2, argv=0x7ffe8750e230) at git.c:346\n> #6  0x000055a730fcbf40 in handle_builtin (argc=2, argv=0x7ffe8750e230)\n> at git.c:554\n> #7  0x000055a730fcc0e8 in run_argv (argcp=0x7ffe8750e0ec,\n> argv=0x7ffe8750e0e0) at git.c:606\n> #8  0x000055a730fcc29b in cmd_main (argc=2, argv=0x7ffe8750e230) at git.c:683\n> #9  0x000055a731068d9e in main (argc=3, argv=0x7ffe8750e228) at common-main.c:43\n> (gdb) p p\n> $1 = (struct commit_list *) 0x0\n>\n> As you can see, the code dereferences to the 'next' while 'p' is NULL.\n>\n> I'm sure I did 'git bisect good' after git _found_ bad commit.  Then I\n> typed 'git bisect skip' on the commit 726804874 of guile repository.\n> If that matters at all.\n>\n> I haven't touched guile repo to preserve the current state.\n\nI can't reproduce this myself, but looking at the backtrace it seems\npretty obvious that 7c117184d7 (\"bisect: fix off-by-one error in\n`best_bisection_sorted()`\", 2017-11-05) is the culprit.\n\nThat changed more careful code added by Christian in 50e62a8e70\n(\"rev-list: implement --bisect-all\", 2007-10-22) to free a pointer which\nas you can see can be NULL.\n\nIf you can test a patch to see if it works this should fix it:\n\ndiff --git a/bisect.c b/bisect.c\nindex 0fca17c02b..2f3008b078 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -229,8 +229,10 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n                if (i < cnt - 1)\n                        p = p->next;\n        }\n-       free_commit_list(p->next);\n-       p->next = NULL;\n+       if (p) {\n+               free_commit_list(p->next);\n+               p->next = NULL;\n+       }\n        strbuf_release(&buf);\n        free(array);\n        return list;\n\nBut given the commit message by Martin maybe there's some deeper bug here.\n"},{"id":"335765","messageId":"CAN0heSrZ4dEFqNX69PgtGCERJKabokz88v-vnNZkUBXfK118mg@mail.gmail.com","threadId":"47528","inReplyTo":"87fu7nc9a2.fsf@evledraar.gmail.com","subject":"Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-01-03T18:26:06Z","receivedAt":"2018-01-03T18:26:12Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 3 January 2018 at 15:21, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> On Wed, Jan 03 2018, Yasushi SHOJI jotted:\n>\n>> Hi,\n>>\n>> git version 2.16.0.rc0 seg faults on my machine when I\n>> [...]\n>> Program terminated with signal SIGSEGV, Segmentation fault.\n>> #0  0x000055a73107f900 in best_bisection_sorted (list=0x0, nr=0) at bisect.c:232\n>> 232 free_commit_list(p->next);\n>> (gdb) bt\n>> #0  0x000055a73107f900 in best_bisection_sorted (list=0x0, nr=0) at bisect.c:232\n>> #1  0x000055a73107fc0f in do_find_bisection (list=0x0, nr=0,\n>> weights=0x55a731b6ffd0, find_all=1) at bisect.c:361\n>> #2  0x000055a73107fcf4 in find_bisection (commit_list=0x7ffe8750d4d0,\n>> reaches=0x7ffe8750d4c4, all=0x7ffe8750d4c0, find_all=1) at\n>> bisect.c:400\n>> #3  0x000055a73108128d in bisect_next_all (prefix=0x0, no_checkout=0)\n>> at bisect.c:969\n>> #4  0x000055a730fd5238 in cmd_bisect__helper (argc=0,\n>> argv=0x7ffe8750e230, prefix=0x0) at builtin/bisect--helper.c:140\n>> #5  0x000055a730fcbc76 in run_builtin (p=0x55a73145c778\n>> <commands+120>, argc=2, argv=0x7ffe8750e230) at git.c:346\n>> #6  0x000055a730fcbf40 in handle_builtin (argc=2, argv=0x7ffe8750e230)\n>> at git.c:554\n>> #7  0x000055a730fcc0e8 in run_argv (argcp=0x7ffe8750e0ec,\n>> argv=0x7ffe8750e0e0) at git.c:606\n>> #8  0x000055a730fcc29b in cmd_main (argc=2, argv=0x7ffe8750e230) at git.c:683\n>> #9  0x000055a731068d9e in main (argc=3, argv=0x7ffe8750e228) at common-main.c:43\n>> (gdb) p p\n>> $1 = (struct commit_list *) 0x0\n>>\n>> As you can see, the code dereferences to the 'next' while 'p' is NULL.\n>>\n>> I'm sure I did 'git bisect good' after git _found_ bad commit.  Then I\n>> typed 'git bisect skip' on the commit 726804874 of guile repository.\n>> If that matters at all.\n>>\n>> I haven't touched guile repo to preserve the current state.\n>\n> I can't reproduce this myself, but looking at the backtrace it seems\n> pretty obvious that 7c117184d7 (\"bisect: fix off-by-one error in\n> `best_bisection_sorted()`\", 2017-11-05) is the culprit.\n>\n> That changed more careful code added by Christian in 50e62a8e70\n> (\"rev-list: implement --bisect-all\", 2007-10-22) to free a pointer which\n> as you can see can be NULL.\n>\n> If you can test a patch to see if it works this should fix it:\n>\n> diff --git a/bisect.c b/bisect.c\n> index 0fca17c02b..2f3008b078 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -229,8 +229,10 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n>                 if (i < cnt - 1)\n>                         p = p->next;\n>         }\n> -       free_commit_list(p->next);\n> -       p->next = NULL;\n> +       if (p) {\n> +               free_commit_list(p->next);\n> +               p->next = NULL;\n> +       }\n>         strbuf_release(&buf);\n>         free(array);\n>         return list;\n>\n> But given the commit message by Martin maybe there's some deeper bug here.\n\nI haven't tried to reproduce, or tested the patch, but from the looks of\nit, your analysis and fix are both spot on. The special case that yashi\nhas hit is that `list` is NULL. The old code handled that very well, the\ncode after my patch ... not so well. The loop-sort-loop pattern reduces\nto a no-op, both before and after my patch. But what I failed to realize\nwas that `list` could be NULL.\n\nThis could be fixed by an early return if `list` is NULL, but that would\nalso need some memory-handling. So I think your patch is just as good or\nbetter, since it can be seen as restoring what was lost in 7c117184d7.\n\nThanks both, and sorry for this.\nMartin\n"},{"id":"335766","messageId":"20180103184852.27271-1-avarab@gmail.com","threadId":"47528","inReplyTo":"CAN0heSrZ4dEFqNX69PgtGCERJKabokz88v-vnNZkUBXfK118mg@mail.gmail.com","subject":"[PATCH] bisect: fix a regression causing a segfault","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-03T18:48:52Z","receivedAt":"2018-01-03T18:49:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In 7c117184d7 (\"bisect: fix off-by-one error in\n`best_bisection_sorted()`\", 2017-11-05) the more careful logic dealing\nwith freeing p->next in 50e62a8e70 (\"rev-list: implement\n--bisect-all\", 2007-10-22) was removed.\n\nRestore the more careful check to avoid segfaulting. Ideally this\nwould come with a test case, but we don't have steps to reproduce\nthis, only a backtrace from gdb pointing to this being the issue.\n\nReported-by: Yasushi SHOJI <yasushi.shoji@gmail.com>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n bisect.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 0fca17c02b..2f3008b078 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -229,8 +229,10 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n \t\tif (i < cnt - 1)\n \t\t\tp = p->next;\n \t}\n-\tfree_commit_list(p->next);\n-\tp->next = NULL;\n+\tif (p) {\n+\t\tfree_commit_list(p->next);\n+\t\tp->next = NULL;\n+\t}\n \tstrbuf_release(&buf);\n \tfree(array);\n \treturn list;\n-- \n2.15.1.424.g9478a66081\n\n"},{"id":"335900","messageId":"CAELBRWK6Y=-7WBwai16dBKd8OLxdXWOiZMALVJXrP9ak8gF-LA@mail.gmail.com","threadId":"47528","inReplyTo":"CAN0heSrZ4dEFqNX69PgtGCERJKabokz88v-vnNZkUBXfK118mg@mail.gmail.com","subject":"Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Yasushi SHOJI","fromEmail":"yasushi.shoji@gmail.com","sentAt":"2018-01-05T02:45:43Z","receivedAt":"2018-01-05T02:45:52Z","isPatch":false,"sender":{"key":"yasushi.shoji@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10152?v=4"},"body":"Hi,\n\nOn Thu, Jan 4, 2018 at 3:26 AM, Martin Ågren <martin.agren@gmail.com> wrote:\n> On 3 January 2018 at 15:21, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>>\n>> On Wed, Jan 03 2018, Yasushi SHOJI jotted:\n>>\n>>> Hi,\n>>>\n>>> git version 2.16.0.rc0 seg faults on my machine when I\n>>> [...]\n>>> Program terminated with signal SIGSEGV, Segmentation fault.\n>>> #0  0x000055a73107f900 in best_bisection_sorted (list=0x0, nr=0) at bisect.c:232\n>>> 232 free_commit_list(p->next);\n>>> (gdb) bt\n>>> #0  0x000055a73107f900 in best_bisection_sorted (list=0x0, nr=0) at bisect.c:232\n>>> #1  0x000055a73107fc0f in do_find_bisection (list=0x0, nr=0,\n>>> weights=0x55a731b6ffd0, find_all=1) at bisect.c:361\n>>> #2  0x000055a73107fcf4 in find_bisection (commit_list=0x7ffe8750d4d0,\n>>> reaches=0x7ffe8750d4c4, all=0x7ffe8750d4c0, find_all=1) at\n>>> bisect.c:400\n>>> #3  0x000055a73108128d in bisect_next_all (prefix=0x0, no_checkout=0)\n>>> at bisect.c:969\n>>> #4  0x000055a730fd5238 in cmd_bisect__helper (argc=0,\n>>> argv=0x7ffe8750e230, prefix=0x0) at builtin/bisect--helper.c:140\n>>> #5  0x000055a730fcbc76 in run_builtin (p=0x55a73145c778\n>>> <commands+120>, argc=2, argv=0x7ffe8750e230) at git.c:346\n>>> #6  0x000055a730fcbf40 in handle_builtin (argc=2, argv=0x7ffe8750e230)\n>>> at git.c:554\n>>> #7  0x000055a730fcc0e8 in run_argv (argcp=0x7ffe8750e0ec,\n>>> argv=0x7ffe8750e0e0) at git.c:606\n>>> #8  0x000055a730fcc29b in cmd_main (argc=2, argv=0x7ffe8750e230) at git.c:683\n>>> #9  0x000055a731068d9e in main (argc=3, argv=0x7ffe8750e228) at common-main.c:43\n>>> (gdb) p p\n>>> $1 = (struct commit_list *) 0x0\n>>>\n>>> As you can see, the code dereferences to the 'next' while 'p' is NULL.\n>>>\n>>> I'm sure I did 'git bisect good' after git _found_ bad commit.  Then I\n>>> typed 'git bisect skip' on the commit 726804874 of guile repository.\n>>> If that matters at all.\n>>>\n>>> I haven't touched guile repo to preserve the current state.\n>>\n>> I can't reproduce this myself, but looking at the backtrace it seems\n>> pretty obvious that 7c117184d7 (\"bisect: fix off-by-one error in\n>> `best_bisection_sorted()`\", 2017-11-05) is the culprit.\n>>\n>> That changed more careful code added by Christian in 50e62a8e70\n>> (\"rev-list: implement --bisect-all\", 2007-10-22) to free a pointer which\n>> as you can see can be NULL.\n>>\n>> If you can test a patch to see if it works this should fix it:\n>>\n>> diff --git a/bisect.c b/bisect.c\n>> index 0fca17c02b..2f3008b078 100644\n>> --- a/bisect.c\n>> +++ b/bisect.c\n>> @@ -229,8 +229,10 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n>>                 if (i < cnt - 1)\n>>                         p = p->next;\n>>         }\n>> -       free_commit_list(p->next);\n>> -       p->next = NULL;\n>> +       if (p) {\n>> +               free_commit_list(p->next);\n>> +               p->next = NULL;\n>> +       }\n>>         strbuf_release(&buf);\n>>         free(array);\n>>         return list;\n>>\n>> But given the commit message by Martin maybe there's some deeper bug here.\n>\n> I haven't tried to reproduce, or tested the patch, but from the looks of\n> it, your analysis and fix are both spot on. The special case that yashi\n> has hit is that `list` is NULL. The old code handled that very well, the\n> code after my patch ... not so well. The loop-sort-loop pattern reduces\n> to a no-op, both before and after my patch. But what I failed to realize\n> was that `list` could be NULL.\n\nThe patch (actually, I've tested the one in pu, 2e9fdc795cb27) avoids\nthe seg fault for sure, but the question is:\n\nWhen does the list allowed to contain NULLs?\n\nSince nobody noticed it since 7c117184d7, it must be a rare case, right?\n\nWould you guys elaborate a bit?  I don't have any insight how\nbest_bisection_sorted() should work and what the list should contain.\nSo that I can make a test case.\n\nThanks,\n-- \n               yashi\n"},{"id":"335909","messageId":"CAELBRWLyZ5s2Wvk2U5Ks2adx23Z+51dbEs3QbAq+5nGLe21srQ@mail.gmail.com","threadId":"47528","inReplyTo":"CAELBRWK6Y=-7WBwai16dBKd8OLxdXWOiZMALVJXrP9ak8gF-LA@mail.gmail.com","subject":"Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Yasushi SHOJI","fromEmail":"yasushi.shoji@gmail.com","sentAt":"2018-01-05T05:28:29Z","receivedAt":"2018-01-05T05:28:36Z","isPatch":false,"sender":{"key":"yasushi.shoji@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10152?v=4"},"body":"Hi,\n\nOn Fri, Jan 5, 2018 at 11:45 AM, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:\n> So that I can make a test case.\n\nOK, here is the step to reproduce on git.git\n\n$ git bisect start\n$ git bisect bad v2.16.0-rc0\n$ git bisect good 0433d533f13671f4313c31e34707f0f5281a18e0\n$ git bisect good\n$ git bisect bad\n$ git bisect bad\n$ git bisect skip #=> works with error\n$ git bisect good #=> Segmentation fault\n$ git bisect skip #=> Segmentation fault\n\nThe following is with full outputs, just in case you need it.\n\n$ git describe\nv2.16.0-rc0\n\n$ git bisect start\n$ git bisect bad v2.16.0-rc0\n\n$ git bisect good 0433d533f13671f4313c31e34707f0f5281a18e0\nBisecting: 4 revisions left to test after this (roughly 2 steps)\n[c87b653c46c4455561642b14efc8920a0b3e44b9] builtin/describe.c: rename\n`oid` to avoid variable shadowing\n\n$ git bisect good\nBisecting: 2 revisions left to test after this (roughly 1 step)\n[4dbc59a4cce418ff8428a9d2ecd67c34ca50db56] builtin/describe.c: factor\nout describe_commit\n\n$ git bisect bad\nBisecting: 0 revisions left to test after this (roughly 0 steps)\n[cdaed0cf023a47cae327671fae11c10d88100ee7] builtin/describe.c: print\ndebug statements earlier\n\n$ git bisect bad\ncdaed0cf023a47cae327671fae11c10d88100ee7 is the first bad commit\ncommit cdaed0cf023a47cae327671fae11c10d88100ee7\nAuthor: Stefan Beller <sbeller@google.com>\nDate:   Wed Nov 15 18:00:37 2017 -0800\n\n    builtin/describe.c: print debug statements earlier\n\n    When debugging, print the received argument at the start of the\n    function instead of in the middle. This ensures that the received\n    argument is printed in all code paths, and also allows a subsequent\n    refactoring to not need to move the \"arg\" parameter.\n\n    Signed-off-by: Stefan Beller <sbeller@google.com>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\n:040000 040000 beb1f2c4d9ee7584e54e0489c7e85e348cbf9fc7\nb9882eea0772e3025690d3513cea5a940567668e M builtin\n\n$ git bisect skip\nThere are only 'skip'ped commits left to test.\nThe first bad commit could be any of:\ncdaed0cf023a47cae327671fae11c10d88100ee7\nWe cannot bisect more!\n\n$ git bisect good\nSegmentation fault\n\n$ git bisect skip\nSegmentation fault\n-- \n               yashi\n"},{"id":"336022","messageId":"xmqqincgaqxw.fsf@gitster.mtv.corp.google.com","threadId":"47528","inReplyTo":"CAELBRWK6Y=-7WBwai16dBKd8OLxdXWOiZMALVJXrP9ak8gF-LA@mail.gmail.com","subject":"Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-05T22:20:11Z","receivedAt":"2018-01-05T22:20:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yasushi SHOJI <yasushi.shoji@gmail.com> writes:\n\n> The patch (actually, I've tested the one in pu, 2e9fdc795cb27) avoids\n> the seg fault for sure, but the question is:\n>\n> When does the list allowed to contain NULLs?\n\nA very legitimate question.  With the proposed log message alone, it\nis even tempting to declare that the change may merely be sweeping\nthe issue under the rug.  A bit better explanation is needed, at\nleast.\n"},{"id":"336045","messageId":"20180106082116.11057-1-martin.agren@gmail.com","threadId":"47528","inReplyTo":"CAELBRWLyZ5s2Wvk2U5Ks2adx23Z+51dbEs3QbAq+5nGLe21srQ@mail.gmail.com","subject":"Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-01-06T08:21:16Z","receivedAt":"2018-01-06T08:21:50Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"> On Fri, Jan 5, 2018 at 11:45 AM, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:\n>> When does the list allowed to contain NULLs?\n\nShort answer: there are no commits left to test.\n\nThe list is built in the for-loop in `find_bisection()`. So the\ntechnical answer is: if all commits in the initial list `commit_list`\nare UNINTERESTING (including if `commit_list` is empty to begin with).\n\nIt's also helpful to study where we should end up from there. We should\ntake the `if (!revs.commits)` branch in `bisect_next_all()`. That is, we\nshould print either \"There are only 'skip'ped commits left to test. The\nfirst bad commit could be any of:\" or \"<commit> was both good and bad\".\n\n>> Since nobody noticed it since 7c117184d7, it must be a rare case, right?\n\nRight, you marked a commit both good and bad. That's probably not very\ncommon. But it obviously happens. :-)\n\nOn 5 January 2018 at 06:28, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:\n> OK, here is the step to reproduce on git.git\n\nThank you for providing a script for reproducing this. It helped me come\nup with the attached patch. The patch is based on ma/bisect-leakfix,\nwhich includes Ævar's patch.\n\nI think this patch could be useful, either as a final \"let's test\nsomething previously non-tested; this would have caught the segfault\",\nor simply squashed into Ævar's patch as a \"let's add a test that would\nhave caught this, and which also tests a previously non-tested code\npath.\"\n\nThanks for digging and finding a reproduction recipe.\n\nMartin\n\n-- >8 --\nSubject: [PATCH] bisect: add test for marking commit both good and bad\n\nSince 670f5fe34f ([PATCH] Fix bisection terminating condition,\n2005-08-30), we have noticed and complained when a commit was marked\nboth good and bad. But we had no tests for this behavior.\n\nTest the behavior when we mark a commit first bad, then good, but also\nwhen we are already in the bad state, where `git bisect skip` should\nnotice it.\n\nThis test would have caught the segfault which was recently fixed in\n2e9fdc795c (bisect: fix a regression causing a segfault, 2018-01-03).\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n t/t6030-bisect-porcelain.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 8c2c6eaef8..190f0ce0ab 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -894,4 +894,13 @@ test_expect_success 'bisect start takes options and revs in any order' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'marking commit both good and bad gets reported' '\n+\tgit bisect reset &&\n+\tgit bisect start HEAD &&\n+\ttest_must_fail git bisect good HEAD >out &&\n+\ttest_i18ngrep \"both good and bad\" out &&\n+\ttest_must_fail git bisect skip >out &&\n+\ttest_i18ngrep \"both good and bad\" out\n+'\n+\n test_done\n-- \n2.16.0.rc1\n\n"},{"id":"336054","messageId":"CAELBRWJJYfRUxkzoeFfHQbSL5xzPQKt4srdoge=4K4n=ChN-TA@mail.gmail.com","threadId":"47528","inReplyTo":"20180106082116.11057-1-martin.agren@gmail.com","subject":"Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Yasushi SHOJI","fromEmail":"yasushi.shoji@gmail.com","sentAt":"2018-01-06T14:27:13Z","receivedAt":"2018-01-06T14:27:21Z","isPatch":false,"sender":{"key":"yasushi.shoji@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10152?v=4"},"body":"Hi Martin,\n\nThank you for your comment.\nI haven't have time to read the code carefully so bare with me.\n\nOn Sat, Jan 6, 2018 at 5:21 PM, Martin Ågren <martin.agren@gmail.com> wrote:\n>> On Fri, Jan 5, 2018 at 11:45 AM, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:\n>>> When does the list allowed to contain NULLs?\n>\n> Short answer: there are no commits left to test.\n>\n> The list is built in the for-loop in `find_bisection()`. So the\n> technical answer is: if all commits in the initial list `commit_list`\n> are UNINTERESTING (including if `commit_list` is empty to begin with).\n>\n> It's also helpful to study where we should end up from there. We should\n> take the `if (!revs.commits)` branch in `bisect_next_all()`. That is, we\n> should print either \"There are only 'skip'ped commits left to test. The\n> first bad commit could be any of:\" or \"<commit> was both good and bad\".\n\nbest_bisection_sorted() seems to do\n\n - get the commit list along with the number of elements in the list\n - walk the list one by one to check whether a element have TREESAME or not\n - if TREESAME, skip\n - if not, add it to array\n - sort the array by distance\n - put elements back to the list\n\nso, if you find TREESAME, you get less elements than given, right?\nAlso, if you sort, the last commit, which has NULL in the ->next,\nmight get in the middle of the array??\n\n# BTW, is it really fast to use QSORT even if you have to convert to\n# an array from list?\n\n>>> Since nobody noticed it since 7c117184d7, it must be a rare case, right?\n>\n> Right, you marked a commit both good and bad. That's probably not very\n> common. But it obviously happens. :-)\n>\n> On 5 January 2018 at 06:28, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:\n>> OK, here is the step to reproduce on git.git\n>\n> Thank you for providing a script for reproducing this. It helped me come\n> up with the attached patch. The patch is based on ma/bisect-leakfix,\n> which includes Ævar's patch.\n>\n> I think this patch could be useful, either as a final \"let's test\n> something previously non-tested; this would have caught the segfault\",\n> or simply squashed into Ævar's patch as a \"let's add a test that would\n> have caught this, and which also tests a previously non-tested code\n> path.\"\n\nDo we really need that?  What is a practical use of a commit having\nboth good and bad?\n\nRegards,\n-- \n            yashi\n"},{"id":"336055","messageId":"CAN0heSosr2xgVrfDNLi3XFY=TXuHGaJMXHLG=i=Hf7doD1vP1Q@mail.gmail.com","threadId":"47528","inReplyTo":"CAELBRWJJYfRUxkzoeFfHQbSL5xzPQKt4srdoge=4K4n=ChN-TA@mail.gmail.com","subject":"Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-01-06T15:02:34Z","receivedAt":"2018-01-06T15:02:41Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 6 January 2018 at 15:27, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:\n> best_bisection_sorted() seems to do\n>\n>  - get the commit list along with the number of elements in the list\n>  - walk the list one by one to check whether a element have TREESAME or not\n>  - if TREESAME, skip\n>  - if not, add it to array\n>  - sort the array by distance\n>  - put elements back to the list\n>\n> so, if you find TREESAME, you get less elements than given, right?\n\nAll of this matches my understanding.\n\n> Also, if you sort, the last commit, which has NULL in the ->next,\n> might get in the middle of the array??\n\nThis is a bit tricky. The elements of the linked list are not sorted.\nInstead, the items of the linked list are copied into an array and\nsorted. The result is then put into a linked list.\n\nOr actually, into the *same* list. That's an optimization to avoid\ndeallocating all objects in the list, then allocate (roughly) the same\nnumber of objects again. It means all the `next`-pointers are already\nset up, and we just need to set the final one to NULL. (It will already\nbe NULL if and only if the new list has the same length as the old,\ni.e., if we didn't skip any TREESAME-commit.)\n\n> # BTW, is it really fast to use QSORT even if you have to convert to\n> # an array from list?\n\nYou'll find some QSORT/qsort-stuff in git-compat-util.h. We may or may\nnot end up doing an actual \"quick sort\". That would depend on, e.g., how\nthe C-library implements `qsort()`. Also, I'd guess that even if we have\nroom for an improvement, those cases (small `cnt`!) are already fast\nenough that no-one really cares. That is, maybe we could measure a\nspeed-up, but would anyone really *notice* it?\n\n>> Thank you for providing a script for reproducing this. It helped me come\n>> up with the attached patch. The patch is based on ma/bisect-leakfix,\n>> which includes Ævar's patch.\n>>\n>> I think this patch could be useful, either as a final \"let's test\n>> something previously non-tested; this would have caught the segfault\",\n>> or simply squashed into Ævar's patch as a \"let's add a test that would\n>> have caught this, and which also tests a previously non-tested code\n>> path.\"\n>\n> Do we really need that?  What is a practical use of a commit having\n> both good and bad?\n\nThere is not much practical use for *having* it. But there might be some\nuse in *detecting* it. Linus wrote in 670f5fe34f: \"Also, add a safety\nnet: if somebody marks as good something that includes the known-bad\npoint, we now notice and complain, instead of writing an empty revision\nto the new bisection branch.\"\n\nSo there might be some value in verifying that the safety-net works and\ntells the user something useful (as opposed to Git segfaulting ;-) ).\n\nRegards,\nMartin\n"},{"id":"336056","messageId":"CAP8UFD03-EPxQQk51RZgb9Osdb4P=B67CLU6PrEP0O9chcHOCw@mail.gmail.com","threadId":"47528","inReplyTo":"CAELBRWJJYfRUxkzoeFfHQbSL5xzPQKt4srdoge=4K4n=ChN-TA@mail.gmail.com","subject":"Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-01-06T15:05:37Z","receivedAt":"2018-01-06T15:05:44Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Yasushi,\n\nOn Sat, Jan 6, 2018 at 3:27 PM, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:\n\n> best_bisection_sorted() seems to do\n>\n>  - get the commit list along with the number of elements in the list\n>  - walk the list one by one to check whether a element have TREESAME or not\n>  - if TREESAME, skip\n>  - if not, add it to array\n>  - sort the array by distance\n>  - put elements back to the list\n\nYes.\n\n> so, if you find TREESAME, you get less elements than given, right?\n\nYes.\n\n> Also, if you sort, the last commit, which has NULL in the ->next,\n> might get in the middle of the array??\n\nWell, first the array is just necessary to sort the items pointed to\nby the list. It is freed at the end of the function. We are mostly\ninterested in getting a sorted list from this function, not a sorted\narray.\n\nAlso the array contains only items (commits) and distances, not list\nelements, so the elements in the array don't have a ->next pointer.\n\nAbout items in the list, when we put them back into the list we only\nchange p->item, so p->next still points to the next one. Only for the\nlast one we set p->next to NULL (if p is not NULL). So we don't\nactually sort the list elements, we sort the items pointed to by the\nlist.\n\n> # BTW, is it really fast to use QSORT even if you have to convert to\n> # an array from list?\n\nIt is easier and probably faster to just use qsort, which we get from\n#include <stdlib.h>, as it is hopefully optimized, rather than\nreimplementing our own list sorting.\n\n>>>> Since nobody noticed it since 7c117184d7, it must be a rare case, right?\n>>\n>> Right, you marked a commit both good and bad. That's probably not very\n>> common. But it obviously happens. :-)\n\nYeah, mistakes unfortunately happens :-)\n\n>> Thank you for providing a script for reproducing this. It helped me come\n>> up with the attached patch. The patch is based on ma/bisect-leakfix,\n>> which includes Ævar's patch.\n>>\n>> I think this patch could be useful, either as a final \"let's test\n>> something previously non-tested; this would have caught the segfault\",\n>> or simply squashed into Ævar's patch as a \"let's add a test that would\n>> have caught this, and which also tests a previously non-tested code\n>> path.\"\n>\n> Do we really need that?  What is a practical use of a commit having\n> both good and bad?\n\nIt is not practical, but it is a user mistake that happens.\n\nThanks for your reports and test cases,\nChristian.\n"},{"id":"336146","messageId":"CAELBRWLV7wQm_b9Lvj67KiAMAxq6vNOzA3z62S-f5=3A5LyjZQ@mail.gmail.com","threadId":"47528","inReplyTo":"CAP8UFD03-EPxQQk51RZgb9Osdb4P=B67CLU6PrEP0O9chcHOCw@mail.gmail.com","subject":"Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Yasushi SHOJI","fromEmail":"yasushi.shoji@gmail.com","sentAt":"2018-01-08T14:45:12Z","receivedAt":"2018-01-08T14:45:21Z","isPatch":false,"sender":{"key":"yasushi.shoji@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10152?v=4"},"body":"Hi all,\n\nThank you guys for insightful help.  I just read the code and now I understand\nwhat you guys are saying.  Yeah, I can say the fix is \"spot on\".\n\nBut, to be honest, it's hard to see why you need \"if (p)\" at  the first glance.\nSo, how about this fix, instead?\n\nI also found a bug in show_list().  I'm attaching a patch as well.\n-- \n           yashi\n\n\nFrom d149a1348e94ea0246a10181751ce3bf9ba48198 Mon Sep 17 00:00:00 2001\nFrom: Yasushi SHOJI <yashi@atmark-techno.com>\nDate: Mon, 8 Jan 2018 22:31:10 +0900\nSubject: [PATCH 1/2] bisect: debug: convert struct object to object_id\n\nThe commit f2fd0760f62e79609fef7bfd7ecebb002e8e4ced converted struct\nobject to object_id but a debug function show_list(), which is\nifdef'ed to noop, in bisect.c wasn't.\n\nSo fix it.\n---\n bisect.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 0fca17c02..fb3e44903 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -132,7 +132,7 @@ static void show_list(const char *debug, int counted, int nr,\n \t\tunsigned flags = commit->object.flags;\n \t\tenum object_type type;\n \t\tunsigned long size;\n-\t\tchar *buf = read_sha1_file(commit->object.sha1, &type, &size);\n+\t\tchar *buf = read_sha1_file(commit->object.oid.hash, &type, &size);\n \t\tconst char *subject_start;\n \t\tint subject_len;\n \n@@ -144,10 +144,10 @@ static void show_list(const char *debug, int counted, int nr,\n \t\t\tfprintf(stderr, \"%3d\", weight(p));\n \t\telse\n \t\t\tfprintf(stderr, \"---\");\n-\t\tfprintf(stderr, \" %.*s\", 8, sha1_to_hex(commit->object.sha1));\n+\t\tfprintf(stderr, \" %.*s\", 8, sha1_to_hex(commit->object.oid.hash));\n \t\tfor (pp = commit->parents; pp; pp = pp->next)\n \t\t\tfprintf(stderr, \" %.*s\", 8,\n-\t\t\t\tsha1_to_hex(pp->item->object.sha1));\n+\t\t\t\tsha1_to_hex(pp->item->object.oid.hash));\n \n \t\tsubject_len = find_commit_subject(buf, &subject_start);\n \t\tif (subject_len)\n-- \n2.15.1\n\n\n\nFrom 2ec8823de3bd0ca8253ddbd07f58b66afcfabe96 Mon Sep 17 00:00:00 2001\nFrom: Yasushi SHOJI <yashi@atmark-techno.com>\nDate: Mon, 8 Jan 2018 22:35:12 +0900\nSubject: [PATCH 2/2] bisect: handle empty list in best_bisection_sorted()\n\nIn 7c117184d7 (\"bisect: fix off-by-one error in\n`best_bisection_sorted()`\", 2017-11-05) the more careful logic dealing\nwith freeing p->next in 50e62a8e70 (\"rev-list: implement\n--bisect-all\", 2007-10-22) was removed.\n\nThis function, and also all other functions below find_bisection() to\nbe complete, will be called with an empty commit list if all commits\nare _uninteresting_.  So the functions must be prepared for it.\n\nThis commit, instead of restoring the check, moves the clean-up code\ninto the loop.  To do that this commit changes the non-last-case\nbranch in the loop to a last-case branch, and clean-up unused trailing\nelements there.\n\nWe could, on the other hand, do a big \"if list\" condition at the very\nbeginning.  But, that doesn't sound right for the current code base.\nNo other functions does that but all blocks in the functions are\ntolerant to an empty list.\n\nBy the way, as you can tell from the non-last-case branch we had in\nthe loop, this fix shouldn't cause a pipeline stall on every iteration\non modern processors with a branch predictor.\n\nSigned-off-by: Yasushi ShOJI <yasushi.shoji@gmail.com>\n---\n bisect.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex fb3e44903..cec4a537f 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -218,7 +218,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n \t\tcnt++;\n \t}\n \tQSORT(array, cnt, compare_commit_dist);\n-\tfor (p = list, i = 0; i < cnt; i++) {\n+\tfor (p = list, i = 0; i < cnt; i++, p = p->next) {\n \t\tstruct object *obj = &(array[i].commit->object);\n \n \t\tstrbuf_reset(&buf);\n@@ -226,11 +226,13 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n \t\tadd_name_decoration(DECORATION_NONE, buf.buf, obj);\n \n \t\tp->item = array[i].commit;\n-\t\tif (i < cnt - 1)\n-\t\t\tp = p->next;\n+\n+\t\tif (i == cnt) {\n+\t\t\t/* clean-up unused elements if any */\n+\t\t\tfree_commit_list(p->next);\n+\t\t\tp->next = NULL;\n+\t\t}\n \t}\n-\tfree_commit_list(p->next);\n-\tp->next = NULL;\n \tstrbuf_release(&buf);\n \tfree(array);\n \treturn list;\n-- \n2.15.1\n\n"},{"id":"336148","messageId":"CAP8UFD1rO1cFCfZg53gG71qfbapkn3gVvja87YFiMBZTkOqNuw@mail.gmail.com","threadId":"47528","inReplyTo":"CAELBRWLV7wQm_b9Lvj67KiAMAxq6vNOzA3z62S-f5=3A5LyjZQ@mail.gmail.com","subject":"Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-01-08T15:09:53Z","receivedAt":"2018-01-08T15:10:00Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Mon, Jan 8, 2018 at 3:45 PM, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:\n> Hi all,\n>\n> Thank you guys for insightful help.  I just read the code and now I understand\n> what you guys are saying.  Yeah, I can say the fix is \"spot on\".\n>\n> But, to be honest, it's hard to see why you need \"if (p)\" at  the first glance.\n> So, how about this fix, instead?\n\n+    for (p = list, i = 0; i < cnt; i++, p = p->next) {\n\nHere \"i\" can reach \"cnt - 1\" at most, so ...\n\n+        if (i == cnt) {\n+            /* clean-up unused elements if any */\n+            free_commit_list(p->next);\n+            p->next = NULL;\n+        }\n\n... \"i == cnt\" is always false above. I think it should be \"i == cnt - 1\".\n\nAnd with your code one can wonder why the cleanup is part of the loop.\n\n> I also found a bug in show_list().  I'm attaching a patch as well.\n\nCould you send it inline as explained in Documentation/SubmittingPatches?\n\nThanks,\nChristian.\n"},{"id":"336226","messageId":"CAELBRW+kvvUVKt2L9xRDtFvD2Vi9qG=3ZnNR2OVmdq6F64RMBg@mail.gmail.com","threadId":"47528","inReplyTo":"CAP8UFD1rO1cFCfZg53gG71qfbapkn3gVvja87YFiMBZTkOqNuw@mail.gmail.com","subject":"Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip","fromName":"Yasushi SHOJI","fromEmail":"yasushi.shoji@gmail.com","sentAt":"2018-01-09T11:09:05Z","receivedAt":"2018-01-09T11:09:12Z","isPatch":false,"sender":{"key":"yasushi.shoji@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10152?v=4"},"body":"Hi,\n\nOn Tue, Jan 9, 2018 at 12:09 AM, Christian Couder\n<christian.couder@gmail.com> wrote:\n> ... \"i == cnt\" is always false above. I think it should be \"i == cnt - 1\".\n\nuga.  Thanks.  That's stupid of me.\n\n> And with your code one can wonder why the cleanup is part of the loop.\n\nOK.  I have nothing to offer, then.  Thank you for taking care of this!\n\n>> I also found a bug in show_list().  I'm attaching a patch as well.\n>\n> Could you send it inline as explained in Documentation/SubmittingPatches?\n\nDone that.\n\nThanks,\n-- \n          yashi\n"}]}