Skip to content

update_view_title: Fix NULL dereference at startup#1293

Merged
koutcher merged 2 commits intojonas:masterfrom
abhinav:update-null-deref
Oct 21, 2023
Merged

update_view_title: Fix NULL dereference at startup#1293
koutcher merged 2 commits intojonas:masterfrom
abhinav:update-null-deref

Conversation

@abhinav
Copy link
Copy Markdown
Contributor

@abhinav abhinav commented Aug 2, 2023

update_view_title can be called with a struct view
where line is NULL, and lines is 0.

Along this call stack:

0  update_view_title (view=0x3fdb88 <main_view>) at [...]/tig/src/view.c:690
1  0x0000000000338018 in report_clear () at [...]/tig/src/display.c:565
2  0x00000000003cfe5b in load_view (view=0x3fdb88 <main_view>, prev=0x3fdb88 <main_view>, flags=OPEN_
   at [...]/tig/src/view.c:857
3  0x00000000003d0bc0 in open_view (prev=0x0, view=0x3fdb88 <main_view>, 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.

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 <main_view>) at [...]/tig/src/view.c:690
    1  0x0000000000338018 in report_clear () at [...]/tig/src/display.c:565
    2  0x00000000003cfe5b in load_view (view=0x3fdb88 <main_view>, prev=0x3fdb88 <main_view>, flags=OPEN_
       at [...]/tig/src/view.c:857
    3  0x00000000003d0bc0 in open_view (prev=0x0, view=0x3fdb88 <main_view>, 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.
@abhinav abhinav force-pushed the update-null-deref branch from b3a2bc8 to a82402f Compare August 2, 2023 02:40
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.
@koutcher
Copy link
Copy Markdown
Collaborator

Thanks

@koutcher koutcher merged commit 2634298 into jonas:master Oct 21, 2023
@abhinav abhinav deleted the update-null-deref branch February 8, 2024 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants