Omar Polo <op@omarpolo.com> wrote:
> 
> by the way, create_diff() has a similar problem with f not being closed
> if the second got_opentemp() or fclose() fail.  A similar treatment
> could be applied to it too.
> 

Thanks for letting me know about this one!

We could fix it a few ways, and I'm partial to either of the below so
whichever you prefer is ok with me :)

As alluded to in your previous email, we can just cleanup f on error in
create_diff() if we haven't already assigned f to s->f as in the topmost
diff.

Alternatively, because s->f is always cleaned up in close_diff_view() on
error or otherwise, we can just reorder things to close s->f before
opening f and then assign f to s->f before calling got_opentemp() the
second time, thereby leaving cleanup to close_diff_view() as in the
second below diff (which also changes a few goto done to return err as
there's no cleanup needed in such cases).


diff 85467924748b0e27f0105bde280878a149df9fc8 7b1dd741d3dd0d749fa33227f3961eba31281935
commit - 85467924748b0e27f0105bde280878a149df9fc8
commit + 7b1dd741d3dd0d749fa33227f3961eba31281935
blob - be366266e526e26c8734719d20d29ed05d463b06
blob + 1c0e6a67d711243f78a28fa4aba05dd2eccf0d98
--- tog/tog.c
+++ tog/tog.c
@@ -5422,6 +5422,8 @@ done:
 	free(lines);
 	if (commit2 != NULL)
 		got_object_commit_close(commit2);
+	if (err != NULL && f != NULL && f != s->f)
+		fclose(f);
 	got_pathlist_free(&changed_paths, GOT_PATHLIST_FREE_ALL);
 	if (s->f && fflush(s->f) != 0 && err == NULL)
 		err = got_error_from_errno("fflush");


--

diff /home/mark/src/got
commit - 85467924748b0e27f0105bde280878a149df9fc8
path + /home/mark/src/got
blob - be366266e526e26c8734719d20d29ed05d463b06
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -5318,21 +5318,17 @@ create_diff(struct tog_diff_view_state *s)
 		return got_error_from_errno("malloc");
 	s->nlines = 0;
 
+	if (s->f && fclose(s->f) == EOF)
+		return got_error_from_errno("fclose");
+
 	f = got_opentemp();
-	if (f == NULL) {
-		err = got_error_from_errno("got_opentemp");
-		goto done;
-	}
+	if (f == NULL)
+		return got_error_from_errno("got_opentemp");
+	s->f = f;
+
 	tmp_diff_file = got_opentemp();
-	if (tmp_diff_file == NULL) {
-		err = got_error_from_errno("got_opentemp");
-		goto done;
-	}
-	if (s->f && fclose(s->f) == EOF) {
-		err = got_error_from_errno("fclose");
-		goto done;
-	}
-	s->f = f;
+	if (tmp_diff_file == NULL)
+		return got_error_from_errno("got_opentemp");
 
 	if (s->id1)
 		err = got_object_get_type(&obj_type, s->repo, s->id1);


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