From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 17 Feb 2022 11:05:26 GMT
Subject: Re: [PATCH v2 1/4] xdiff: fix a memory leak
Message-ID: <08f1cc5f-672f-f806-49d9-a6d6be3d2a8b@gmail.com>
In-Reply-To: <220216.86h78yuar0.gmgdl@evledraar.gmail.com>

On 16/02/2022 14:38, Ævar Arnfjörð Bjarmason wrote:
> [...]
>>> More generally I think this patch is taking the wrong approach, why are
>>> we trying to the members of a stack variable in xdl_do_diff(), when that
>>> variable isn't ours, but is created by our caller? Why aren't they
>>> dealing with it?
>>
>> They are not dealing with it because they do not initialize it - it is
>> an "out" parameter that is used to return data to the caller. This
>> patch changes the logic to "whoever initializes it is responsible for
>> freeing it if there is an error". By doing that we localize the error
>> handling to xdl_do_diff() and can leave the callers unchanged.
> 
> Yes, I'm saying that we're needlessly piling on complexity by continuing
> with this pattern in the xdiff/ codebase. I think it's fair to question
> the direction in general.

I think if there were a lot of these bugs that needed fixing then the 
wholesale changes to the way memory is managed you seem to be suggesting 
would be worth it. However I think the existing code is mostly correct 
so such a change would be a lot of churn and would absorb a lot of 
reviewer time that would be better spent elsewhere compared to fixing up 
the code we have.

Best Wishes

Phillip