Re: [PATCH v2 1/4] xdiff: fix a memory leak

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 17, 2022, 11:05 UTC
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:

Show 15 quoted lines

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