BUG: Sigsegv while holding the key 'Stage/Unstage current line'

[Mephistophiles]

For reproduce:

• tig latest version
• same git repo, for my case it tig repo:

WITH DIFF

diff --git a/NEWS.adoc b/NEWS.adoc
index 36d967c..a6d582a 100644
--- a/NEWS.adoc
+++ b/NEWS.adoc
@@ -1177,3 +1177,16 @@ Improvements:
  - Tree view: use a stack for remembering the lines for parent tree.
 
 /* vim: set tw=80: */
+
+
+
+
+
+
+
+
+
+
+
+
+

• open tig
• hold '1' key (Stage current line)
• SIGSEGV

To debug the crash, I used the address sanitizer:

diff --git a/Makefile b/Makefile
index f17419d..3963eda 100644
--- a/Makefile
+++ b/Makefile
@@ -19,7 +19,7 @@ TIG_NCURSES ?= -lcurses
 LDFLAGS ?= $(TIG_LDFLAGS)
 CPPFLAGS ?= $(TIG_CPPFLAGS)
 LDLIBS ?= $(TIG_NCURSES) $(TIG_LDLIBS)
-CFLAGS ?= -Wall -O2 $(TIG_CFLAGS)
+CFLAGS ?= -fsanitize=address -fsanitize=undefined -g -Wall -O2 $(TIG_CFLAGS)
 
 prefix ?= $(HOME)
 bindir ?= $(prefix)/bin

It show full trace:

cat /tmp/ubsan.log.tig.71603 
=================================================================
==tig==71603==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000020 (pc 0x557740f79356 bp 0x000000000000 sp 0x7fffd3ac3f20 T0)
==tig==71603==The signal is caused by a READ memory access.
==tig==71603==Hint: address points to the zero page.
    #0 0x557740f79355 in htab_delete compat/hashtab.c:417
    #1 0x557740f67d7b in done_graph src/graph-v2.c:260
    #2 0x557740f53a85 in main_done src/main.c:334
    #3 0x557740f113cb in load_view src/view.c:782
    #4 0x557740f5a248 in main_status_exists src/main.c:40
    #5 0x557740f52d41 in stage_exists src/stage.c:349
    #6 0x557740f52d41 in stage_request src/stage.c:462
    #7 0x557740ea9727 in view_request src/tig.c:67
    #8 0x557740ea9727 in view_driver src/tig.c:166
    #9 0x557740ea9727 in main src/tig.c:862
    #10 0x7eff770f7152 in __libc_start_main (/usr/lib/libc.so.6+0x27152)
    #11 0x557740ead51d in _start (/tmp/tig/src/tig+0x15051d)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV compat/hashtab.c:417 in htab_delete
==tig==71603==ABORTING

[Mephistophiles]

I bisected this bug:

9ca77109dfdd3969420ac3420ea24a00abdd6773 is the first bad commit
commit 9ca77109dfdd3969420ac3420ea24a00abdd6773
Author: stevenyvr987 <[email protected]>
Date:   Sat Jun 1 13:18:49 2019 -0700

    Fix memory leak in main view (#931)
    
    When the main_done() is called, done_graph() is called, which calls free(graph), where graph is a pointer to a struct graph.
    
    struct graph points to a hash table colors.id_map, which should be freed before the call to free(graph).

 src/graph-v2.c | 2 ++
 1 file changed, 2 insertions(+)

$ git bisect log
git bisect start
# bad: [89c8fa23e112bdfc5aee7e44e9e541b5c9e80e67] tig-2.5.0
git bisect bad 89c8fa23e112bdfc5aee7e44e9e541b5c9e80e67
# good: [fc1bb3a535331fe752316f8f30bd4fd3c642c555] tig-2.4.1
git bisect good fc1bb3a535331fe752316f8f30bd4fd3c642c555
# good: [fe636aba650e709f65886c5a3cd92ae58046509d] Revert "No graph when pathspec is specified"
git bisect good fe636aba650e709f65886c5a3cd92ae58046509d
# good: [fe636aba650e709f65886c5a3cd92ae58046509d] Revert "No graph when pathspec is specified"
git bisect good fe636aba650e709f65886c5a3cd92ae58046509d
# bad: [798604bf6a92a7bbbd6c4a4cd5c370ba95ceb3c3] Merge pull request #938 from koutcher/gh-755-start-on-head
git bisect bad 798604bf6a92a7bbbd6c4a4cd5c370ba95ceb3c3
# bad: [3458d5ba040a810ea0674305d4be2341600ad86b] Exit gracefully if refs view was defined without ref column
git bisect bad 3458d5ba040a810ea0674305d4be2341600ad86b
# good: [909b4e5a2ad96f2137888cc30e5264e321be5937] Combined diff uses @@@ as hunk marker
git bisect good 909b4e5a2ad96f2137888cc30e5264e321be5937
# good: [bff4a43fb7734b843b1c5ce11be74aceca9c2bb4] Show annotated commits in main view (#928)
git bisect good bff4a43fb7734b843b1c5ce11be74aceca9c2bb4
# bad: [9ca77109dfdd3969420ac3420ea24a00abdd6773] Fix memory leak in main view (#931)
git bisect bad 9ca77109dfdd3969420ac3420ea24a00abdd6773
# good: [3b5bded69944c61711122d8ad8332ceb09d53459] Fix memory leak induced by 'tig grep' (#930)
git bisect good 3b5bded69944c61711122d8ad8332ceb09d53459
# first bad commit: [9ca77109dfdd3969420ac3420ea24a00abdd6773] Fix memory leak in main view (#931)

[koutcher]

Yes, there should be a check on graph->colors.id_map before doing the htab_delete. However you shouldn't be able to reach this point without having id_map. Do you use any unusual setting ? Can you detail step by step how to reproduce ?

[Mephistophiles]

I used the default configuration.

For reproduce:

• compile tig with this patch

diff --git a/Makefile b/Makefile
index f17419d..3963eda 100644
--- a/Makefile
+++ b/Makefile
@@ -19,7 +19,7 @@ TIG_NCURSES ?= -lcurses
 LDFLAGS ?= $(TIG_LDFLAGS)
 CPPFLAGS ?= $(TIG_CPPFLAGS)
 LDLIBS ?= $(TIG_NCURSES) $(TIG_LDLIBS)
-CFLAGS ?= -Wall -O2 $(TIG_CFLAGS)
+CFLAGS ?= -fsanitize=address -fsanitize=undefined -g -Wall -O2 $(TIG_CFLAGS)
 
 prefix ?= $(HOME)
 bindir ?= $(prefix)/bin

• In the tig repo run:

for i in $(seq 1 100); do echo "" >> NEWS.adoc; done

• In tig select NEWS.adoc and hold '1' key for stage lines
• for me tig crashed after stage 71 lines

[koutcher]

No luck. How does it behave with the following patch ?

diff --git a/src/graph-v2.c b/src/graph-v2.c
index 46504a8..69ac158 100644
--- a/src/graph-v2.c
+++ b/src/graph-v2.c
@@ -257,7 +257,8 @@ done_graph(struct graph *graph_ref)
 {
        struct graph_v2 *graph = graph_ref->private;

-       htab_delete(graph->colors.id_map);
+       if (graph->colors.id_map)
+               htab_delete(graph->colors.id_map);

        free(graph);

[Mephistophiles]

Yep, this patch works.

[Mephistophiles]

Thanks!