patch, 4 partsFix resource leaks in various helpers and builtin commands

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

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(+)

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(+)

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

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.

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

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(+)

On Tue, Jul 22, 2025 at 4:12 AM Hoyoung Lee <lhywkd22@gmail.com> wrote:

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.

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

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:

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

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

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

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

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

-Peff

Jeff King <peff@peff.net> writes:

;-)

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

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

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