[PATCH v3 0/2] fix resource leaks in test helpers

[PATCH v3 0/2] fix resource leaks in test helpers

[Hoyoung Lee]

Thanks for the detailed and thoughtful reviews. Your comments helped me better understand how resource cleanup should be handled, especially when dealing with early program termination.

I especially appreciate your in-depth explanation regarding the difference between true resource leaks and stack-unwinding cases. The example you gave was very insightful, and it clarified how automated tools and reviewers should assess whether a resource is truly leaked. Thank you for taking the time to provide such a clear and instructive explanation.

Hoyoung Lee (2):
  t/helper/test-truncate: close file descriptor after truncation
  t/helper/test-delta: fix possible resource leak and ensure safe
    cleanup

 t/helper/test-delta.c    | 9 +++++----
 t/helper/test-truncate.c | 3 +++
 2 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] t/helper/test-truncate: close file descriptor after truncation

[Hoyoung Lee]

Fix a resource leak where the file descriptor was not closed after
truncating a file in t/helper/test-truncate.c.

Signed-off-by: Hoyoung Lee <lhywkd22@gmail.com>
---
 t/helper/test-truncate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/helper/test-truncate.c b/t/helper/test-truncate.c
index 3931deaec7..104bc36cc0 100644
--- a/t/helper/test-truncate.c
+++ b/t/helper/test-truncate.c
@@ -21,5 +21,8 @@ int cmd__truncate(int argc, const char **argv)
 
 	if (ftruncate(fd, (off_t) sz) < 0)
 		die_errno("failed to truncate file");
+	
+	close(fd);
+
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v3 1/2] t/helper/test-truncate: close file descriptor after truncation

[Junio C Hamano]

Hoyoung Lee <lhywkd22@gmail.com> writes:

> Fix a resource leak where the file descriptor was not closed after
> truncating a file in t/helper/test-truncate.c.
>
> Signed-off-by: Hoyoung Lee <lhywkd22@gmail.com>
> ---
>  t/helper/test-truncate.c | 3 +++
>  1 file changed, 3 insertions(+)

While it is not wrong per-se, it is not like a function that can
potentially be called number of times returns without closing it.
Nobody other than the main() function would be calling this
function, so the only thing that is done after this function returns
to its caller is for the caller to return leading to exit from the
process at which point the leftover file descriptor would be
closed.

So I am a bit curious what triggered you to send in these changes.
Are there some automated resource leak checker, without fixes like
these whose output would be noisy with complaints about these "known
leaking, no ill effects in practice, yet gets flagged by checkers"
code to be useful, or something?

Will queue; thanks.

> diff --git a/t/helper/test-truncate.c b/t/helper/test-truncate.c
> index 3931deaec7..104bc36cc0 100644
> --- a/t/helper/test-truncate.c
> +++ b/t/helper/test-truncate.c
> @@ -21,5 +21,8 @@ int cmd__truncate(int argc, const char **argv)
>  
>  	if (ftruncate(fd, (off_t) sz) < 0)
>  		die_errno("failed to truncate file");
> +	
> +	close(fd);
> +
>  	return 0;
>  }

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

[Hoyoung Lee]

Initialize `fd` to -1 and unify all `open()`-related `close()` calls
under a single cleanup label. This prevents undefined behavior when
`fd` is used without initialization in error paths.

The cleanup logic now safely avoids calling `close()` on invalid
descriptors and ensures consistent resource management.

Signed-off-by: Hoyoung Lee <lhywkd22@gmail.com>
---
 t/helper/test-delta.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 6bc787a474..f5811e96ad 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -17,7 +17,7 @@ static const char usage_str[] =
 
 int cmd__delta(int argc, const char **argv)
 {
-	int fd;
+	int fd = -1;
 	struct stat st;
 	void *from_buf = NULL, *data_buf = NULL, *out_buf = NULL;
 	unsigned long from_size, data_size, out_size;
@@ -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;
 }
-- 
2.34.1

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

[Eric Sunshine]

On Tue, Jul 22, 2025 at 1:41 PM Hoyoung Lee <lhywkd22@gmail.com> wrote:
> Initialize `fd` to -1 and unify all `open()`-related `close()` calls
> under a single cleanup label. This prevents undefined behavior when
> `fd` is used without initialization in error paths.

