[PATCH 0/4] Fix resource leaks in various helpers and builtin commands

[PATCH 0/4] Fix resource leaks in various helpers and builtin commands

[Hoyoung Lee]

This patch series fixes several cases where file descriptors were not
properly closed on error paths. The changes affect helper programs and
a builtin command, and ensure that system resources are correctly
released before returning from the function.

Each fix is minimal and follows the existing style of the surrounding
code. These changes help improve the robustness of the code by avoiding
potential file descriptor leaks.

Hoyoung Lee (4):
  t/helper/test-truncate: close file descriptor after truncation
  builtin/archive: close file descriptor on dup2() failure
  t/helper/test-delta: close fd if fstat() fails after open()
  t/helper/test-delta: close fd if fstat() fails after second open()

 builtin/archive.c        | 4 +++-
 t/helper/test-delta.c    | 3 +++
 t/helper/test-truncate.c | 3 +++
 3 files changed, 9 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH v2 1/4] 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

[PATCH v2 2/4] builtin/archive: close file descriptor on dup2() failure

[Hoyoung Lee]

In create_output_file(), the file descriptor returned by xopen()
was not closed if dup2() failed. This leads to a potential resource
leak. Ensure the file descriptor is closed regardless of whether
dup2() succeeds or fails.

Signed-off-by: Hoyoung Lee <lhywkd22@gmail.com>
---
 builtin/archive.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/archive.c b/builtin/archive.c
index 13ea7308c8..c919a39f90 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -14,6 +14,7 @@ static void create_output_file(const char *output_file)
 	int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
 	if (output_fd != 1) {
 		if (dup2(output_fd, 1) < 0)
+			close(output_fd);
 			die_errno(_("could not redirect output"));
 		else
 			close(output_fd);
-- 
2.34.1

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

[Hoyoung Lee]

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;
 	}
 	from_size = st.st_size;
-- 
2.34.1

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

[Hoyoung Lee]

When opening argv[3], if open() succeeds but fstat() fails,
the file descriptor is not closed, resulting in a resource leak.
This patch ensures that the descriptor is closed on failure.

