Re: [PATCH v3 2/3] refs/files-backend: fix memory leak in lock_ref_for_update

["Martin Ågren" <martin.agren@gmail.com>]

On 5 September 2017 at 10:47, Jeff King <peff@peff.net> wrote:
> On Tue, Aug 29, 2017 at 07:18:23PM +0200, Martin Ågren wrote:
>
>> After the previous patch, none of the functions we call hold on to
>> `referent.buf`, so we can safely release the string buffer before
>> returning.
>
> I ended up doing this a little differently in my version:
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 9266f5ab9d..1d16c1b33e 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2287,9 +2292,12 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>                          * the transaction, so we have to read it here
>                          * to record and possibly check old_sha1:
>                          */
> -                       if (refs_read_ref_full(&refs->base,
> -                                              referent.buf, 0,
> -                                              lock->old_oid.hash, NULL)) {
> +                       ret = refs_read_ref_full(&refs->base,
> +                                                referent.buf, 0,
> +                                                lock->old_oid.hash, NULL);
> +                       strbuf_release(&referent);
> +
> +                       if (ret) {
>                                 if (update->flags & REF_HAVE_OLD) {
>                                         strbuf_addf(err, "cannot lock ref '%s': "
>                                                     "error reading reference",
> @@ -2310,6 +2318,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>                         ret = split_symref_update(refs, update,
>                                                   referent.buf, transaction,
>                                                   affected_refnames, err);
> +                       strbuf_release(&referent);
>                         if (ret)
>                                 return ret;
>                 }
>
> After we look at referent.buf once in each of the branch arms, we don't
> need it at all. Disposing of it there means we don't have to worry about
> it in all of the later early-returns.
>
> I'm assuming that referent will always be empty unless REF_ISSYMREF is
> set. Which seems reasonable, but I didn't double check.

Some time after I posted v3, I had the same thought. I did double-check
and it does hold. But then I thought that even if it holds now, maybe
it's a bit too brittle. On the other hand, my patch is quite noisy and
maybe the connection between the two is obvious enough that it will hold
in the future as well.

> Food for thought. I'd be OK with either version.

So would I...

Martin