It's not clear what this means. As far as I can tell, the original
code never used an uninitialized `fd` in error paths.

> The cleanup logic now safely avoids calling `close()` on invalid
> descriptors and ensures consistent resource management.

Again, it's not clear what this means. Although your previous version
of this patch did add a call to close() with an invalid descriptor,
the original code did not do so. So, the above statement seems to be
misleading.

Those issues aside, the patch itself has problems, some minor, such as
making the code a bit confusing or misleading, and some major, such as
calling close() on an already closed descriptor.

Unfortunately, there are enough important context lines missing from
the patch itself that, instead of directly reviewing the patch
directly, I'm going to review the code following the application of
your patch...

>   int fd = -1;

This new initialization (-1) is useless because...

>   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);

...the very first time `fd` is mentioned (aside from the declaration)
is here where it is unconditionally assigned a value. Thus, the -1
initialization is wasted (and potentially confusing for readers).

>   if (fd < 0 || fstat(fd, &st)) {
>     perror(argv[2]);
>     goto cleanup;
>   }

Okay, no problem here. The `if (fd >= 0) close(fd)` you added to the
"cleanup" action handles both the cases here when `fd` might be
negative after the open() call or a valid descriptor.

>   from_size = st.st_size;
>   from_buf = xmalloc(from_size);
>   if (read_in_full(fd, from_buf, from_size) < 0) {
>     perror(argv[2]);
>     goto cleanup;
>   }
>   close(fd);

Here `fd` is closed manually which is good because...

>   fd = open(argv[3], O_RDONLY);

...this code immediately assigns it a new value.

>   if (fd < 0 || fstat(fd, &st)) {
>     perror(argv[3]);
>     goto cleanup;
>   }

Okay for the same reason mentioned above.

>   data_size = st.st_size;
>   data_buf = xmalloc(data_size);
>   if (read_in_full(fd, data_buf, data_size) < 0) {
>     perror(argv[3]);
>     goto cleanup;
>   }
>   close(fd);

The descriptor is closed manually (again) because a subsequent open()
call is going to reuse the variable. However...

>   if (argv[1][1] == 'd')
>     out_buf = diff_delta(from_buf, from_size,
>              data_buf, data_size,
>              &out_size, 0);
>   else
>     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;
>   }

...although `fd` was closed, it still holds the previously-open
non-negative file descriptor, which means that this `goto cleanup`...

>   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;
>   }
>
>   ret = 0;
> cleanup:
>   free(from_buf);
>   free(data_buf);
>   free(out_buf);
>
>   if (fd >= 0)
>     close(fd);

...will arrive here and the condition will evaluate to "true",
resulting in the already-closed descriptor being closed again.

>   return ret;

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

[Jeff King]

On Wed, Jul 23, 2025 at 03:28:05AM -0400, Eric Sunshine wrote:

> >   close(fd);
> 
> The descriptor is closed manually (again) because a subsequent open()
> call is going to reuse the variable. However...
> 
> >   if (argv[1][1] == 'd')
> >     out_buf = diff_delta(from_buf, from_size,
> >              data_buf, data_size,
> >              &out_size, 0);
> >   else
> >     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;
> >   }
> 
> ...although `fd` was closed, it still holds the previously-open
> non-negative file descriptor, which means that this `goto cleanup`...

Oof, good catch. This iteration of the patch was based on my suggestion,
but I didn't notice the jump to cleanup between that close/open pair.

I wonder if it would be more clear written as:

  int from_fd = -1;
  int data_fd = -1;
  int out_fd = -1;

  ...
  from_fd = open(...);
  if (...errors...)
	goto cleanup;
  close(from_fd);
  from_fd = -1;

  [...same for other fds...]

cleanup:

  if (from_fd >= 0)
	close(from_fd);
  [etc]

You could even drop that first close() in the happy path entirely, if
you don't mind holding all three descriptors open at once.

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

-Peff

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

[Jeff King]

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.

OK, I clearly could not resist spending more brainpower on it. If we are
doing quick-and-dirty, why not just die()? The end result is the same,
but per my argument in the earlier iteration of the series, that means
we do not have to worry about cleaning up at all.

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

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

