Crash in X server [with Patch]

[Alex Deucher]

On Fri, Oct 24, 2014 at 3:02 PM, Keith Packard <keithp at keithp.com> wrote:
> Michel Dänzer <michel at daenzer.net> writes:
>
>> On 24.10.2014 07:09, Andreas Hartmetz wrote:
>>>
>>> commit bbcc084afc1396d3df42516baafea5839c95a488
>>> Author: Andreas Hartmetz <ahartmetz at gmail.com>
>>> Date:   Sat Oct 4 18:13:04 2014 +0200
>>>
>>>      glamor: Don't free memory we are going to use.
>>>
>>>      glamor_color_convert_to_bits() returns its second argument on
>>>      success, NULL on error, and need_free_bits already makes sure that
>>>      "bits" aliasing converted_bits is freed in the success case.
>>>      Looks like the memory leak that was supposed to be fixed in
>>>      6e50bfa706cd3ab884c933bf1f17c221a6208aa4 only occurred in the error
>>>      case.
>>>
>>> diff --git a/glamor/glamor_pixmap.c b/glamor/glamor_pixmap.c
>>> index 355fe4b..7c9bf26 100644
>>> --- a/glamor/glamor_pixmap.c
>>> +++ b/glamor/glamor_pixmap.c
>>> @@ -774,8 +774,8 @@ _glamor_upload_bits_to_pixmap_texture(PixmapPtr
>>> pixmap, GLenum format,
>>>               return FALSE;
>>>           bits = glamor_color_convert_to_bits(bits, converted_bits, w, h,
>>>                                               stride, no_alpha, revert,
>>> swap_rb);
>>> -        free(converted_bits);
>>>           if (bits == NULL) {
>>> +            free(converted_bits);
>>>               ErrorF("Failed to convert pixmap no_alpha %d,"
>>>                      "revert mode %d, swap mode %d\n", no_alpha, revert,
>>> swap_rb);
>>>               return FALSE;
>>>
>>> I've already, a week or two ago, sent this to Michel a who reviewed it
>>> (no idea how to forward that, guess I'll need another review from
>>> whoever volunteers) and OK'd it.
>>
>> You could have just submitted the patch with my
>>
>> Reviewed-by: Michel Dänzer <michel.daenzer at amd.com>
>>
>> to this list and to Keith Packard (Cc'd, though that probably won't be
>> visible in my post as distributed by the list), as I suggested. :)
>
> Looks like there's a path which will leak these bits still -- the fast
> path doesn't reach the call to free the bits. Can either of you review
> this patch and I'll merge both in?

Makes sense to me.

Reviewed-by: Alex Deucher <alexander.deucher at amd.com>