Signed-off-by: Hoyoung Lee <lhywkd22@gmail.com>
---
 builtin/archive.c     | 3 ++-
 t/helper/test-delta.c | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index c919a39f90..951fc2e444 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -13,9 +13,10 @@ static void create_output_file(const char *output_file)
 {
 	int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
 	if (output_fd != 1) {
-		if (dup2(output_fd, 1) < 0)
+		if (dup2(output_fd, 1) < 0) {
 			close(output_fd);
 			die_errno(_("could not redirect output"));
+		}
 		else
 			close(output_fd);
 	}
diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 103bf7f3e9..ba2d897aa3 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -46,6 +46,8 @@ int cmd__delta(int argc, const char **argv)
 	fd = open(argv[3], O_RDONLY);
 	if (fd < 0 || fstat(fd, &st)) {
 		perror(argv[3]);
+		if (fd >= 0)
+			close(fd);
 		goto cleanup;
 	}
 	data_size = st.st_size;
-- 
2.34.1

Re: [PATCH v2 2/4] builtin/archive: close file descriptor on dup2() failure

[Jeff King]

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

> In create_output_file(), the file descriptor returned by xopen()
> was not closed if dup2() failed. This leads to a potential resource
> leak. Ensure the file descriptor is closed regardless of whether
> dup2() succeeds or fails.

The other patches in this series made sense to me, but I don't think
this one does. It is not really a "leak" in the sense that output_fd is
still on the stack when we exit (via the die_errno() call).

That may sound pedantic, but we have run into the same question with
memory leaks. If we are not unwinding the stack, then we can never fully
clean up the program state. E.g., with:

  void foo(void)
  {
	int fd = open(...);
	if (bar() < 0)
		die(...);
  }

it is tempting to think that you should close(fd) to clean up your
state. But now imagine that bar(), instead of returning an error, itself
exits the program, like this:

  void bar(void)
  {
	if (baz() < 0)
		die(...);
  }

It _can't_ close "fd" because it doesn't even know about it!

So when we consider whether something is leaking, I think it only makes
sense in terms of unwinding the stack. And likewise any automated tools
we use should consider that.

> diff --git a/builtin/archive.c b/builtin/archive.c
> index 13ea7308c8..c919a39f90 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -14,6 +14,7 @@ static void create_output_file(const char *output_file)
>  	int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
>  	if (output_fd != 1) {
>  		if (dup2(output_fd, 1) < 0)
> +			close(output_fd);
>  			die_errno(_("could not redirect output"));
>  		else
>  			close(output_fd);

So I don't think this patch makes sense, but also...does this even
compile? You did not add braces around the conditional block, so we'll
call close() on failure but we won't call die_errno() anymore. And the
else is no longer coupled to the if (because of the unrelated die
statement in between), which should cause the compiler to barf.

-Peff

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

[Jeff King]

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

> When opening argv[3], if open() succeeds but fstat() fails,
> the file descriptor is not closed, resulting in a resource leak.
> This patch ensures that the descriptor is closed on failure.
> 
> Signed-off-by: Hoyoung Lee <lhywkd22@gmail.com>
> ---
>  builtin/archive.c     | 3 ++-
>  t/helper/test-delta.c | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/archive.c b/builtin/archive.c
> index c919a39f90..951fc2e444 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -13,9 +13,10 @@ static void create_output_file(const char *output_file)
>  {
>  	int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
>  	if (output_fd != 1) {
> -		if (dup2(output_fd, 1) < 0)
> +		if (dup2(output_fd, 1) < 0) {
>  			close(output_fd);
>  			die_errno(_("could not redirect output"));
> +		}
>  		else
>  			close(output_fd);
>  	}

Ah, I guess you found the problem from patch 3. But it should have been
squashed in there, not to this patch. (But as I said there, I think we
should just drop patch 3 entirely).

-Peff

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

[Jeff King]

On Tue, Jul 22, 2025 at 04:25:57AM -0400, Jeff King wrote:

> >  	int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
> >  	if (output_fd != 1) {
> > -		if (dup2(output_fd, 1) < 0)
> > +		if (dup2(output_fd, 1) < 0) {
> >  			close(output_fd);
> >  			die_errno(_("could not redirect output"));
> > +		}
> >  		else
> >  			close(output_fd);
> >  	}
> 
> Ah, I guess you found the problem from patch 3. But it should have been
> squashed in there, not to this patch. (But as I said there, I think we
> should just drop patch 3 entirely).

Er, sorry, I meant patch 2. Counting is hard.

-Peff

Re: [PATCH 0/4] Fix resource leaks in various helpers and builtin commands

[Jeff King]

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

> This patch series fixes several cases where file descriptors were not
> properly closed on error paths. The changes affect helper programs and
> a builtin command, and ensure that system resources are correctly
> released before returning from the function.
> 
> Each fix is minimal and follows the existing style of the surrounding
> code. These changes help improve the robustness of the code by avoiding
> potential file descriptor leaks.
> 
> Hoyoung Lee (4):
>   t/helper/test-truncate: close file descriptor after truncation
>   builtin/archive: close file descriptor on dup2() failure
>   t/helper/test-delta: close fd if fstat() fails after open()
>   t/helper/test-delta: close fd if fstat() fails after second open()

I looked through these and I think patches 1, 3, and 4 are all good
(minus the fixup for patch 2 that snuck into patch 4). I responded to
patch 2 in the v1 thread, and i think it should be dropped.

In each case the descriptor is leaked before exiting from the main
function, so they're not practical leaks (the OS will close them for us
anyway). But it seems reasonable to me to close them anyway. It's a good
general practice, and it could help with any tools that try to
auto-detect leaked descriptors.

Out of curiosity, are you using such a tool, or just finding these
manually? I could imagine coverity or some other static analysis tool
finding them. I've never seen a tool which finds descriptor leaks the
way we find memory leaks with LSan, etc. I'd guess it would be hard to
distinguish "reachable" global descriptors that are meant to be held
open versus true leaks (doubly so when we call die() in the middle of a
deep call chain).

One non-obvious thing to consider in reviewing this: sometimes calling
close(), or any other syscall, in an error code path may clobber errno,
breaking a message we may print (or even confusing our caller if they
check errno). But I don't see any problems in these three sites.

-Peff

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

[Eric Sunshine]

On Tue, Jul 22, 2025 at 4:12 AM Hoyoung Lee <lhywkd22@gmail.com> wrote:
> 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>
> ---
> diff --git 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;
>         }

One condition under which this block is entered is if `fd` is less
than 0, which means close() is now being called with a negative file
descriptor, which seems quite suspect. I'd think you would want to
either restructure it into two `if` statements:

    if (fd < 0) {
        ...
        return 1;
    }
    if (fstat(fd, ...)) {
        ...
        return 1;
    }

or at least protect the call to close():

    if (fd < 0 || fstat(fd, ...)) {
        ...
        if (fd >= 0)
            close(fd);
        return 1;
    }

I think the separate `if` statements are probably a bit easier to
reason about, but it is of course subjective.

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

[Jeff King]

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

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jul 22, 2025 at 04:25:57AM -0400, Jeff King wrote:
>
>> >  	int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
>> >  	if (output_fd != 1) {
>> > -		if (dup2(output_fd, 1) < 0)
>> > +		if (dup2(output_fd, 1) < 0) {
>> >  			close(output_fd);
>> >  			die_errno(_("could not redirect output"));
>> > +		}
>> >  		else
>> >  			close(output_fd);
>> >  	}
>> 
>> Ah, I guess you found the problem from patch 3. But it should have been
>> squashed in there, not to this patch. (But as I said there, I think we
>> should just drop patch 3 entirely).
>
> Er, sorry, I meant patch 2. Counting is hard.

;-)

Thanks for taking a look on these patches before I wake up, so I
didn't have to ;-)