Mark Jamsek <mark@jamsek.com> wrote:
> 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:

Let's try that again with the right fix this time :)


commit 9b234aafe57408c4499430093c09448abfc118fb
from: Mark Jamsek <mark@jamsek.dev>
date: Tue Dec 31 13:10:33 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 + 9b234aafe57408c4499430093c09448abfc118fb
blob - 0fcbbd78a8afe17bd2f12a187cecec268453d4ea
blob + 381950ee222f7b284d4a41b67b5b08dde376678a
--- 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_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