[Eric Sunshine]

On Wed, Jul 23, 2025 at 4:06 AM Jeff King <peff@peff.net> wrote:
> 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.
>
> OK, I clearly could not resist spending more brainpower on it. If we are
> doing quick-and-dirty, why not just die()? The end result is the same,
> but per my argument in the earlier iteration of the series, that means
> we do not have to worry about cleaning up at all.

Yes, die() seems sensible here. It's nice and tidy and makes the code
easier to reason about.

> 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.

Can of worms opened.

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

[Jeff King]

On Wed, Jul 23, 2025 at 04:06:39AM -0400, Jeff King wrote:

> 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.
> 
> OK, I clearly could not resist spending more brainpower on it. If we are
> doing quick-and-dirty, why not just die()? The end result is the same,
> but per my argument in the earlier iteration of the series, that means
> we do not have to worry about cleaning up at all.

So...I feel a little bad about hijacking Hoyoung's thread. But after all
of the discussion, it seemed simplest to just stick it all into patches.
So here is a potential replacement for patch 2.

  [1/3]: test-delta: handle errors with die()
  [2/3]: test-delta: use strbufs to hold input files
  [3/3]: test-delta: close output descriptor after use

 t/helper/test-delta.c | 77 ++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 53 deletions(-)

-Peff

[PATCH 1/3] test-delta: handle errors with die()

[Jeff King]

This is a short test helper that does all of its work in the main
function. When we encounter an error, we try to clean up memory and
descriptors and then jump to an error return, which exits the program.

We can get the same effect by just calling die(), which means we do not
have to bother with cleaning up. This simplifies the code, and also
removes some inconsistencies where a few code paths forgot to clean up
descriptors (though in practice it was not a big deal since we were
exiting anyway).

In addition to die() and die_errno(), we'll also use a few of our usual
helpers like xopen() and usage() that make things more ergonomic.

This does change the exit code in these cases from 1 to 128, but I
don't think it matters (and arguably is better, as we'd already exit 128
for other errors like xmalloc() failure).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-delta.c | 55 ++++++++++++++-----------------------------
 1 file changed, 18 insertions(+), 37 deletions(-)

diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 6bc787a474..4495b32b49 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -21,39 +21,26 @@ 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;
-	}
+	if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p")))
+		usage(usage_str);
 
-	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 +51,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;
 }
-- 
2.50.1.666.gdb1e186d6a

[PATCH 2/3] test-delta: use strbufs to hold input files

[Jeff King]

We want to read the whole contents of two files into memory. If we
switch from raw ptr/len pairs to strbufs, we can use strbuf_read_file()
to shorten the code.

This incidentally fixes two small bugs:

  1. We stat() the files and allocate our buffers based on st.st_size.
     But that is an off_t which may be larger than the size_t we'd use
     to allocate. We should use xsize_t() to do a checked conversion.
     Otherwise integer truncation (on a file >4GB) could cause us to
     under-allocate (though in practice this does not result in a buffer
     overflow because the same truncation happens when read_in_full()
     also takes a size_t).

  2. We get the size from st.st_size, and then try to read_in_full()
     that many bytes. But it may return fewer bytes than expected (if
     the file changed racily and we get an early EOF), leading us to
     read uninitialized bytes in the allocated buffer. We don't notice
     because we only check the value for error, not that we got the
     expected number of bytes.

The strbuf code doesn't run into this, because it just reads to EOF,
expanding the buffer dynamically as necessary. Neither bug is a big deal
for a test helper, but fixing them is a nice bonus on top of simplifying
the code.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-delta.c | 40 ++++++++++++++--------------------------
 1 file changed, 14 insertions(+), 26 deletions(-)

diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 4495b32b49..7945793078 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -11,45 +11,33 @@
 #include "test-tool.h"
 #include "git-compat-util.h"
 #include "delta.h"
