Patchwork vnc_refresh: calling vnc_update_client might free vs

login
register
mail settings
Submitter Stefano Stabellini
Date Jan. 25, 2010, 12:54 p.m.
Message ID <alpine.DEB.2.00.1001251210570.7512@kaball-desktop>
Download mbox | patch
Permalink /patch/43623/
State New
Headers show

Comments

Stefano Stabellini - Jan. 25, 2010, 12:54 p.m.
Hi all,
this patch fixes another bug in vnc_refresh: calling vnc_update_client
might cause vs to be free()ed, in this case we cannot access vs->next
right after to examine the next item on the list.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---
Anthony Liguori - Jan. 27, 2010, 12:07 a.m.
On 01/25/2010 06:54 AM, Stefano Stabellini wrote:
> Hi all,
> this patch fixes another bug in vnc_refresh: calling vnc_update_client
> might cause vs to be free()ed, in this case we cannot access vs->next
> right after to examine the next item on the list.
>
> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>
> diff --git a/vnc.c b/vnc.c
> index cc2a26e..92facde 100644
> --- a/vnc.c
> +++ b/vnc.c
> @@ -2345,7 +2345,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>   static void vnc_refresh(void *opaque)
>   {
>       VncDisplay *vd = opaque;
> -    VncState *vs = NULL;
> +    VncState *vs = NULL, *vn = NULL;
>       int has_dirty = 0, rects = 0;
>
>       vga_hw_update();
> @@ -2354,8 +2354,10 @@ static void vnc_refresh(void *opaque)
>
>       vs = vd->clients;
>       while (vs != NULL) {
> +        vn = vs->next;
>           rects += vnc_update_client(vs, has_dirty);
> -        vs = vs->next;
> +        /* vs might be free()ed here */
> +        vs = vn;
>       }
>       /* vd->timer could be NULL now if the last client disconnected,
>        * in this case don't update the timer */
>
>
>
>
Gerd Hoffmann - Jan. 27, 2010, 9:53 a.m.
On 01/27/10 01:07, Anthony Liguori wrote:
> On 01/25/2010 06:54 AM, Stefano Stabellini wrote:
>> Hi all,
>> this patch fixes another bug in vnc_refresh: calling vnc_update_client
>> might cause vs to be free()ed, in this case we cannot access vs->next
>> right after to examine the next item on the list.
>>
>> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>
> Applied. Thanks.

IMHO this should go to stable too.

cheers,
   Gerd
Stefano Stabellini - Jan. 27, 2010, 12:29 p.m.
On Wed, 27 Jan 2010, Gerd Hoffmann wrote:
> On 01/27/10 01:07, Anthony Liguori wrote:
> > On 01/25/2010 06:54 AM, Stefano Stabellini wrote:
> >> Hi all,
> >> this patch fixes another bug in vnc_refresh: calling vnc_update_client
> >> might cause vs to be free()ed, in this case we cannot access vs->next
> >> right after to examine the next item on the list.
> >>
> >> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> >
> > Applied. Thanks.
> 
> IMHO this should go to stable too.
> 

Yes, good idea.
Anthony Liguori - Jan. 27, 2010, 2:08 p.m.
On 01/27/2010 06:29 AM, Stefano Stabellini wrote:
> On Wed, 27 Jan 2010, Gerd Hoffmann wrote:
>    
>> On 01/27/10 01:07, Anthony Liguori wrote:
>>      
>>> On 01/25/2010 06:54 AM, Stefano Stabellini wrote:
>>>        
>>>> Hi all,
>>>> this patch fixes another bug in vnc_refresh: calling vnc_update_client
>>>> might cause vs to be free()ed, in this case we cannot access vs->next
>>>> right after to examine the next item on the list.
>>>>
>>>> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>>          
>>> Applied. Thanks.
>>>        
>> IMHO this should go to stable too.
>>
>>      
> Yes, good idea.
>    

Already did.

Regards,

Anthony Liguori

Patch

diff --git a/vnc.c b/vnc.c
index cc2a26e..92facde 100644
--- a/vnc.c
+++ b/vnc.c
@@ -2345,7 +2345,7 @@  static int vnc_refresh_server_surface(VncDisplay *vd)
 static void vnc_refresh(void *opaque)
 {
     VncDisplay *vd = opaque;
-    VncState *vs = NULL;
+    VncState *vs = NULL, *vn = NULL;
     int has_dirty = 0, rects = 0;
 
     vga_hw_update();
@@ -2354,8 +2354,10 @@  static void vnc_refresh(void *opaque)
 
     vs = vd->clients;
     while (vs != NULL) {
+        vn = vs->next;
         rects += vnc_update_client(vs, has_dirty);
-        vs = vs->next;
+        /* vs might be free()ed here */
+        vs = vn;
     }
     /* vd->timer could be NULL now if the last client disconnected,
      * in this case don't update the timer */