On 3 January 2018 at 15:21, Ævar Arnfjörð Bjarmason <[email protected]> wrote: > > On Wed, Jan 03 2018, Yasushi SHOJI jotted: > >> Hi, >> >> git version 2.16.0.rc0 seg faults on my machine when I >> [...] >> Program terminated with signal SIGSEGV, Segmentation fault. >> #0 0x000055a73107f900 in best_bisection_sorted (list=0x0, nr=0) at >> bisect.c:232 >> 232 free_commit_list(p->next); >> (gdb) bt >> #0 0x000055a73107f900 in best_bisection_sorted (list=0x0, nr=0) at >> bisect.c:232 >> #1 0x000055a73107fc0f in do_find_bisection (list=0x0, nr=0, >> weights=0x55a731b6ffd0, find_all=1) at bisect.c:361 >> #2 0x000055a73107fcf4 in find_bisection (commit_list=0x7ffe8750d4d0, >> reaches=0x7ffe8750d4c4, all=0x7ffe8750d4c0, find_all=1) at >> bisect.c:400 >> #3 0x000055a73108128d in bisect_next_all (prefix=0x0, no_checkout=0) >> at bisect.c:969 >> #4 0x000055a730fd5238 in cmd_bisect__helper (argc=0, >> argv=0x7ffe8750e230, prefix=0x0) at builtin/bisect--helper.c:140 >> #5 0x000055a730fcbc76 in run_builtin (p=0x55a73145c778 >> <commands+120>, argc=2, argv=0x7ffe8750e230) at git.c:346 >> #6 0x000055a730fcbf40 in handle_builtin (argc=2, argv=0x7ffe8750e230) >> at git.c:554 >> #7 0x000055a730fcc0e8 in run_argv (argcp=0x7ffe8750e0ec, >> argv=0x7ffe8750e0e0) at git.c:606 >> #8 0x000055a730fcc29b in cmd_main (argc=2, argv=0x7ffe8750e230) at git.c:683 >> #9 0x000055a731068d9e in main (argc=3, argv=0x7ffe8750e228) at >> common-main.c:43 >> (gdb) p p >> $1 = (struct commit_list *) 0x0 >> >> As you can see, the code dereferences to the 'next' while 'p' is NULL. >> >> I'm sure I did 'git bisect good' after git _found_ bad commit. Then I >> typed 'git bisect skip' on the commit 726804874 of guile repository. >> If that matters at all. >> >> I haven't touched guile repo to preserve the current state. > > I can't reproduce this myself, but looking at the backtrace it seems > pretty obvious that 7c117184d7 ("bisect: fix off-by-one error in > `best_bisection_sorted()`", 2017-11-05) is the culprit. > > That changed more careful code added by Christian in 50e62a8e70 > ("rev-list: implement --bisect-all", 2007-10-22) to free a pointer which > as you can see can be NULL. > > If you can test a patch to see if it works this should fix it: > > diff --git a/bisect.c b/bisect.c > index 0fca17c02b..2f3008b078 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -229,8 +229,10 @@ static struct commit_list *best_bisection_sorted(struct > commit_list *list, int n > if (i < cnt - 1) > p = p->next; > } > - free_commit_list(p->next); > - p->next = NULL; > + if (p) { > + free_commit_list(p->next); > + p->next = NULL; > + } > strbuf_release(&buf); > free(array); > return list; > > But given the commit message by Martin maybe there's some deeper bug here.
I haven't tried to reproduce, or tested the patch, but from the looks of it, your analysis and fix are both spot on. The special case that yashi has hit is that `list` is NULL. The old code handled that very well, the code after my patch ... not so well. The loop-sort-loop pattern reduces to a no-op, both before and after my patch. But what I failed to realize was that `list` could be NULL. This could be fixed by an early return if `list` is NULL, but that would also need some memory-handling. So I think your patch is just as good or better, since it can be seen as restoring what was lost in 7c117184d7. Thanks both, and sorry for this. Martin
