From a82402f1f553fb02d0661708cbeab9288ffef970 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 1 Aug 2023 06:14:27 -0700 Subject: [PATCH 1/2] update_view_title: Fix NULL dereference at startup update_view_title can be called with a `struct view` where `line` is NULL, and `lines` is 0. Specifically, along this call stack: 0 update_view_title (view=0x3fdb88 ) at [...]/tig/src/view.c:690 1 0x0000000000338018 in report_clear () at [...]/tig/src/display.c:565 2 0x00000000003cfe5b in load_view (view=0x3fdb88 , prev=0x3fdb88 , flags=OPEN_ at [...]/tig/src/view.c:857 3 0x00000000003d0bc0 in open_view (prev=0x0, view=0x3fdb88 , flags=OPEN_DEFAULT) at [...]/tig/src/view.c:894 4 0x00000000003b2932 in open_main_view (prev=0x0, flags=OPEN_DEFAULT) at include/tig/main.h:57 5 0x00000000003b0cca in view_driver (view=0x0, request=REQ_VIEW_MAIN) at [...]/tig/src/tig.c:179 6 0x00000000003af96a in main (argc=1, argv=0x7fffffffddb8) at [...]/tig/src/tig.c:864 Specifically, load_view calls report_clear when `view->lines == 0`, which calls `update_view_title`, which attempts `&view->line[...]` on a null `line`. It's not clear why this doesn't explode today. I caught it when I ran tig compiled with Zig in debug mode and it failed with an illegal instruction on the line: struct line *line = &view->line[view->pos.lineno]; Adding a check for `NULL` or `lines == 0` resolves the issue. --- src/view.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/view.c b/src/view.c index e64ef19f9..2891e01b7 100644 --- a/src/view.c +++ b/src/view.c @@ -687,6 +687,8 @@ void update_view_title(struct view *view) { WINDOW *window = view->title; + if (view->line == NULL) + return; struct line *line = &view->line[view->pos.lineno]; unsigned int view_lines, lines; int update_increment = view_has_flags(view, VIEW_LOG_LIKE | VIEW_GREP_LIKE) From 3b018b59b84d312da11b042aa1477fe3cd4704e7 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Fri, 4 Aug 2023 05:42:19 -0700 Subject: [PATCH 2/2] Don't return early for non-line cases The prior fix was pretty blunt: return early if line is null. However, line isn't really used unless specific conditions are met. Instead of returning early, add a check for the value of line where it's first used. This also allows reducing the scope of the corresponding variables from function to a specific block. --- src/view.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/view.c b/src/view.c index 2891e01b7..bf86fe3ac 100644 --- a/src/view.c +++ b/src/view.c @@ -687,9 +687,6 @@ void update_view_title(struct view *view) { WINDOW *window = view->title; - if (view->line == NULL) - return; - struct line *line = &view->line[view->pos.lineno]; unsigned int view_lines, lines; int update_increment = view_has_flags(view, VIEW_LOG_LIKE | VIEW_GREP_LIKE) ? 100 @@ -709,16 +706,18 @@ update_view_title(struct view *view) wprintw(window, " %s", view->ref); } - if (!view_has_flags(view, VIEW_CUSTOM_STATUS) && view_has_line(view, line) && - line->lineno) { - wprintw(window, " - %s %u of %zu", - view->ops->type, - line->lineno, - MAX(line->lineno, - view->pipe - ? update_increment * - (size_t) ((view->lines - view->custom_lines) / update_increment) - : view->lines - view->custom_lines)); + if (!view_has_flags(view, VIEW_CUSTOM_STATUS) && view->line != NULL) { + struct line *line = &view->line[view->pos.lineno]; + if (view_has_line(view, line) && line->lineno) { + wprintw(window, " - %s %u of %zu", + view->ops->type, + line->lineno, + MAX(line->lineno, + view->pipe + ? update_increment * + (size_t) ((view->lines - view->custom_lines) / update_increment) + : view->lines - view->custom_lines)); + } } if (view->pipe) {