[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        | 1 +
 t/helper/test-delta.c    | 3 +++
 t/helper/test-truncate.c | 3 +++
 3 files changed, 7 insertions(+)

-- 
2.34.1

[PATCH 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 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 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 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>
---
 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 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

[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

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