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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 23, 2025, 08:11 UTC
Message-ID: <CAPig+cQ2p3muA0-8O_u-rOW0uVGQ_ZE7hcz-7qq8M1U+PdvtpQ@mail.gmail.com>
In-Reply-To: <20250723075513.GA570540@coredump.intra.peff.net>

On Wed, Jul 23, 2025 at 3:55 AM Jeff King <peff@peff.net> wrote:

Show 11 quoted lines

> On Wed, Jul 23, 2025 at 03:28:05AM -0400, Eric Sunshine wrote:
> > The descriptor is closed manually (again) because a subsequent open()
> > call is going to reuse the variable. However...
> > ...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 dunno. We are reaching diminishing returns spending brainpower on a
> function that is meant to be somewhat quick-and-dirty.

Aside from preferring that the patch not make the code worse or more confusing, I don't have a strong opinion. Superficially, the existing code presents itself as being careful by cleaning up after itself, but as Hoyoung Lee discovered, there are holes in the cleanup implementation. So, I do like that the intention of the patch is to plug those holes, but we don't necessarily need to be particularly clever about it. For such simple test-related code, even a pure brute-force fix should be acceptable.

For completeness, I'll mention that I even had the thought that another "fix" would be to tear out all the cleanup code entirely since we _know_ that this function will be exiting immediately and the OS will clean up any dangling resources.