Re: [PATCH v2 3/4] t/helper/test-delta: close fd if fstat() fails after open()

From: Jeff King <peff@peff.net>
Date: Jul 22, 2025, 08:40 UTC
Message-ID: <20250722084059.GA863857@coredump.intra.peff.net>
In-Reply-To: <20250722081219.1086866-4-lhywkd22@gmail.com>

On Tue, Jul 22, 2025 at 08:12:18AM +0000, Hoyoung Lee wrote:

Show 19 quoted lines

> If open() succeeds but fstat() fails, the file descriptor is not
> closed, causing a resource leak. This patch adds a close(fd) call
> in the failure path after fstat() to ensure proper resource cleanup.
> 
> Signed-off-by: Hoyoung Lee <lhywkd22@gmail.com>
> ---
>  t/helper/test-delta.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
> index 6bc787a474..103bf7f3e9 100644
> --- a/t/helper/test-delta.c
> +++ b/t/helper/test-delta.c
> @@ -31,6 +31,7 @@ int cmd__delta(int argc, const char **argv)
>  	fd = open(argv[2], O_RDONLY);
>  	if (fd < 0 || fstat(fd, &st)) {
>  		perror(argv[2]);
> +		close(fd);
>  		return 1;

This will result in close(-1) if the open() call failed (rather than the fstat() call). Not the end of the world, but you do the correct check in the same function for patch 4.

Since there are two spots here (and it looks like a third for argv[4] later in the function!), perhaps it would make more sense to attach the close to the cleanup label, like this:

diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 6bc787a474..d291d6f02d 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -31,13 +31,12 @@ int cmd__delta(int argc, const char **argv)
 	fd = open(argv[2], O_RDONLY);
 	if (fd < 0 || fstat(fd, &st)) {
 		perror(argv[2]);
-		return 1;
+		goto cleanup;
 	}
 	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;
 	}
 	close(fd);
@@ -51,7 +50,6 @@ int cmd__delta(int argc, const char **argv)
 	data_buf = xmalloc(data_size);
 	if (read_in_full(fd, data_buf, data_size) < 0) {
 		perror(argv[3]);
-		close(fd);
 		goto cleanup;
 	}
 	close(fd);
@@ -81,5 +79,8 @@ int cmd__delta(int argc, const char **argv)
 	free(data_buf);
 	free(out_buf);
 
+	if (fd >= 0)
+		close(fd);
+
 	return ret;
 }

It is a little hard to see from just the diff that this always does the
right thing, since we reuse "fd" over and over. But it is OK because we
always close() it right before calling open() again (and never jump to
cleanup in between).

Probably the whole thing would be more readable with three separate
descriptors initialized to -1, but I'm not sure how much effort it is
worth putting into polishing this test helper.

-Peff