[PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Emil Velikov]

On 10 April 2018 at 10:58, Michel Dänzer <michel at daenzer.net> wrote:
> On 2018-04-10 11:47 AM, Emil Velikov wrote:
>> On 10 April 2018 at 09:28, Michel Dänzer <michel at daenzer.net> wrote:
>>> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>>>> From: Emil Velikov <emil.velikov at collabora.com>
>>>>
>>>> Seems like we've been leaking this for years. It became more obvious
>>>> with the recent refactoring.
>>>>
>>>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>>>> ---
>>>>  src/amdgpu_probe.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>>>> index 537d44c..588891c 100644
>>>> --- a/src/amdgpu_probe.c
>>>> +++ b/src/amdgpu_probe.c
>>>> @@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
>>>>       return TRUE;
>>>>
>>>>  error:
>>>> +     free(pPriv->ptr);
>>>> +     pPriv->ptr = NULL;
>>>>       return FALSE;
>>>>  }
>>>>
>>>>
>>>
>>> valgrind doesn't report a leak if I force this error path; presumably
>>> Xorg frees the private after returning FALSE here.
>>>
>> Just double-checked and Xorg does not know anything about ptr. The
>> only one who clears it up is AMDGPUFreeScreen_KMS.
>>
>> The magic (for this and the other 'leak') seems to be happening in
>> xf86platformAddDevice. Namely:
>>  - ::platformProbe is called via doPlatformProbe
>>  - the driver explicitly calls xf86AllocateScreen, yet fails later on
>>  - back in Xorg, the "if (old_screens == xf86NumGPUDrivers)" is false
>>  - ::PreInit fails, ::configured is false
>>  - xf86DeleteScreen() gets called, which dives into ::FreeScreen (aka
>> AMDGPUFreeScreen_KMS)
>>
>> Eventually, I could unwrap all that although it makes sense to keep
>> things simpler. As effectively done by the patch.
>>
>> I believe you'll agree?
>
> I'm afraid not. There's no leak because it's getting cleaned up as
> designed, so there's no need for this change.
>
Fair enough. I'll swap the commit with a comment one for v2.
This way, the next person will be less tempted to send the same patch.

Something like:

pPriv->ptr is freed in our ::FreeScreen callback. Latter of which gets
called by xf86DeleteScreen() as the driver ::*Probe call fails.

-Emil