On Wed, Oct 18, 2023 at 01:08:45PM -0400, Eric Sunshine wrote:
> On Wed, Oct 18, 2023 at 1:35 AM Patrick Steinhardt <ps@xxxxxx> wrote:
> > Introduce a new subcommand for our ref-store test helper that explicitly
> > checks only for the presence or absence of a reference. This addresses
> > these limitations:
> > [...]
> > Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> > ---
> > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
> > @@ -221,6 +221,30 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv)
> > +static int cmd_ref_exists(struct ref_store *refs, const char **argv)
> > +{
> > + const char *refname = notnull(*argv++, "refname");
> > + struct strbuf unused_referent = STRBUF_INIT;
> > + struct object_id unused_oid;
> > + unsigned int unused_type;
> > + int failure_errno;
> > +
> > + if (refs_read_raw_ref(refs, refname, &unused_oid, &unused_referent,
> > + &unused_type, &failure_errno)) {
> > + /*
> > + * We handle ENOENT separately here such that it is possible to
> > + * distinguish actually-missing references from any kind of
> > + * generic error.
> > + */
> > + if (failure_errno == ENOENT)
> > + return 17;
> > + return -1;
> > + }
> > +
> > + strbuf_release(&unused_referent);
> > + return 0;
> > +}
>
> Unless refs_read_raw_ref() guarantees that `unused_referent` remains
> unallocated upon failure[*], then the early returns inside the
> conditional leak the strbuf. True, the program is exiting immediately
> anyhow, so this (potential) leak isn't significant, but it seems odd
> to clean up in one case (return 0) but not in the others (return -1 &
> 17).
>
> [*] In my (admittedly brief) scan of the code and documentation, I
> didn't see any such promise.
Agreed, let's just be thorough and plug any potential memor leak here.
Patrick
Attachment:
signature.asc
Description: PGP signature
