[Spice-devel] patch[1/1] fix a memory leak in qxl_screen_init

Submitted by bigclouds on Nov. 6, 2013, 9:37 a.m.

Details

Message ID 1f1861f5.24a1e.1422cc5c3c0.Coremail.bigclouds@163.com
State New
Headers show

Not browsing as part of any series.

Commit Message

bigclouds Nov. 6, 2013, 9:37 a.m.
hi, it allocate twice memory for qxl->uxa in function qxl_screen_init and qxl_uxa_init
-----------------

Patch hide | download patch | download mbox

diff --git a/src/qxl_driver.c b/src/qxl_driver.c
index 91ba6c2..6be61e4 100644
--- a/src/qxl_driver.c
+++ b/src/qxl_driver.c
@@ -746,7 +746,9 @@  qxl_screen_init (SCREEN_INIT_ARGS_DECL)
     }
     
     qxl->uxa = uxa_driver_alloc ();
-    
+    if (qxl->uxa == NULL)
+return FALSE;
+
 #ifndef XSPICE
     qxl->io_pages = (void *)((unsigned long)qxl->ram);
     qxl->io_pages_physical = (void *)((unsigned long)qxl->ram_physical);
diff --git a/src/qxl_uxa.c b/src/qxl_uxa.c
index 875d663..79dfe03 100644
--- a/src/qxl_uxa.c
+++ b/src/qxl_uxa.c
@@ -546,10 +546,6 @@  qxl_uxa_init (qxl_screen_t *qxl, ScreenPtr screen)
 return FALSE;
 #endif
 
-    qxl->uxa = uxa_driver_alloc ();
-    if (qxl->uxa == NULL)
-return FALSE;
-
     memset (qxl->uxa, 0, sizeof (*qxl->uxa));
 
     qxl->uxa->uxa_major = 1;

Comments

Nice catch!

On 11/06/2013 03:37 AM, bigclouds wrote:
> hi, it allocate twice memory for qxl->uxa in function qxl_screen_init and qxl_uxa_init
> -----------------
> diff --git a/src/qxl_driver.c b/src/qxl_driver.c
> index 91ba6c2..6be61e4 100644
> --- a/src/qxl_driver.c
> +++ b/src/qxl_driver.c
> @@ -746,7 +746,9 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL)
>       }
>       qxl->uxa = uxa_driver_alloc ();
> -
> +    if (qxl->uxa == NULL)
> +return FALSE;
> +

Wouldn't it be better to just delete this instance of the
allocation and leave it all in qxl_uxa.c?

Also, just a kibitz, but most open source projects require
a full name on a submitted patch.

Cheers,

Jeremy
it is needed to detect the return of qxl_uxa_init  in qxl_screen_init .








At 2013-11-06 21:59:58,"Jeremy White" <jwhite@codeweavers.com> wrote:
>Nice catch!
>
>On 11/06/2013 03:37 AM, bigclouds wrote:
>> hi, it allocate twice memory for qxl->uxa in function qxl_screen_init and qxl_uxa_init
>> -----------------
>> diff --git a/src/qxl_driver.c b/src/qxl_driver.c
>> index 91ba6c2..6be61e4 100644
>> --- a/src/qxl_driver.c
>> +++ b/src/qxl_driver.c
>> @@ -746,7 +746,9 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL)
>>       }
>>       qxl->uxa = uxa_driver_alloc ();
>> -
>> +    if (qxl->uxa == NULL)
>> +return FALSE;
>> +
>
>Wouldn't it be better to just delete this instance of the
>allocation and leave it all in qxl_uxa.c?
>
>Also, just a kibitz, but most open source projects require
>a full name on a submitted patch.
>
>Cheers,
>
>Jeremy
>_______________________________________________
>Spice-devel mailing list
>Spice-devel@lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/spice-devel
On 11/06/2013 08:48 AM, bigclouds wrote:
> it is needed to detect the return of qxl_uxa_init  in qxl_screen_init .

I don't think we have understood each other.

There is a duplicate allocation; you are trying to fix that.  As I
understand it, your patch removes the allocation in qxl_uxa_init and
leaves only the one in qxl_screen_init.

I said that it seems to me it would be better to remove the allocation
in qxl_screen_init and leave only the one in qxl_uxa_init.

If you're concerned about error conditions propagating, you can change
qxl_driver to check the return status of qxl_uxa_init.

Cheers,

Jeremy