+#include "strbuf.h"
 
 static const char usage_str[] =
 	"test-tool delta (-d|-p) <from_file> <data_file> <out_file>";
 
 int cmd__delta(int argc, const char **argv)
 {
 	int fd;
-	struct stat st;
-	void *from_buf = NULL, *data_buf = NULL, *out_buf = NULL;
-	unsigned long from_size, data_size, out_size;
+	struct strbuf from = STRBUF_INIT, data = STRBUF_INIT;
+	char *out_buf;
+	unsigned long out_size;
 
 	if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p")))
 		usage(usage_str);
 
-	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)
-		die_errno("read(%s)", argv[2]);
-	close(fd);
-
-	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)
-		die_errno("read(%s)", argv[3]);
-	close(fd);
+	if (strbuf_read_file(&from, argv[2], 0) < 0)
+		die_errno("unable to read '%s'", argv[2]);
+	if (strbuf_read_file(&data, argv[3], 0) < 0)
+		die_errno("unable to read '%s'", argv[3]);
 
 	if (argv[1][1] == 'd')
-		out_buf = diff_delta(from_buf, from_size,
-				     data_buf, data_size,
+		out_buf = diff_delta(from.buf, from.len,
+				     data.buf, data.len,
 				     &out_size, 0);
 	else
-		out_buf = patch_delta(from_buf, from_size,
-				      data_buf, data_size,
+		out_buf = patch_delta(from.buf, from.len,
+				      data.buf, data.len,
 				      &out_size);
 	if (!out_buf)
 		die("delta operation failed (returned NULL)");
@@ -58,8 +46,8 @@ int cmd__delta(int argc, const char **argv)
 	if (write_in_full(fd, out_buf, out_size) < 0)
 		die_errno("write(%s)", argv[4]);
 
-	free(from_buf);
-	free(data_buf);
+	strbuf_release(&from);
+	strbuf_release(&data);
 	free(out_buf);
 
 	return 0;
-- 
2.50.1.666.gdb1e186d6a

[PATCH 3/3] test-delta: close output descriptor after use

[Jeff King]

After we write to the output file, the program exits. This naturally
closes the descriptor. But we should do an explicit close for two
reasons:

  1. It's possible to hit an error on close(), which we should detect
     and report via our exit code.

  2. Leaking descriptors is a bad practice in general. Even if it isn't
     meaningful here, it sets a bad example.

It is tempting to write:

  if (write_in_full(fd, ...) < 0 || close(fd) < 0)
        die_errno(...);

But that pattern contains a subtle problem that has resulted in
descriptor leaks before. If write_in_full() fails, we'll short-circuit
and never call close(), leaking the descriptor.

That's not a problem here, since our error path dies instead of
returning up the stack. But since we're trying to set a good example,
let's write it out as two separate conditions. As a bonus, that lets us
produce a slightly more specific error message.

Signed-off-by: Jeff King <peff@peff.net>
---
Another option is that the program should just write to stdout, then we
do not have to open or close it at all. ;)

 t/helper/test-delta.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 7945793078..52ea00c937 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -45,6 +45,8 @@ int cmd__delta(int argc, const char **argv)
 	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]);
+	if (close(fd) < 0)
+		die_errno("close(%s)", argv[4]);
 
 	strbuf_release(&from);
 	strbuf_release(&data);
-- 
2.50.1.666.gdb1e186d6a

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

[Eric Sunshine]

On Wed, Jul 23, 2025 at 3:55 AM Jeff King <peff@peff.net> wrote:
> On Wed, Jul 23, 2025 at 03:28:05AM -0400, Eric Sunshine wrote:
> > The descriptor is closed manually (again) because a subsequent open()
> > call is going to reuse the variable. However...
> > ...although `fd` was closed, it still holds the previously-open
> > non-negative file descriptor, which means that this `goto cleanup`...
>
> Oof, good catch. This iteration of the patch was based on my suggestion,
> but I didn't notice the jump to cleanup between that close/open pair.
>
> I dunno. We are reaching diminishing returns spending brainpower on a
> function that is meant to be somewhat quick-and-dirty.

Aside from preferring that the patch not make the code worse or more
confusing, I don't have a strong opinion. Superficially, the existing
code presents itself as being careful by cleaning up after itself, but
as Hoyoung Lee discovered, there are holes in the cleanup
implementation. So, I do like that the intention of the patch is to
plug those holes, but we don't necessarily need to be particularly
clever about it. For such simple test-related code, even a pure
brute-force fix should be acceptable.

