[Mesa-dev] [PATCH] mesa: fix renderbuffer leak

[Brian Paul]

On 04/06/2017 03:55 PM, Timothy Arceri wrote:
> We don't need to call _mesa_reference_renderbuffer() for the first
> assignment as refCount starts at 1. For swrast we work around the
> fact we will indirectly call _mesa_reference_renderbuffer() by
> resetting refCount to 0.
>
> Fixes: 32141e53d1520 (mesa: tidy up renderbuffer RefCount initialisation)
> ---
>   src/mesa/main/fbobject.c         | 2 +-
>   src/mesa/swrast/s_renderbuffer.c | 5 +++++
>   2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index e52c1e3..e46a7ca 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -441,21 +441,21 @@ _mesa_update_texture_renderbuffer(struct gl_context *ctx,
>
>      texImage = att->Texture->Image[att->CubeMapFace][att->TextureLevel];
>
>      rb = att->Renderbuffer;
>      if (!rb) {
>         rb = ctx->Driver.NewRenderbuffer(ctx, ~0);
>         if (!rb) {
>            _mesa_error(ctx, GL_OUT_OF_MEMORY, "glFramebufferTexture()");
>            return;
>         }
> -      _mesa_reference_renderbuffer(&att->Renderbuffer, rb);
> +      att->Renderbuffer = rb;
>
>         /* This can't get called on a texture renderbuffer, so set it to NULL
>          * for clarity compared to user renderbuffers.
>          */
>         rb->AllocStorage = NULL;
>
>         rb->NeedsFinishRenderTexture = ctx->Driver.FinishRenderTexture != NULL;
>      }
>
>      if (!texImage)
> diff --git a/src/mesa/swrast/s_renderbuffer.c b/src/mesa/swrast/s_renderbuffer.c
> index d8c4ed3..af09955 100644
> --- a/src/mesa/swrast/s_renderbuffer.c
> +++ b/src/mesa/swrast/s_renderbuffer.c
> @@ -264,20 +264,25 @@ add_color_renderbuffers(struct gl_context *ctx, struct gl_framebuffer *fb,
>            continue;
>
>         assert(fb->Attachment[b].Renderbuffer == NULL);
>
>         rb = ctx->Driver.NewRenderbuffer(ctx, 0);
>         if (!rb) {
>            _mesa_error(ctx, GL_OUT_OF_MEMORY, "Allocating color buffer");
>            return GL_FALSE;
>         }
>
> +      /* Set refcount to 0 to avoid a leak since the _mesa_add_renderbuffer()
> +       * call below will bump the initial refcount.
> +       */
> +      rb->RefCount = 0;
> +
>         rb->InternalFormat = GL_RGBA;
>
>         rb->AllocStorage = soft_renderbuffer_storage;
>         _mesa_add_renderbuffer(fb, b, rb);

Perhaps another way of doing this which wouldn't look quite so hacky 
would be to call _mesa_renderbuffer_reference(&rb, NULL) here, after the 
_mesa_add_renderbuffer() call.  The idea is we're transferring ownership 
of the rb and the rb var is going out of scope.

Something similar could be done above.  A comment indicating that we're 
transferring ownership could be helpful too.

-Brian

>      }
>
>      return GL_TRUE;
>   }
>
>
>