> 
> 
> 
> 
> 
> 
> At 2013-11-06 21:59:58,"Jeremy White" <jwhite@codeweavers.com> wrote:
>>Nice catch!
>>
>>On 11/06/2013 03:37 AM, bigclouds wrote:
>>> hi, it allocate twice memory for qxl->uxa in function qxl_screen_init and qxl_uxa_init
>>> -----------------
>>> diff --git a/src/qxl_driver.c b/src/qxl_driver.c
>>> index 91ba6c2..6be61e4 100644
>>> --- a/src/qxl_driver.c
>>> +++ b/src/qxl_driver.c
>>> @@ -746,7 +746,9 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL)
>>>       }
>>>       qxl->uxa = uxa_driver_alloc ();
>>> -
>>> +    if (qxl->uxa == NULL)
>>> +return FALSE;
>>> +
>>
>>Wouldn't it be better to just delete this instance of the
>>allocation and leave it all in qxl_uxa.c?
>>
>>Also, just a kibitz, but most open source projects require
>>a full name on a submitted patch.
>>
>>Cheers,
>>
>>Jeremy
>>_______________________________________________
>>Spice-devel mailing list
>>Spice-devel@lists.freedesktop.org
>>http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 
>
ping..

"bigclouds",
are you willing to finish the patch and address Jeremy's comment? It's imo worthy to fix and It would be nice If you can finish that.

----- Original Message -----
> From: "Jeremy White" <jwhite@codeweavers.com>
> To: "bigclouds" <bigclouds@163.com>
> Cc: spice-devel@lists.freedesktop.org
> Sent: Wednesday, November 6, 2013 4:28:45 PM
> Subject: Re: [Spice-devel] patch[1/1] fix a memory leak  in qxl_screen_init
> 
> On 11/06/2013 08:48 AM, bigclouds wrote:
> > it is needed to detect the return of qxl_uxa_init  in qxl_screen_init .
> 
> I don't think we have understood each other.
> 
> There is a duplicate allocation; you are trying to fix that.  As I
> understand it, your patch removes the allocation in qxl_uxa_init and
> leaves only the one in qxl_screen_init.
> 
> I said that it seems to me it would be better to remove the allocation
> in qxl_screen_init and leave only the one in qxl_uxa_init.
> 
> If you're concerned about error conditions propagating, you can change
> qxl_driver to check the return status of qxl_uxa_init.
> 
> Cheers,
> 
> Jeremy
> 
> > 
> > 
> > 
> > 
> > 
> > 
> > At 2013-11-06 21:59:58,"Jeremy White" <jwhite@codeweavers.com> wrote:
> >>Nice catch!
> >>
> >>On 11/06/2013 03:37 AM, bigclouds wrote:
> >>> hi, it allocate twice memory for qxl->uxa in function qxl_screen_init and
> >>> qxl_uxa_init
> >>> -----------------
> >>> diff --git a/src/qxl_driver.c b/src/qxl_driver.c
> >>> index 91ba6c2..6be61e4 100644
> >>> --- a/src/qxl_driver.c
> >>> +++ b/src/qxl_driver.c
> >>> @@ -746,7 +746,9 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL)
> >>>       }
> >>>       qxl->uxa = uxa_driver_alloc ();
> >>> -
> >>> +    if (qxl->uxa == NULL)
> >>> +return FALSE;
> >>> +
> >>
> >>Wouldn't it be better to just delete this instance of the
> >>allocation and leave it all in qxl_uxa.c?
> >>
> >>Also, just a kibitz, but most open source projects require
> >>a full name on a submitted patch.
> >>
> >>Cheers,
> >>
> >>Jeremy
> >>_______________________________________________
> >>Spice-devel mailing list
> >>Spice-devel@lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> > 
> > 
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
please forgive my ignore.

jwhite you are right, i post a new patch as you suggest.
 
thanks







