[Mesa-dev] Mesa (master): gallium/u_upload_mgr: fix a serious memory leak

[Brian Paul]

On 01/27/2013 06:52 PM, Marek Olšák wrote:
> Module: Mesa
> Branch: master
> Commit: 87592cff57feef29565150b9203e220b50623f30
> URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=87592cff57feef29565150b9203e220b50623f30
>
> Author: Marek Olšák<maraeo at gmail.com>
> Date:   Mon Jan 28 02:47:24 2013 +0100
>
> gallium/u_upload_mgr: fix a serious memory leak
>
> It can eat all memory and crash in a matter of minutes with r600g.
>
> ---
>
>   src/gallium/auxiliary/util/u_upload_mgr.c |    3 +--
>   1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/auxiliary/util/u_upload_mgr.c b/src/gallium/auxiliary/util/u_upload_mgr.c
> index 47d39af..6859751 100644
> --- a/src/gallium/auxiliary/util/u_upload_mgr.c
> +++ b/src/gallium/auxiliary/util/u_upload_mgr.c
> @@ -167,7 +167,7 @@ enum pipe_error u_upload_alloc( struct u_upload_mgr *upload,
>       * sure the caller doesn't get garbage values.
>       */
>      *out_offset = ~0;
> -   *outbuf = NULL;
> +   pipe_resource_reference(outbuf, NULL);
>      *ptr = NULL;
>
>      /* Make sure we have enough space in the upload buffer
> @@ -189,7 +189,6 @@ enum pipe_error u_upload_alloc( struct u_upload_mgr *upload,
>   					  PIPE_TRANSFER_UNSYNCHRONIZED,
>   					&upload->transfer);
>         if (!upload->map) {
> -         pipe_resource_reference(outbuf, NULL);
>            upload->transfer = NULL;
>            return PIPE_ERROR_OUT_OF_MEMORY;
>         }
>

I'm not 100% sure this is right either.

When I was working on this last week, all the calls to 
u_upload_alloc() that I was looking at were setting the outbuf 
variable to NULL before the call.  But per r600g (and others?) that's 
not true.

With your change above, will it ever be the case that the incoming 
outbuf will have a refcount=1?  If so, it seems we'd be mistakenly 
deallocating the buffer at this point.

I _think_ that the refcount will always be >1 since it should be a 
pointer to the u_upload_mgr::buffer buffer.  But I haven't completely 
convinced myself of that.

The safest solution might be to restore the original code (before my 
patch) regarding outbuf.

-Brian