From: Mark Jamsek <mark@jamsek.com>
Subject: tog: lock mutex on script error + blame view deallocation tweak
To: Game of Trees <gameoftrees@openbsd.org>
Date: Sat, 15 Apr 2023 23:02:42 +1000

Below are two diffs. Both of which apply on top of the pending blame
regress diff, but they _might_ apply without it.

First is short and simple: while working on something else, I noticed
that we fail to relock the mutex if we return from view_loop() early
due to a tog_read_script_key() error.

diff /home/mark/src/got
commit - 6419890e5180822b712d47c23cc4d30e63966085
path + /home/mark/src/got
blob - 232ab4c8aa553e2c4cf0a880fe27470a40b63e0f
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -1707,8 +1707,10 @@ view_input(struct tog_view **new, int *done, struct to
 
 	if (using_mock_io) {
 		err = tog_read_script_key(tog_io.f, &ch, done);
-		if (err)
+		if (err) {
+			errcode = pthread_mutex_lock(&tog_mutex);
 			return err;
+		}
 	} else if (view->count && --view->count) {
 		cbreak();
 		nodelay(view->window, TRUE);


Next, while more closely studying the recent view leak fix in 2ca2f982e,
I realised we need to treat the blame view case a little differently.
Specifically, we only want to free the tog_view structure if
open_blame_view() fails--not on any error case in cmd_blame() because if
we make it to a view_loop() call in cmd_blame(), view gets freed at the
end of view_loop() irrespective of error.  And apparently simply setting
view to NULL inside view_close() wasn't enough for the (view != NULL)
guard. So I've moved view_open() to immediately before the
open_blame_view() call, and moved the view->close/view_close() calls to
immediately after it so they are only called when open_blame_view()
returns an error.

The above is not a problem in the other views, because we are calling
view_close/view->close from the open_*_view() routines--not cmd_*();
that is, view_loop() is never entered in those cases.

I've attached a diff(1) patch of this because 'got diff' doesn't show
this change as cleanly.

--- tog.c	Sat Apr 15 22:29:33 2023
+++ tog/tog.c	Sat Apr 15 22:25:51 2023
@@ -1707,8 +1707,10 @@ view_input(struct tog_view **new, int *done, struct to
 
 	if (using_mock_io) {
 		err = tog_read_script_key(tog_io.f, &ch, done);
-		if (err)
+		if (err) {
+			errcode = pthread_mutex_lock(&tog_mutex);
 			return err;
+		}
 	} else if (view->count && --view->count) {
 		cbreak();
 		nodelay(view->window, TRUE);
@@ -6997,12 +6999,6 @@ cmd_blame(int argc, char *argv[])
 	if (error != NULL)
 		goto done;
 
-	view = view_open(0, 0, 0, 0, TOG_VIEW_BLAME);
-	if (view == NULL) {
-		error = got_error_from_errno("view_open");
-		goto done;
-	}
-
 	error = got_object_open_as_commit(&commit, repo, commit_id);
 	if (error)
 		goto done;
@@ -7012,10 +7008,19 @@ cmd_blame(int argc, char *argv[])
 	if (error)
 		goto done;
 
+	view = view_open(0, 0, 0, 0, TOG_VIEW_BLAME);
+	if (view == NULL) {
+		error = got_error_from_errno("view_open");
+		goto done;
+	}
 	error = open_blame_view(view, link_target ? link_target : in_repo_path,
 	    commit_id, repo);
-	if (error)
+	if (error != NULL) {
+		if (view->close == NULL)
+			close_blame_view(view);
+		view_close(view);
 		goto done;
+	}
 	if (worktree) {
 		/* Release work tree lock. */
 		got_worktree_close(worktree);
@@ -7028,11 +7033,6 @@ done:
 	free(link_target);
 	free(cwd);
 	free(commit_id);
-	if (error != NULL && view != NULL) {
-		if (view->close == NULL)
-			close_blame_view(view);
-		view_close(view);
-	}
 	if (commit)
 		got_object_commit_close(commit);
 	if (worktree)

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