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

From: Jeff King <peff@peff.net>
Date: Jul 22, 2025, 08:24 UTC
Message-ID: <20250722082451.GB862006@coredump.intra.peff.net>
In-Reply-To: <20250722081219.1086866-3-lhywkd22@gmail.com>

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.

Show 12 quoted lines

> 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