From: Jeff King Date: Tue, 05 Sep 2017 08:47:52 GMT Subject: Re: [PATCH v3 2/3] refs/files-backend: fix memory leak in lock_ref_for_update Message-ID: <20170905084751.c4rbeuxx5rdqwdgo@sigill.intra.peff.net> In-Reply-To: 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. Food for thought. I'd be OK with either version. -Peff