From: Mark Jamsek <mark@jamsek.com>
Subject: got: plug got_diffreg_result leak
To: Game of Trees <gameoftrees@openbsd.org>
Date: Sat, 7 Jan 2023 22:52:09 +1100

We leak the diff result when we fail to call got_diffreg_result_free()
in two of the three callers of got_diffreg_output() in lib/diff.c.

If got_diffreg_output() fails, we goto done, which jumps straight over
got_diffreg_result_free().

I opted for the fix of least churn, which simply raises the 'done:'
label, making it the same as the third got_diffreg_output() caller.

We could instead remove all goto in these routines because disposing of
got_diffreg_result, which is allocated at the end, is the only cleanup
we do. So all prior goto's could just return instead. OTOH, the chosen
approach is more future-proof incase we end up adding further resources
that need deallocating, and, as mentioned, a lot less churn.

That said, I'm happy to go with either fix.

-----------------------------------------------
commit ddc1bfe037dd99b2df4bcb955f8383d2c0f2a770 (main)
from: Mark Jamsek <mark@jamsek.dev>
date: Sat Jan  7 11:47:26 2023 UTC
 
 plug leak of got_diffreg_result on output failure
 
 Move 'done' labels up so that if got_diffreg_output() fails we still call
 got_diffreg_result_free() instead of jumping straight over it.
 
diff 5191b70b5b2e123aadd88aeafe2e2cfc0958c327 ddc1bfe037dd99b2df4bcb955f8383d2c0f2a770
commit - 5191b70b5b2e123aadd88aeafe2e2cfc0958c327
commit + ddc1bfe037dd99b2df4bcb955f8383d2c0f2a770
blob - 2007e46b23e16bc18f223174ce22d2e157bad96e
blob + 52bcc33dbb28753dca85d62d8e81f6da86803cbd
--- lib/diff.c
+++ lib/diff.c
@@ -185,14 +185,15 @@ diff_blobs(struct got_diff_line **lines, size_t *nline
 			goto done;
 	}
 
+done:
 	if (resultp && err == NULL)
 		*resultp = result;
-	else {
+	else if (result) {
 		free_err = got_diffreg_result_free(result);
 		if (free_err && err == NULL)
 			err = free_err;
 	}
-done:
+
 	return err;
 }
 
@@ -276,6 +277,7 @@ diff_blob_file(struct got_diffreg_result **resultp,
 			goto done;
 	}
 
+done:
 	if (resultp && err == NULL)
 		*resultp = result;
 	else if (result) {
@@ -283,7 +285,6 @@ done:
 		if (free_err && err == NULL)
 			err = free_err;
 	}
-done:
 	return err;
 }
 

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