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

From: Jeff King <peff@peff.net>
Date: Jul 22, 2025, 08:35 UTC
Message-ID: <20250722083516.GE862006@coredump.intra.peff.net>
In-Reply-To: <20250722081219.1086866-1-lhywkd22@gmail.com>

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

Show 14 quoted lines

> 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