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

[Alejandro Piñeiro]

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