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

> 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.