Mark Jamsek <mark@jamsek.com> 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 <mark@jamsek.dev>
> 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 <mark@jamsek.dev>
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 <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68