From: Mark Jamsek <mark@jamsek.com>
Subject: plug tog_view leaks on open_*_view() errors
To: Game of Trees <gameoftrees@openbsd.org>
Date: Sat, 15 Apr 2023 00:50:49 +1000

For each of the log, diff, blame, ref, and tree views, when their
respective open_*_view() call errors, we call the corresponding
close_*_view(), which releases view state. However, this does not
deallocate the tog_view structure itself so it is leaked.

The below diff replaces the close_*_view() calls with view_close(),
which also calls close_*_view() if the view->close function pointer has
been initialised. If view->close has not been initialised, we also call
close_*_view() too. This ensures both the view->state.* and tog_view
structures are freed.

It might be easier understood by looking at the cmd_log() flow of
events before this diff (which is the same for diff, tree, and ref;
blame is slightly different but view is nonetheless leaked):

  view_open()
    -> allocates view
  open_log_view()
    -> fails
      -> close_log_view()
        -> free view->state.log members
      -> goto done
  view leaked

The diff also includes a little whitespace cleanup which just happened
to be where some related changes were made otherwise I'd split it out.

diff /home/mark/src/got
commit - 7483826edb3332b2f49e9dfa9515a2a097f61d60
path + /home/mark/src/got
blob - 25ca0cca73b31182890d474c3baa4f54dce0e53e
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -3588,8 +3588,10 @@ open_log_view(struct tog_view *view, struct got_object
 	if (in_repo_path != s->in_repo_path) {
 		free(s->in_repo_path);
 		s->in_repo_path = strdup(in_repo_path);
-		if (s->in_repo_path == NULL)
-			return got_error_from_errno("strdup");
+		if (s->in_repo_path == NULL) {
+			err = got_error_from_errno("strdup");
+			goto done;
+		}
 	}
 
 	/* The commit queue only contains commits being displayed. */
@@ -3691,8 +3693,11 @@ done:
 	s->thread_args.limit_regex = &s->limit_regex;
 	s->thread_args.limit_commits = &s->limit_commits;
 done:
-	if (err)
-		close_log_view(view);
+	if (err) {
+		if (view->close == NULL)
+			close_log_view(view);
+		view_close(view);
+	}
 	return err;
 }
 
@@ -5322,16 +5327,19 @@ open_diff_view(struct tog_view *view, struct got_objec
 	s->fd2 = -1;
 
 	if (id1 != NULL && id2 != NULL) {
-	    int type1, type2;
-	    err = got_object_get_type(&type1, repo, id1);
-	    if (err)
-		return err;
-	    err = got_object_get_type(&type2, repo, id2);
-	    if (err)
-		return err;
+		int type1, type2;
 
-	    if (type1 != type2)
-		return got_error(GOT_ERR_OBJ_TYPE);
+		err = got_object_get_type(&type1, repo, id1);
+		if (err)
+			goto done;
+		err = got_object_get_type(&type2, repo, id2);
+		if (err)
+			goto done;
+
+		if (type1 != type2) {
+			err = got_error(GOT_ERR_OBJ_TYPE);
+			goto done;
+		}
 	}
 	s->first_displayed_line = 1;
 	s->last_displayed_line = view->nlines;
@@ -5344,8 +5352,10 @@ open_diff_view(struct tog_view *view, struct got_objec
 
 	if (id1) {
 		s->id1 = got_object_id_dup(id1);
-		if (s->id1 == NULL)
-			return got_error_from_errno("got_object_id_dup");
+		if (s->id1 == NULL) {
+			err = got_error_from_errno("got_object_id_dup");
+			goto done;
+		}
 	} else
 		s->id1 = NULL;
 
@@ -5435,8 +5445,11 @@ done:
 	view->search_setup = search_setup_diff_view;
 	view->search_next = search_next_view_match;
 done:
-	if (err)
-		close_diff_view(view);
+	if (err) {
+		if (view->close == NULL)
+			close_diff_view(view);
+		view_close(view);
+	}
 	return err;
 }
 
@@ -6887,7 +6900,7 @@ cmd_blame(int argc, char *argv[])
 	struct got_commit_object *commit = NULL;
 	char *commit_id_str = NULL;
 	int ch;
-	struct tog_view *view;
+	struct tog_view *view = NULL;
 	int *pack_fds = NULL;
 
 	while ((ch = getopt(argc, argv, "c:r:")) != -1) {
@@ -7000,6 +7013,11 @@ 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)
@@ -7369,8 +7387,10 @@ open_tree_view(struct tog_view *view, struct got_objec
 	STAILQ_INIT(&s->colors);
 
 	s->commit_id = got_object_id_dup(commit_id);
-	if (s->commit_id == NULL)
-		return got_error_from_errno("got_object_id_dup");
+	if (s->commit_id == NULL) {
+		err = got_error_from_errno("got_object_id_dup");
+		goto done;
+	}
 
 	err = got_object_open_as_commit(&commit, repo, commit_id);
 	if (err)
@@ -7444,8 +7464,11 @@ done:
 	free(commit_id_str);
 	if (commit)
 		got_object_commit_close(commit);
-	if (err)
-		close_tree_view(view);
+	if (err) {
+		if (view->close == NULL)
+			close_tree_view(view);
+		view_close(view);
+	}
 	return err;
 }
 
@@ -8047,7 +8070,7 @@ open_ref_view(struct tog_view *view, struct got_reposi
 
 	err = ref_view_load_refs(s);
 	if (err)
-		return err;
+		goto done;
 
 	if (has_colors() && getenv("TOG_COLORS") != NULL) {
 		err = add_color(&s->colors, "^refs/heads/",
@@ -8081,8 +8104,11 @@ done:
 	view->search_start = search_start_ref_view;
 	view->search_next = search_next_ref_view;
 done:
-	if (err)
-		free_colors(&s->colors);
+	if (err) {
+		if (view->close == NULL)
+			close_ref_view(view);
+		view_close(view);
+	}
 	return err;
 }
 

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