At 2013-11-19 20:21:35,"Marian Krcmarik" <mkrcmari@redhat.com> wrote:
>ping..
>
>"bigclouds",
>are you willing to finish the patch and address Jeremy's comment? It's imo worthy to fix and It would be nice If you can finish that.
>
>----- Original Message -----
>> From: "Jeremy White" <jwhite@codeweavers.com>
>> To: "bigclouds" <bigclouds@163.com>
>> Cc: spice-devel@lists.freedesktop.org
>> Sent: Wednesday, November 6, 2013 4:28:45 PM
>> Subject: Re: [Spice-devel] patch[1/1] fix a memory leak  in qxl_screen_init
>> 
>> On 11/06/2013 08:48 AM, bigclouds wrote:
>> > it is needed to detect the return of qxl_uxa_init  in qxl_screen_init .
>> 
>> I don't think we have understood each other.
>> 
>> There is a duplicate allocation; you are trying to fix that.  As I
>> understand it, your patch removes the allocation in qxl_uxa_init and
>> leaves only the one in qxl_screen_init.
>> 
>> I said that it seems to me it would be better to remove the allocation
>> in qxl_screen_init and leave only the one in qxl_uxa_init.
>> 
>> If you're concerned about error conditions propagating, you can change
>> qxl_driver to check the return status of qxl_uxa_init.
>> 
>> Cheers,
>> 
>> Jeremy
>> 
>> > 
>> > 
>> > 
>> > 
>> > 
>> > 
>> > At 2013-11-06 21:59:58,"Jeremy White" <jwhite@codeweavers.com> wrote:
>> >>Nice catch!
>> >>
>> >>On 11/06/2013 03:37 AM, bigclouds wrote:
>> >>> hi, it allocate twice memory for qxl->uxa in function qxl_screen_init and
>> >>> qxl_uxa_init
>> >>> -----------------
>> >>> diff --git a/src/qxl_driver.c b/src/qxl_driver.c
>> >>> index 91ba6c2..6be61e4 100644
>> >>> --- a/src/qxl_driver.c
>> >>> +++ b/src/qxl_driver.c
>> >>> @@ -746,7 +746,9 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL)
>> >>>       }
>> >>>       qxl->uxa = uxa_driver_alloc ();
>> >>> -
>> >>> +    if (qxl->uxa == NULL)
>> >>> +return FALSE;
>> >>> +
>> >>
>> >>Wouldn't it be better to just delete this instance of the
>> >>allocation and leave it all in qxl_uxa.c?
>> >>
>> >>Also, just a kibitz, but most open source projects require
>> >>a full name on a submitted patch.
>> >>
>> >>Cheers,
>> >>
>> >>Jeremy
>> >>_______________________________________________
>> >>Spice-devel mailing list
>> >>Spice-devel@lists.freedesktop.org
>> >>http://lists.freedesktop.org/mailman/listinfo/spice-devel
>> > 
>> > 
>> > 
>> 
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
please forgive my ignore.

jwhite you are right, i post a new patch as you suggest.
 
thanks








At 2013-11-19 20:21:35,"Marian Krcmarik" <mkrcmari@redhat.com> wrote:
>ping..
>
>"bigclouds",
>are you willing to finish the patch and address Jeremy's comment? It's imo worthy to fix and It would be nice If you can finish that.
>
>----- Original Message -----
>> From: "Jeremy White" <jwhite@codeweavers.com>
>> To: "bigclouds" <bigclouds@163.com>
>> Cc: spice-devel@lists.freedesktop.org
>> Sent: Wednesday, November 6, 2013 4:28:45 PM
>> Subject: Re: [Spice-devel] patch[1/1] fix a memory leak  in qxl_screen_init
>> 
>> On 11/06/2013 08:48 AM, bigclouds wrote:
>> > it is needed to detect the return of qxl_uxa_init  in qxl_screen_init .
>> 
>> I don't think we have understood each other.
>> 
>> There is a duplicate allocation; you are trying to fix that.  As I
>> understand it, your patch removes the allocation in qxl_uxa_init and
>> leaves only the one in qxl_screen_init.
>> 
>> I said that it seems to me it would be better to remove the allocation
>> in qxl_screen_init and leave only the one in qxl_uxa_init.
>> 
>> If you're concerned about error conditions propagating, you can change
>> qxl_driver to check the return status of qxl_uxa_init.
>> 
>> Cheers,
>> 
>> Jeremy
>> 
>> > 
>> > 
>> > 
>> > 
>> > 
>> > 
>> > At 2013-11-06 21:59:58,"Jeremy White" <jwhite@codeweavers.com> wrote:
>> >>Nice catch!
>> >>
>> >>On 11/06/2013 03:37 AM, bigclouds wrote:
>> >>> hi, it allocate twice memory for qxl->uxa in function qxl_screen_init and
>> >>> qxl_uxa_init
>> >>> -----------------
>> >>> diff --git a/src/qxl_driver.c b/src/qxl_driver.c
>> >>> index 91ba6c2..6be61e4 100644
>> >>> --- a/src/qxl_driver.c
>> >>> +++ b/src/qxl_driver.c
>> >>> @@ -746,7 +746,9 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL)
>> >>>       }
>> >>>       qxl->uxa = uxa_driver_alloc ();
>> >>> -
>> >>> +    if (qxl->uxa == NULL)
>> >>> +return FALSE;
>> >>> +
>> >>
>> >>Wouldn't it be better to just delete this instance of the
>> >>allocation and leave it all in qxl_uxa.c?
>> >>
>> >>Also, just a kibitz, but most open source projects require
>> >>a full name on a submitted patch.
>> >>
>> >>Cheers,
>> >>
>> >>Jeremy
>> >>_______________________________________________
>> >>Spice-devel mailing list
>> >>Spice-devel@lists.freedesktop.org
>> >>http://lists.freedesktop.org/mailman/listinfo/spice-devel
>> > 
>> > 
>> > 
>> 
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
On 03/26/2014 11:30 AM, longguang.yue wrote:
> please forgive my ignore.
>
> jwhite you are right, i post a new patch as you suggest.

