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@pks.im> 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@pks.im>
> > ---
> > 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