Re: [PATCH v2 2/2] refs/files-backend: fix memory leak in lock_ref_for_update
- Date: Tue, 29 Aug 2017 12:41:49 +0200
- From: Martin Ågren <martin.agren@xxxxxxxxx>
- Subject: Re: [PATCH v2 2/2] refs/files-backend: fix memory leak in lock_ref_for_update
On 29 August 2017 at 10:39, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> 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
> This patch looks good to me, but I did notice a pre-existing problem in
> the area...
>> 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",
>> - 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.
Sure. I'll send out a v3 later today. I'll fix this in a third patch,
and I'll also address your comments on the first patch.
Thanks a lot.