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

[Marek Olšák]

On Mon, Jan 28, 2013 at 6:18 PM, Brian Paul <brianp at vmware.com> wrote:
> 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.

u_upload_data also calls u_upload_alloc, so all calls to u_upload_data
should be checked as well.

>
> 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.

No, it's alright. u_upload_alloc shouldn't usually fail and outbuf is
always unreferenced later in the code. That has always been the
expected behavior. r600g sets outbuf to a member of r600_context,
which can be non-NULL, and it expects the buffer to be unreferenced in
u_upload_data.

The original code might have lower overhead in the case of no error though.

Marek