[Mesa-dev] [PATCH 1/4] xlib: fix memory leak in glXChooseVisual/glXGetVisualFromFBConfig

[John Sheu]

Whoops.  Patch updated on a different thread.

-John Sheu

On Sun, Apr 3, 2016 at 11:33 PM, Alejandro Piñeiro <apinheiro at igalia.com> wrote:
> On 04/04/16 08:11, Alejandro Piñeiro wrote:
>> On 02/04/16 01:52, John Sheu wrote:
>>> XMesaVisual.vishandle is being unconditionally overwritten every time
>>> glXChooseVisual/glXGetVisualFromFBConfig returns a new XVisualInfo
>>> instance, causing a memory leak.
>>> ---
>>>  src/mesa/drivers/x11/fakeglx.c | 26 ++++++++------------------
>>>  1 file changed, 8 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/x11/fakeglx.c b/src/mesa/drivers/x11/fakeglx.c
>>> index 2f4d9669..d62d5abd 100644
>>> --- a/src/mesa/drivers/x11/fakeglx.c
>>> +++ b/src/mesa/drivers/x11/fakeglx.c
>>> @@ -1241,16 +1241,11 @@ Fake_glXChooseVisual( Display *dpy, int screen, int *list )
>>>
>>>     xmvis = choose_visual(dpy, screen, list, GL_FALSE);
>>>     if (xmvis) {
>>> -#if 0
>>> -      return xmvis->vishandle;
>>> -#else
>>> -      /* create a new vishandle - the cached one may be stale */
>> This comments points that xmvis->vishandle is a cached value that needs
>> to be updated. And taking a quick look on the code, is used in other parts.
>>
>>> -      xmvis->vishandle = malloc(sizeof(XVisualInfo));
>>> -      if (xmvis->vishandle) {
>>> -         memcpy(xmvis->vishandle, xmvis->visinfo, sizeof(XVisualInfo));
>> The old code created space for vishandle and assigned the value of
>> xmvis->visinfo.
>>
>>> +      XVisualInfo* visinfo = malloc(sizeof(XVisualInfo));
>>> +      if (visinfo) {
>>> +         memcpy(visinfo, xmvis->visinfo, sizeof(XVisualInfo));
>> But the new code created a new XVisualInfo, and returned it, without
>> updating xmis>vishandle, so the new code seems to do something
>> different. Additionally, I don't see how this would prevent the memory
>> leak, as a new XVisualInfo is created each time it is called, without
>> being assigned to the structure.
>>
>> If you want to prevent the leak, I think that it would be better to
>> maintain the same code, but freeing the previous xmvis->vishandle if it
>> is different to NULL.
>
> Ok. Nevermind, I see that you remove vishandle on a following patch, and
> frees the newly created visinfo. But that means that the leak is has the
> old code is not fixed with this patch, but with the following one.
> Probably it would be better to update the commit summary and description.
>
>>
>>>        }
>>> -      return xmvis->vishandle;
>>> -#endif
>>> +      return visinfo;
>>>     }
>>>     else
>>>        return NULL;
>>> @@ -1974,16 +1969,11 @@ Fake_glXGetVisualFromFBConfig( Display *dpy, GLXFBConfig config )
>>>  {
>>>     if (dpy && config) {
>>>        XMesaVisual xmvis = (XMesaVisual) config;
>>> -#if 0
>>> -      return xmvis->vishandle;
>>> -#else
>>> -      /* create a new vishandle - the cached one may be stale */
>>> -      xmvis->vishandle = malloc(sizeof(XVisualInfo));
>>> -      if (xmvis->vishandle) {
>>> -         memcpy(xmvis->vishandle, xmvis->visinfo, sizeof(XVisualInfo));
>>> +      XVisualInfo* visinfo = malloc(sizeof(XVisualInfo));
>>> +      if (visinfo) {
>>> +         memcpy(visinfo, xmvis->visinfo, sizeof(XVisualInfo));
>>>        }
>>> -      return xmvis->vishandle;
>>> -#endif
>>> +      return visinfo;
>>>     }
>> Ditto.
>>
>> And as I mentioned on my review to your "xlib: fix memory leak on
>> Display close" patch, this code is C&P in other places of the code.
>>
>> BRb
>>>     else {
>>>        return NULL;
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>