From: Mark Jamsek Subject: Re: tog: don't mark incorrect base commit in nested log views To: Mark Jamsek Cc: gameoftrees@openbsd.org Date: Wed, 01 Jan 2025 00:08:49 +1100 Mark Jamsek wrote: > > The second and third diffs contain two small albeit related fixes for > bugs discovered during the process of writing the test: the first fixes > a NULL pointer deref from passing a NULL s->last_displayed_entry to > got_tree_entry_get_next() when scrolling down in a somewhat contrived > tree view that is less than 4 lines in height; and the other diff clamps > an s->selected assignment to 0 rather than becoming negative in the > similarly contrived case of views less than their header and border > lines in height (i.e., [2,5] depending on the view), which can result in > bogus indexes displayed in the tree view header. Both diffs include a > test covering these bugs: without the fixes, the added log test fails > with a [-1/4] index displayed in the nested tree view header; and the > tree test segfaults when scrolling down in the 3-line high tree view. > > > ----------------------------------------------- > commit df7caa1a7ae745e41ecee2ae1d3420a536796715 > from: Mark Jamsek > date: Tue Dec 31 12:45:28 2024 UTC > > fix NULL deref when scrolling small tog tree views down > > In the rare case a tree view is smaller than four lines in height, > the last_displayed_entry is NULL. Check this condition on a scroll > down event with j/down arrow or ^f/pgdn to guard a NULL got_tree_entry > from being passed to got_tree_entry_get_next() where it is dereferenced. > And add a test case covering this path. > fix a pasto in this diff: revised diff below: commit 4a0f8f1e9d408259f98b361b8175dc72cf8cbc82 from: Mark Jamsek date: Tue Dec 31 13:05:40 2024 UTC fix NULL deref when scrolling small tog tree views down In the rare case a tree view is smaller than four lines in height, the last_displayed_entry is NULL. Check this condition on a scroll down event with j/down arrow or ^f/pgdn to guard a NULL got_tree_entry from being passed to got_tree_entry_get_next() where it is dereferenced. And add a test case covering this path. M regress/tog/tree.sh | 33+ 0- M tog/tog.c | 4+ 2- 2 files changed, 37 insertions(+), 2 deletions(-) commit - ece1c9be5bbce981c493f47522d70ed1a1355a03 commit + 4a0f8f1e9d408259f98b361b8175dc72cf8cbc82 blob - 0fcbbd78a8afe17bd2f12a187cecec268453d4ea blob + c681e5048e5ea60b6a261db1f49d94d45298c5a3 --- regress/tog/tree.sh +++ regress/tog/tree.sh @@ -341,9 +341,42 @@ test_tree_commit_keywords() test_done "$testroot" "$ret" } +test_tree_insufficient_height() +{ + test_init tree_tree_insufficient_height 120 3 + + local id=$(git_show_head $testroot/repo) + + # Cover the path that guards a NULL dereference when scrolling + # down in a tree view too small to display any tree entries. + cat <<-EOF >$TOG_TEST_SCRIPT + j attempt to scroll down + f + SCREENDUMP + EOF + + cat <<-EOF >$testroot/view.expected + commit $id + [1/4] / + + EOF + + cd $testroot/repo && tog tree + cmp -s $testroot/view.expected $testroot/view + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/view.expected $testroot/view + test_done "$testroot" "$ret" + return 1 + fi + + test_done "$testroot" "$ret" +} + test_parseargs "$@" run_test test_tree_basic run_test test_tree_vsplit_blame run_test test_tree_hsplit_blame run_test test_tree_symlink run_test test_tree_commit_keywords +run_test test_tree_insufficient_height blob - e31dd5f281a3090496215529ba601df0b1b29849 blob + e8031edb015e8f848ba06221e89e864108afdd00 --- tog/tog.c +++ tog/tog.c @@ -9671,7 +9671,8 @@ input_tree_view(struct tog_view **new_view, struct tog s->selected++; break; } - if (got_tree_entry_get_next(s->tree, s->last_displayed_entry) + if (s->last_displayed_entry == NULL || + got_tree_entry_get_next(s->tree, s->last_displayed_entry) == NULL) { /* can't scroll any further */ view->count = 0; @@ -9687,7 +9688,8 @@ input_tree_view(struct tog_view **new_view, struct tog case CTRL('f'): case 'f': case ' ': - if (got_tree_entry_get_next(s->tree, s->last_displayed_entry) + if (s->last_displayed_entry == NULL || + got_tree_entry_get_next(s->tree, s->last_displayed_entry) == NULL) { /* can't scroll any further; move cursor down */ if (s->selected < s->ndisplayed - 1) -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68