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

[Alejandro Piñeiro]

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.

>        }
> -      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;