Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup

From: Jeff King <peff@peff.net>
Date: Jul 23, 2025, 07:55 UTC
Message-ID: <20250723075513.GA570540@coredump.intra.peff.net>
In-Reply-To: <CAPig+cR-r=CeEaSTeWsX00MLCSRJUUVXMUWS6Ui-HQcR_qMGJA@mail.gmail.com>

On Wed, Jul 23, 2025 at 03:28:05AM -0400, Eric Sunshine wrote:

Show 20 quoted lines

> >   close(fd);
> 
> The descriptor is closed manually (again) because a subsequent open()
> call is going to reuse the variable. However...
> 
> >   if (argv[1][1] == 'd')
> >     out_buf = diff_delta(from_buf, from_size,
> >              data_buf, data_size,
> >              &out_size, 0);
> >   else
> >     out_buf = patch_delta(from_buf, from_size,
> >               data_buf, data_size,
> >               &out_size);
> >   if (!out_buf) {
> >     fprintf(stderr, "delta operation failed (returned NULL)\n");
> >     goto cleanup;
> >   }
> 
> ...although `fd` was closed, it still holds the previously-open
> non-negative file descriptor, which means that this `goto cleanup`...

Oof, good catch. This iteration of the patch was based on my suggestion, but I didn't notice the jump to cleanup between that close/open pair.

I wonder if it would be more clear written as:

  int from_fd = -1;
  int data_fd = -1;
  int out_fd = -1;

  ...
  from_fd = open(...);
  if (...errors...)
	goto cleanup;
  close(from_fd);
  from_fd = -1;

  [...same for other fds...]

cleanup:

  if (from_fd >= 0)
	close(from_fd);
  [etc]

You could even drop that first close() in the happy path entirely, if you don't mind holding all three descriptors open at once.

I dunno. We are reaching diminishing returns spending brainpower on a function that is meant to be somewhat quick-and-dirty.

-Peff