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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Aug 29, 2017, 08:39 UTC
Message-ID: <425e16e7-188e-ecff-3a09-228ef1dcf319@alum.mit.edu>
In-Reply-To: <fe842671061cea7972b0c8695a4b7f82f854453a.1503951733.git.martin.agren@gmail.com>

On 08/28/2017 10:32 PM, 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.

This patch looks good to me, but I did notice a pre-existing problem in the area...

Show 16 quoted lines

> ---
>  refs/files-backend.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index bdb0e22e5..15f34b10e 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> [...]
> @@ -2305,10 +2305,12 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>  					strbuf_addf(err, "cannot lock ref '%s': "
>  						    "error reading reference",
>  						    original_update_refname(update));
> -					return -1;
> +					ret = -1;
> +					goto out;

It is incorrect to return -1 here. First of all, stylistically, the return value should be a symbolic constant. But in fact, it should be returning `TRANSACTION_GENERIC_ERROR` here, whereas -1 is `TRANSACTION_NAME_CONFLICT`. So the code is not just stylistically wrong; it is functionally wrong.

I know that this is not your mistake, but would you like to add another patch to your series to fix this up? I'd do it myself, but it's a little bit awkward because the fix will conflict with your patch.

Thanks, Michael