Stefan Sperling <stsp@stsp.name> wrote:
> On Mon, Dec 30, 2024 at 03:50:37AM +1100, Mark Jamsek wrote:
> > And here's the actual updated diff; sorry for the noise!
> 
> ok by me

Thanks, stsp! I've realised a problem with the diff in the error case:
it leaks tree_view's member objects because free(tree_view) is wrong,
we need to call view_close(tree_view).

Also, open_tree_view() already calls view_close() (and close_tree_view()
if tree_view's close() function pointer has not already been assigned)
in all of its error paths, which simplifies things a fair bit and makes
for a much nicer fix.

I'll write a regression test for the T keymap on work tree entries later
tonight too.


commit 199165079af358a1775965af2d4477f968fcce1c (main)
from: Mark Jamsek <mark@jamsek.dev>
date: Mon Dec 30 04:31:18 2024 UTC

fix NULL deref with T keymap on log view worktree entry

When the log view T keymap is used, the selected_entry is passed to
browse_commit_tree() to open a tree view of the repository as at
the selected commit, which passes the selected_entry->id member to
open_tree_view() to open the commit. The id member, which is a NULL
pointer in work tree entries, is dereferenced so we segfault.
If the T keymap is used on a work tree entry, use the base commit id
instead, which we must have if a work tree entry is selected.

M  tog/tog.c  |  28+  2-

1 file changed, 28 insertions(+), 2 deletions(-)

commit - 8e5958b86dfbd58e1637258b72b19c17ce5c8503
commit + 199165079af358a1775965af2d4477f968fcce1c
blob - 368c057ac8f74f6cc9f839c25c94eaa812c22b53
blob + 212306ce6089ace5c4afbb0b88dd625fe9156e9d
--- tog/tog.c
+++ tog/tog.c
@@ -3841,12 +3841,23 @@ browse_commit_tree(struct tog_view **new_view, int beg
 	const struct got_error *err = NULL;
 	struct tog_tree_view_state *s;
 	struct tog_view *tree_view;
+	struct got_commit_object *commit = NULL;
+	struct got_object_id *commit_id;
 
+	*new_view = NULL;
+
+	if (entry->id != NULL)
+		commit_id = entry->id;
+	else if (entry->worktree_entry)
+		commit_id = tog_base_commit.id;
+	else /* cannot happen */
+		return got_error(GOT_ERR_NOT_WORKTREE);
+
 	tree_view = view_open(0, 0, begin_y, begin_x, TOG_VIEW_TREE);
 	if (tree_view == NULL)
 		return got_error_from_errno("view_open");
 
-	err = open_tree_view(tree_view, entry->id, head_ref_name, repo);
+	err = open_tree_view(tree_view, commit_id, head_ref_name, repo);
 	if (err)
 		return err;
 	s = &tree_view->state.tree;
@@ -3856,7 +3867,22 @@ browse_commit_tree(struct tog_view **new_view, int beg
 	if (got_path_is_root_dir(path))
 		return NULL;
 
-	return tree_view_walk_path(s, entry->commit, path);
+	if (entry->worktree_entry) {
+		err = got_object_open_as_commit(&commit, repo, commit_id);
+		if (err != NULL)
+			goto done;
+	}
+
+	err = tree_view_walk_path(s, commit ? commit : entry->commit, path);
+
+done:
+	if (commit != NULL)
+		got_object_commit_close(commit);
+	if (err != NULL) {
+		view_close(tree_view);
+		*new_view = NULL;
+	}
+	return err;
 }
 
 /*


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68