Re: [PATCH v2] object-file: use real paths when adding alternates

From: Glen Choo <chooglen@google.com>
Date: Nov 24, 2022, 00:50 UTC
Message-ID: <kl6lwn7lch1h.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <Y30onDTUFmAezkSl@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 19 quoted lines

>> Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the
>> "REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the
>> &tmp you pass in here) and then "does a "goto error_out".
>> 
>> It then *resets* the strbuf, but doesn't release it, assuming that
>> you're going to pass it in again. So in that case we'd leak here, no?
>> 
>> I.e. a NULL return value from strbuf_realpath() doesn't mean that it
>> didn't allocate in the scratch area passed to it, so we need to
>> strbuf_release(&tmp) here too.
>
> We don't use MANY_MISSING in this code path, but I didn't read
> strbuf_realpath_1() carefully enough to see if that is the only case.
> But regardless, I think it is a bug in strbuf_realpath(). All of the
> strbuf functions generally try to leave a buffer untouched on error.
>
> So IMHO we would want a preparatory patch with s/reset/release/ in that
> function, which better matches the intent (we might be freeing an
> allocated buffer, but that's OK from the caller perspective).

Is that always OK? I would think that we'd do something closer to strbuf_getcwd():

  int strbuf_getcwd(struct strbuf *sb)
  {
    size_t oldalloc = sb->alloc;
    /* ... */
    if (oldalloc == 0)
      strbuf_release(sb);
    else
      strbuf_reset(sb);
  }

i.e. if the caller passed in a strbuf with allocated contents, they're responsible for free()-ing it, otherwise we free() it. That does fix the leak in this patch, but I don't feel strongly enough about changing strbuf_realpath() to do it now, so I'll do without the change for now.