[Mesa-dev] [PATCH] st/mesa: fix memleak in glDrawPixels cache code

[Jose Fonseca]

On 12/04/16 16:15, Brian Paul wrote:
> If the glDrawPixels size changed, we leaked the previously cached
> texture, if there was one.  This patch fixes the reference counting,
> adds a refcount assertion check, and better handles potential malloc()
> failures.
>
> Tested with a modified version of the drawpix Mesa demo which changed
> the image size for each glDrawPixels call.
>
> Cc: "11.2" <mesa-stable at lists.freedesktop.org>
> ---
>   src/mesa/state_tracker/st_cb_drawpixels.c | 23 ++++++++++++++++++-----
>   1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c b/src/mesa/state_tracker/st_cb_drawpixels.c
> index 01ed544..3c7bc0c 100644
> --- a/src/mesa/state_tracker/st_cb_drawpixels.c
> +++ b/src/mesa/state_tracker/st_cb_drawpixels.c
> @@ -384,7 +384,7 @@ make_texture(struct st_context *st,
>      struct gl_context *ctx = st->ctx;
>      struct pipe_context *pipe = st->pipe;
>      mesa_format mformat;
> -   struct pipe_resource *pt;
> +   struct pipe_resource *pt = NULL;
>      enum pipe_format pipeFormat;
>      GLenum baseInternalFormat;
>
> @@ -403,10 +403,18 @@ make_texture(struct st_context *st,
>          unpack->SkipRows == 0 &&
>          unpack->SwapBytes == GL_FALSE &&
>          st->drawpix_cache.image) {
> +      assert(st->drawpix_cache.texture);
> +
>         /* check if the pixel data is the same */
>         if (memcmp(pixels, st->drawpix_cache.image, width * height * bpp) == 0) {
>            /* OK, re-use the cached texture */
> -         return st->drawpix_cache.texture;
> +         pipe_resource_reference(&pt, st->drawpix_cache.texture);
> +         /* refcount of returned texture should be at least two here.  One
> +          * reference for the cache to hold on to, one for the caller (which
> +          * it will release), and possibly more held by the driver.
> +          */
> +         assert(pt->reference.count >= 2);
> +         return pt;
>         }
>      }
>
> @@ -525,8 +533,14 @@ make_texture(struct st_context *st,
>         st->drawpix_cache.image = malloc(width * height * bpp);
>         if (st->drawpix_cache.image) {
>            memcpy(st->drawpix_cache.image, pixels, width * height * bpp);
> +         pipe_resource_reference(&st->drawpix_cache.texture, pt);
> +      }
> +      else {
> +         /* out of memory, free/disable cached texture */
> +         st->drawpix_cache.width = 0;
> +         st->drawpix_cache.height = 0;
> +         pipe_resource_reference(&st->drawpix_cache.texture, NULL);
>         }
> -      st->drawpix_cache.texture = pt;
>      }
>   #endif
>
> @@ -1160,9 +1174,8 @@ st_DrawPixels(struct gl_context *ctx, GLint x, GLint y,
>      if (num_sampler_view > 1)
>         pipe_sampler_view_reference(&sv[1], NULL);
>
> -#if !USE_DRAWPIXELS_CACHE
> +   /* free the texture (but may persist in the cache) */
>      pipe_resource_reference(&pt, NULL);
> -#endif
>   }
>
>

Looks good AFAICT.

Reviewed-by: Jose Fonseca <jfonseca at vmware.com>