[Mesa-dev] [PATCH 2/4] xlib: fix memory leak of and remove vishandle from XMesaVisualInfo

[Alejandro Piñeiro]

Assuming that the comments on the previous patch would be updated:

Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>

On 02/04/16 01:52, John Sheu wrote:
> The vishandle member of XMesaVisualInfo is used to support the
> comparison of XVisualInfo instances by pointer value, in
> find_glx_visual().  The comparison however will always be false, as in
> every case the comparison is made, the VisualInfo instance being
> compared to is a new allocation passed in through a GLX API call.
>
> In addition, the XVisualInfo instance pointed to by vishandle is itself
> never freed, causing a memory leak.  Since vishandle is essentially
> useless, we just remove it and thereby also fix the leak.
> ---
>  src/mesa/drivers/x11/fakeglx.c | 62 ++++++++++++++++--------------------------
>  src/mesa/drivers/x11/xmesaP.h  |  1 -
>  2 files changed, 24 insertions(+), 39 deletions(-)
>
> diff --git a/src/mesa/drivers/x11/fakeglx.c b/src/mesa/drivers/x11/fakeglx.c
> index d62d5abd..208fc5bb 100644
> --- a/src/mesa/drivers/x11/fakeglx.c
> +++ b/src/mesa/drivers/x11/fakeglx.c
> @@ -256,7 +256,6 @@ save_glx_visual( Display *dpy, XVisualInfo *vinfo,
>     GLboolean ximageFlag = GL_TRUE;
>     XMesaVisual xmvis;
>     GLint i;
> -   GLboolean comparePointers;
>  
>     if (dbFlag) {
>        /* Check if the MESA_BACK_BUFFER env var is set */
> @@ -279,37 +278,34 @@ save_glx_visual( Display *dpy, XVisualInfo *vinfo,
>        return NULL;
>     }
>  
> -   /* Comparing IDs uses less memory but sometimes fails. */
> -   /* XXX revisit this after 3.0 is finished. */
> -   if (getenv("MESA_GLX_VISUAL_HACK"))
> -      comparePointers = GL_TRUE;
> -   else
> -      comparePointers = GL_FALSE;
>  
>     /* Force the visual to have an alpha channel */
>     if (getenv("MESA_GLX_FORCE_ALPHA"))
>        alphaFlag = GL_TRUE;
>  
> -   /* First check if a matching visual is already in the list */
> -   for (i=0; i<NumVisuals; i++) {
> -      XMesaVisual v = VisualTable[i];
> -      if (v->display == dpy
> -          && v->mesa_visual.level == level
> -          && v->mesa_visual.numAuxBuffers == numAuxBuffers
> -          && v->ximage_flag == ximageFlag
> -          && v->mesa_visual.doubleBufferMode == dbFlag
> -          && v->mesa_visual.stereoMode == stereoFlag
> -          && (v->mesa_visual.alphaBits > 0) == alphaFlag
> -          && (v->mesa_visual.depthBits >= depth_size || depth_size == 0)
> -          && (v->mesa_visual.stencilBits >= stencil_size || stencil_size == 0)
> -          && (v->mesa_visual.accumRedBits >= accumRedSize || accumRedSize == 0)
> -          && (v->mesa_visual.accumGreenBits >= accumGreenSize || accumGreenSize == 0)
> -          && (v->mesa_visual.accumBlueBits >= accumBlueSize || accumBlueSize == 0)
> -          && (v->mesa_visual.accumAlphaBits >= accumAlphaSize || accumAlphaSize == 0)) {
> -         /* now either compare XVisualInfo pointers or visual IDs */
> -         if ((!comparePointers && v->visinfo->visualid == vinfo->visualid)
> -             || (comparePointers && v->vishandle == vinfo)) {
> -            return v;
> +   /* Comparing IDs uses less memory but sometimes fails. */
> +   /* XXX revisit this after 3.0 is finished. */
> +   if (!getenv("MESA_GLX_VISUAL_HACK")) {
> +      /* First check if a matching visual is already in the list */
> +      for (i=0; i<NumVisuals; i++) {
> +         XMesaVisual v = VisualTable[i];
> +         if (v->display == dpy
> +             && v->mesa_visual.level == level
> +             && v->mesa_visual.numAuxBuffers == numAuxBuffers
> +             && v->ximage_flag == ximageFlag
> +             && v->mesa_visual.doubleBufferMode == dbFlag
> +             && v->mesa_visual.stereoMode == stereoFlag
> +             && (v->mesa_visual.alphaBits > 0) == alphaFlag
> +             && (v->mesa_visual.depthBits >= depth_size || depth_size == 0)
> +             && (v->mesa_visual.stencilBits >= stencil_size || stencil_size == 0)
> +             && (v->mesa_visual.accumRedBits >= accumRedSize || accumRedSize == 0)
> +             && (v->mesa_visual.accumGreenBits >= accumGreenSize || accumGreenSize == 0)
> +             && (v->mesa_visual.accumBlueBits >= accumBlueSize || accumBlueSize == 0)
> +             && (v->mesa_visual.accumAlphaBits >= accumAlphaSize || accumAlphaSize == 0)) {
> +            /* now compare visual IDs */
> +            if (v->visinfo->visualid == vinfo->visualid) {
> +               return v;
> +            }
>           }
>        }
>     }
> @@ -323,10 +319,6 @@ save_glx_visual( Display *dpy, XVisualInfo *vinfo,
>                                accumBlueSize, accumAlphaSize, 0, level,
>                                GLX_NONE_EXT );
>     if (xmvis) {
> -      /* Save a copy of the pointer now so we can find this visual again
> -       * if we need to search for it in find_glx_visual().
> -       */
> -      xmvis->vishandle = vinfo;
>        /* Allocate more space for additional visual */
>        VisualTable = realloc(VisualTable, sizeof(XMesaVisual) * (NumVisuals + 1));
>        /* add xmvis to the list */
> @@ -442,13 +434,6 @@ find_glx_visual( Display *dpy, XVisualInfo *vinfo )
>        }
>     }
>  
> -   /* if that fails, try to match pointers */
> -   for (i=0;i<NumVisuals;i++) {
> -      if (VisualTable[i]->display==dpy && VisualTable[i]->vishandle==vinfo) {
> -         return VisualTable[i];
> -      }
> -   }
> -
>     return NULL;
>  }
>  
> @@ -1225,6 +1210,7 @@ choose_visual( Display *dpy, int screen, const int *list, GLboolean fbConfig )
>                                 stereo_flag, depth_size, stencil_size,
>                                 accumRedSize, accumGreenSize,
>                                 accumBlueSize, accumAlphaSize, level, numAux );
> +      free(vis);
>     }
>  
>     return xmvis;
> diff --git a/src/mesa/drivers/x11/xmesaP.h b/src/mesa/drivers/x11/xmesaP.h
> index d7a934e8..6cd020f2 100644
> --- a/src/mesa/drivers/x11/xmesaP.h
> +++ b/src/mesa/drivers/x11/xmesaP.h
> @@ -78,7 +78,6 @@ struct xmesa_visual {
>     int screen, visualID;
>     int visualType;
>     XMesaVisualInfo visinfo;	/* X's visual info (pointer to private copy) */
> -   XVisualInfo *vishandle;	/* Only used in fakeglx.c */
>     GLint BitsPerPixel;		/* True bits per pixel for XImages */
>  
>     GLboolean ximage_flag;	/* Use XImage for back buffer (not pixmap)? */