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