Yes, this patch does look correct.  I have been meaning to get time to 
put it through some further testing, and then push it.  I hope to do 
that this week.

Cheers,

Jeremy
On 03/26/2014 11:30 AM, longguang.yue wrote:
> please forgive my ignore.
>
> jwhite you are right, i post a new patch as you suggest.

ACKed

(and pushed)

Thanks!

Jeremy

> thanks
>
>
>
>
>
>
> At 2013-11-19 20:21:35,"Marian Krcmarik" <mkrcmari@redhat.com> wrote:
>>ping..
>>
>>"bigclouds",
>>are you willing to finish the patch and address Jeremy's comment? It's imo worthy to fix and It would be nice If you can finish that.
>>
>>----- Original Message -----
>>> From: "Jeremy White" <jwhite@codeweavers.com>
>>> To: "bigclouds" <bigclouds@163.com>
>>> Cc: spice-devel@lists.freedesktop.org
>>> Sent: Wednesday, November 6, 2013 4:28:45 PM
>>> Subject: Re: [Spice-devel] patch[1/1] fix a memory leak  in qxl_screen_init
>>>
>>> On 11/06/2013 08:48 AM, bigclouds wrote:
>>> > it is needed to detect the return of qxl_uxa_init  in qxl_screen_init .
>>>
>>> I don't think we have understood each other.
>>>
>>> There is a duplicate allocation; you are trying to fix that.  As I
>>> understand it, your patch removes the allocation in qxl_uxa_init and
>>> leaves only the one in qxl_screen_init.
>>>
>>> I said that it seems to me it would be better to remove the allocation
>>> in qxl_screen_init and leave only the one in qxl_uxa_init.
>>>
>>> If you're concerned about error conditions propagating, you can change
>>> qxl_driver to check the return status of qxl_uxa_init.
>>>
>>> Cheers,
>>>
>>> Jeremy
>>>
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> > At 2013-11-06 21:59:58,"Jeremy White" <jwhite@codeweavers.com> wrote:
>>> >>Nice catch!
>>> >>
>>> >>On 11/06/2013 03:37 AM, bigclouds wrote:
>>> >>> hi, it allocate twice memory for qxl->uxa in function qxl_screen_init and
>>> >>> qxl_uxa_init
>>> >>> -----------------
>>> >>> diff --git a/src/qxl_driver.c b/src/qxl_driver.c
>>> >>> index 91ba6c2..6be61e4 100644
>>> >>> --- a/src/qxl_driver.c
>>> >>> +++ b/src/qxl_driver.c
>>> >>> @@ -746,7 +746,9 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL)
>>> >>>       }
>>> >>>       qxl->uxa = uxa_driver_alloc ();
>>> >>> -
>>> >>> +    if (qxl->uxa == NULL)
>>> >>> +return FALSE;
>>> >>> +
>>> >>
>>> >>Wouldn't it be better to just delete this instance of the
>>> >>allocation and leave it all in qxl_uxa.c?
>>> >>
>>> >>Also, just a kibitz, but most open source projects require
>>> >>a full name on a submitted patch.
>>> >>
>>> >>Cheers,
>>> >>
>>> >>Jeremy
>>> >>_______________________________________________
>>> >>Spice-devel mailing list
>>> >>Spice-devel@lists.freedesktop.org
>>> >>http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>> >
>>> >
>>> >
>>>
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>
>
>
>