Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup

From

Jeff King <peff@peff.net>

Date

Jul 23, 2025, 08:06 UTC

Message-ID

<20250723080639.GA570920@coredump.intra.peff.net>

In-Reply-To

<20250723075513.GA570540@coredump.intra.peff.net>

On Wed, Jul 23, 2025 at 03:55:13AM -0400, Jeff King wrote:

> I dunno. We are reaching diminishing returns spending brainpower on a
> function that is meant to be somewhat quick-and-dirty.

Like:

 t/helper/test-delta.c | 49 ++++++++++++-----------------------
 1 file changed, 16 insertions(+), 33 deletions(-)

diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 6bc787a474..769b68839d 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -21,39 +21,28 @@ int cmd__delta(int argc, const char **argv)
 	struct stat st;
 	void *from_buf = NULL, *data_buf = NULL, *out_buf = NULL;
 	unsigned long from_size, data_size, out_size;
-	int ret = 1;
 
 	if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p"))) {
 		fprintf(stderr, "usage: %s\n", usage_str);
 		return 1;
 	}
 
-	fd = open(argv[2], O_RDONLY);
-	if (fd < 0 || fstat(fd, &st)) {
-		perror(argv[2]);
-		return 1;
-	}
+	fd = xopen(argv[2], O_RDONLY);
+	if (fstat(fd, &st) < 0)
+		die_errno("fstat(%s)", argv[2]);
 	from_size = st.st_size;
 	from_buf = xmalloc(from_size);
-	if (read_in_full(fd, from_buf, from_size) < 0) {
-		perror(argv[2]);
-		close(fd);
-		goto cleanup;
-	}
+	if (read_in_full(fd, from_buf, from_size) < 0)
+		die_errno("read(%s)", argv[2]);
 	close(fd);
 
-	fd = open(argv[3], O_RDONLY);
-	if (fd < 0 || fstat(fd, &st)) {
-		perror(argv[3]);
-		goto cleanup;
-	}
+	fd = xopen(argv[3], O_RDONLY);
+	if (fstat(fd, &st) < 0)
+		die_errno("fstat(%s)", argv[3]);
 	data_size = st.st_size;
 	data_buf = xmalloc(data_size);
-	if (read_in_full(fd, data_buf, data_size) < 0) {
-		perror(argv[3]);
-		close(fd);
-		goto cleanup;
-	}
+	if (read_in_full(fd, data_buf, data_size) < 0)
+		die_errno("read(%s)", argv[3]);
 	close(fd);
 
 	if (argv[1][1] == 'd')
@@ -64,22 +53,16 @@ int cmd__delta(int argc, const char **argv)
 		out_buf = patch_delta(from_buf, from_size,
 				      data_buf, data_size,
 				      &out_size);
-	if (!out_buf) {
-		fprintf(stderr, "delta operation failed (returned NULL)\n");
-		goto cleanup;
-	}
+	if (!out_buf)
+		die("delta operation failed (returned NULL)");
 
-	fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
-	if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {
-		perror(argv[4]);
-		goto cleanup;
-	}
+	fd = xopen(argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
+	if (write_in_full(fd, out_buf, out_size) < 0)
+		die_errno("write(%s)", argv[4]);
 
-	ret = 0;
-cleanup:
 	free(from_buf);
 	free(data_buf);
 	free(out_buf);
 
-	return ret;
+	return 0;
 }


I'd guess that one could probably go even further by replacing the bare
pointers with strbufs. And then you could use strbuf_read_file().

Incidentally that would also fix two minor bugs I noticed:

  - passing st.st_size directly to xmalloc() is wrong, because of
    truncation from off_t to size_t. This should use the xsize_t helper.
    This is even a potential security vulnerability, but probably not
    important in a test helper.

  - likewise read_in_full() might return a non-negative value smaller
    than the requested size (if the file racily changes and we get an
    early EOF). But we only check whether we got a negative error value.
    So we may read fewer bytes than expected and feed uninitialized
    garbage to the delta code.

-Peff