For completeness, I'll mention that I even had the thought that
another "fix" would be to tear out all the cleanup code entirely since
we _know_ that this function will be exiting immediately and the OS
will clean up any dangling resources.

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

[Jeff King]

On Wed, Jul 23, 2025 at 04:11:26AM -0400, Eric Sunshine wrote:

> For completeness, I'll mention that I even had the thought that
> another "fix" would be to tear out all the cleanup code entirely since
> we _know_ that this function will be exiting immediately and the OS
> will clean up any dangling resources.

The reason we have the "cleanup" label at all is because of the memory
leaks. And there the issue is that we build the test helpers with the
same compiler settings as the rest of the code, so SANITIZE=leak will
complain. So I think that is a non-starter.

But if you just meant leaking descriptors, sure, I don't think any tools
complain about that. ;)

-Peff

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

[Eric Sunshine]

On Wed, Jul 23, 2025 at 4:46 AM Jeff King <peff@peff.net> wrote:
> On Wed, Jul 23, 2025 at 04:11:26AM -0400, Eric Sunshine wrote:
> > For completeness, I'll mention that I even had the thought that
> > another "fix" would be to tear out all the cleanup code entirely since
> > we _know_ that this function will be exiting immediately and the OS
> > will clean up any dangling resources.
>
> The reason we have the "cleanup" label at all is because of the memory
> leaks. And there the issue is that we build the test helpers with the
> same compiler settings as the rest of the code, so SANITIZE=leak will
> complain. So I think that is a non-starter.
>
> But if you just meant leaking descriptors, sure, I don't think any tools
> complain about that. ;)

At the time the momentary (but not really serious) thought flashed
through my brain, I probably was imagining dropping all of the cleanup
code; I almost certainly wasn't thinking about the SANITIZE=leak case.
But immediately upon seeing your idea to use die(), I recognized it
(and liked it) as a superior version of my half-baked thought.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> For completeness, I'll mention that I even had the thought that
> another "fix" would be to tear out all the cleanup code entirely since
> we _know_ that this function will be exiting immediately and the OS
> will clean up any dangling resources.

;-)

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

True.  After all, the function is ready to exit and have the system
close the file descriptor anyway ;-)

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Jul 22, 2025 at 1:41 PM Hoyoung Lee <lhywkd22@gmail.com> wrote:
>> Initialize `fd` to -1 and unify all `open()`-related `close()` calls
>> under a single cleanup label. This prevents undefined behavior when
>> `fd` is used without initialization in error paths.
>
> It's not clear what this means. As far as I can tell, the original
> code never used an uninitialized `fd` in error paths.
>
>> The cleanup logic now safely avoids calling `close()` on invalid
>> descriptors and ensures consistent resource management.
>
> Again, it's not clear what this means. Although your previous version
> of this patch did add a call to close() with an invalid descriptor,
> the original code did not do so. So, the above statement seems to be
> misleading.

Great to see that good guidance is offered to new folks.  Yes, it is
a great thing to keep in mind to describe the latest iteration for
readers who haven't even seen the previous iterations.

> Those issues aside, the patch itself has problems, some minor, such as
> making the code a bit confusing or misleading, and some major, such as
> calling close() on an already closed descriptor.

>>   data_size = st.st_size;
>>   data_buf = xmalloc(data_size);
>>   if (read_in_full(fd, data_buf, data_size) < 0) {
>>     perror(argv[3]);
>>     goto cleanup;
>>   }
>>   close(fd);
>
> The descriptor is closed manually (again) because a subsequent open()
> call is going to reuse the variable. However...
>
>>   if (argv[1][1] == 'd')
>>     out_buf = diff_delta(from_buf, from_size,
>>              data_buf, data_size,
>>              &out_size, 0);
>>   else
>>     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;
>>   }
>
> ...although `fd` was closed, it still holds the previously-open
> non-negative file descriptor, which means that this `goto cleanup`...

Good eyes.

>>   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;
>>   }
>>
>>   ret = 0;
>> cleanup:
>>   free(from_buf);
>>   free(data_buf);
>>   free(out_buf);
>>
>>   if (fd >= 0)
>>     close(fd);
>
> ...will arrive here and the condition will evaluate to "true",
> resulting in the already-closed descriptor being closed again.

Very clear description.

Thanks for your review.