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:17 UTC
- Message-ID
- <CAPig+cQAcEiM7aPQZXYX03Ok_qZktWpAgPPS9DUWu3UPVrxx7Q@mail.gmail.com>
- In-Reply-To
- <20250723080639.GA570920@coredump.intra.peff.net>
On Wed, Jul 23, 2025 at 4:06 AM Jeff King <peff@peff.net> wrote:
Show 8 quoted lines
> On Wed, Jul 23, 2025 at 03:55:13AM -0400, Jeff King wrote: > > I dunno. We are reaching diminishing returns spending brainpower on a > > function that is meant to be somewhat quick-and-dirty. > > OK, I clearly could not resist spending more brainpower on it. If we are > doing quick-and-dirty, why not just die()? The end result is the same, > but per my argument in the earlier iteration of the series, that means > we do not have to worry about cleaning up at all.
Yes, die() seems sensible here. It's nice and tidy and makes the code easier to reason about.
Show 12 quoted lines
> Incidentally that would also fix two minor bugs I noticed: > > - passing st.st_size directly to xmalloc() is wrong, because of > truncation from off_t to size_t. This should use the xsize_t helper. > This is even a potential security vulnerability, but probably not > important in a test helper. > > - likewise read_in_full() might return a non-negative value smaller > than the requested size (if the file racily changes and we get an > early EOF). But we only check whether we got a negative error value. > So we may read fewer bytes than expected and feed uninitialized > garbage to the delta code.
Can of worms opened.