Jeff King [off-list ref] writes:

quoted

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.