[Qemu-devel] [PATCH] vnc: Fix qemu crash on vnc client disconnection

[Qemu-devel] [PATCH] vnc: Fix qemu crash on vnc client disconnection

[Gonglei (Arei)]

[-- Attachment #1: Type: text/plain, Size: 6164 bytes --]

Hi,

I encount a qemu crash when the vnc client disconnection, and I got the next log:

qemu: qemu_mutex_lock: Invalid argument

and the backtrace listed:

Core was generated by `/mnt/sdd/gonglei/kvm/qemu-unstable/x86_64-softmmu/qemu-system-x86_64 -name suse'.
Program terminated with signal 6, Aborted.
#0  0x00007fab8498ed95 in raise () from /lib64/libc.so.6
(gdb) bt
#0  0x00007fab8498ed95 in raise () from /lib64/libc.so.6
#1  0x00007fab849902ab in abort () from /lib64/libc.so.6
#2  0x00007fab87c22915 in error_exit (err=22, msg=0x7fab87c97a70 <__func__.4762> "qemu_mutex_lock") at util/qemu-thread-posix.c:28
#3  0x00007fab87c22a19 in qemu_mutex_lock (mutex=0x7fab8858e688) at util/qemu-thread-posix.c:59
#4  0x00007fab87ae52ea in vnc_lock_output (vs=0x7fab885823f0) at ui/vnc-jobs.h:63
#5  0x00007fab87ae5217 in vnc_jobs_consume_buffer (vs=0x7fab885823f0) at ui/vnc-jobs.c:166
#6  0x00007fab87ae51dd in vnc_jobs_join (vs=0x7fab885823f0) at ui/vnc-jobs.c:159
#7  0x00007fab87aea776 in vnc_update_client_sync (vs=0x7fab885823f0, has_dirty=1) at ui/vnc.c:880
#8  0x00007fab87aea007 in vnc_dpy_copy (dcl=0x7fab8088f048, src_x=746, src_y=578, dst_x=772, dst_y=578, w=22, h=22) at ui/vnc.c:753
#9  0x00007fab87ac8df6 in dpy_gfx_copy (con=0x7fab885cdb40, src_x=746, src_y=578, dst_x=772, dst_y=578, w=22, h=22)
    at ui/console.c:1455
#10 0x00007fab87ac9fd7 in qemu_console_copy (con=0x7fab885cdb40, src_x=746, src_y=578, dst_x=772, dst_y=578, w=22, h=22)
    at ui/console.c:1837
#11 0x00007fab8799bd91 in cirrus_do_copy (s=0x7fab885ff450, dst=1228339, src=1228287, w=22, h=22) at hw/display/cirrus_vga.c:738
#12 0x00007fab8799bf03 in cirrus_bitblt_videotovideo_copy (s=0x7fab885ff450) at hw/display/cirrus_vga.c:757
#13 0x00007fab8799c48c in cirrus_bitblt_videotovideo (s=0x7fab885ff450) at hw/display/cirrus_vga.c:879
#14 0x00007fab8799cc00 in cirrus_bitblt_start (s=0x7fab885ff450) at hw/display/cirrus_vga.c:1020
#15 0x00007fab8799cfbb in cirrus_write_bitblt (s=0x7fab885ff450, reg_value=2) at hw/display/cirrus_vga.c:1041
#16 0x00007fab8799dedb in cirrus_vga_write_gr (s=0x7fab885ff450, reg_index=49, reg_value=2) at hw/display/cirrus_vga.c:1536
#17 0x00007fab8799e721 in cirrus_mmio_blt_write (s=0x7fab885ff450, address=64, value=2 '\002') at hw/display/cirrus_vga.c:1890
#18 0x00007fab879a068d in cirrus_mmio_write (opaque=0x7fab885ff450, addr=320, val=2, size=1) at hw/display/cirrus_vga.c:2670
#19 0x00007fab87b77921 in memory_region_write_accessor (mr=0x7fab8860fe90, addr=320, value=0x7fab818d5cc8, size=1, shift=0, mask=
    255) at /mnt/sdd/gonglei/kvm/qemu-unstable/memory.c:440
#20 0x00007fab87b77a5d in access_with_adjusted_size (addr=320, value=0x7fab818d5cc8, size=4, access_size_min=1, access_size_max=1, 
    access=0x7fab87b77898 <memory_region_write_accessor>, mr=0x7fab8860fe90) at /mnt/sdd/gonglei/kvm/qemu-unstable/memory.c:477
#21 0x00007fab87b7a8c0 in memory_region_dispatch_write (mr=0x7fab8860fe90, addr=320, data=18446744073709551362, size=4)
    at /mnt/sdd/gonglei/kvm/qemu-unstable/memory.c:984
#22 0x00007fab87b7e176 in io_mem_write (mr=0x7fab8860fe90, addr=320, val=18446744073709551362, size=4)
    at /mnt/sdd/gonglei/kvm/qemu-unstable/memory.c:1748
#23 0x00007fab87b0e91e in address_space_rw (as=0x7fab8848b960 <address_space_memory>, addr=4273832256, buf=
    0x7fab87830028 <Address 0x7fab87830028 out of bounds>, len=4, is_write=true) at /mnt/sdd/gonglei/kvm/qemu-unstable/exec.c:1963
#24 0x00007fab87b0eec0 in cpu_physical_memory_rw (addr=4273832256, buf=0x7fab87830028 <Address 0x7fab87830028 out of bounds>, len=
    4, is_write=1) at /mnt/sdd/gonglei/kvm/qemu-unstable/exec.c:2042
#25 0x00007fab87b74b47 in kvm_cpu_exec (cpu=0x7fab88520c20) at /mnt/sdd/gonglei/kvm/qemu-unstable/kvm-all.c:1673
#26 0x00007fab87b022e9 in qemu_kvm_cpu_thread_fn (arg=0x7fab88520c20) at /mnt/sdd/gonglei/kvm/qemu-unstable/cpus.c:785
#27 0x00007fab85b07f05 in start_thread () from /lib64/libpthread.so.0
#28 0x00007fab84a3353d in clone () from /lib64/libc.so.6

When Vnc client was disconnected, the vs->csock will be set to -1 in function vnc_disconnect_start. And on the next loop, in case of the function transfer:
vnc_dpy_copy-->vnc_update_client_sync-->vnc_update_client-->vnc_disconnect_finish(vs)
and
vnc_dpy_copy-->vnc_update_client_sync--> vnc_jobs_consume_buffer-->vnc_lock_output(vs)--> qemu_mutex_lock(&vs->output_mutex);
because the vs has been freed, the qemu_mutex_lock(&vs->output_mutex) will cause qemu abort.

The patch fixed the bug:

when the vs object be freed, function vnc_update_client return -1,
and vnc_update_client_sync do not deal with the situation avoid that
qemu abort.
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 ui/vnc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 5601cc3..2177704 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -876,7 +876,8 @@ static int find_and_clear_dirty_height(struct VncState *vs,
 static int vnc_update_client_sync(VncState *vs, int has_dirty)
 {
     int ret = vnc_update_client(vs, has_dirty);
-    vnc_jobs_join(vs);
+    if (ret >= 0)
+        vnc_jobs_join(vs);
     return ret;
 }
 
@@ -939,8 +940,10 @@ static int vnc_update_client(VncState *vs, int has_dirty)
         return n;
     }
 
-    if (vs->csock == -1)
+    if (vs->csock == -1) {
         vnc_disconnect_finish(vs);
+	 return -1;
+    } 
 
     return 0;
 }
@@ -2737,6 +2740,7 @@ static void vnc_refresh(DisplayChangeListener *dcl)
     VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
     VncState *vs, *vn;
     int has_dirty, rects = 0;
+    int ret;
 
     graphic_hw_update(NULL);
 
@@ -2749,8 +2753,10 @@ static void vnc_refresh(DisplayChangeListener *dcl)
     vnc_unlock_display(vd);
 
     QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
-        rects += vnc_update_client(vs, has_dirty);
+        ret = vnc_update_client(vs, has_dirty);
         /* vs might be free()ed here */
+        if (ret >= 0)
+	    rects += ret;
     }
 
     if (QTAILQ_EMPTY(&vd->clients)) {
-- 
1.8.3.4

Best regards,
-Gonglei


[-- Attachment #2: 0001-vnc-Fix-qemu-crash-on-vnc-client-disconnection.patch --]
[-- Type: application/octet-stream, Size: 1758 bytes --]

From 56763ddef53fb12b8fabb08bc41429c2df67940b Mon Sep 17 00:00:00 2001
From: Gonglei <arei.gonglei@huawei.com>
Date: Thu, 24 Oct 2013 12:05:29 +0800
Subject: [PATCH] vnc: Fix qemu crash on vnc client disconnection

when the vs object be freed, function vnc_update_client return -1,
and vnc_update_client_sync do not deal with the situation avoid that
qemu abort.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 ui/vnc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 5601cc3..2177704 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -876,7 +876,8 @@ static int find_and_clear_dirty_height(struct VncState *vs,
 static int vnc_update_client_sync(VncState *vs, int has_dirty)
 {
     int ret = vnc_update_client(vs, has_dirty);
-    vnc_jobs_join(vs);
+    if (ret >= 0)
+        vnc_jobs_join(vs);
     return ret;
 }
 
@@ -939,8 +940,10 @@ static int vnc_update_client(VncState *vs, int has_dirty)
         return n;
     }
 
-    if (vs->csock == -1)
+    if (vs->csock == -1) {
         vnc_disconnect_finish(vs);
+	 return -1;
+    } 
 
     return 0;
 }
@@ -2737,6 +2740,7 @@ static void vnc_refresh(DisplayChangeListener *dcl)
     VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
     VncState *vs, *vn;
     int has_dirty, rects = 0;
+    int ret;
 
     graphic_hw_update(NULL);
 
@@ -2749,8 +2753,10 @@ static void vnc_refresh(DisplayChangeListener *dcl)
     vnc_unlock_display(vd);
 
     QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
-        rects += vnc_update_client(vs, has_dirty);
+        ret = vnc_update_client(vs, has_dirty);
         /* vs might be free()ed here */
+        if (ret >= 0)
+	    rects += ret;
     }
 
     if (QTAILQ_EMPTY(&vd->clients)) {
-- 
1.8.3.4

Re: [Qemu-devel] [PATCH] vnc: Fix qemu crash on vnc client disconnection

[Gonglei (Arei)]

Hi,

Any comments will be welcome.

Best regards,
-Gonglei

> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Thursday, October 24, 2013 1:14 PM
> To: qemu-devel@nongnu.org; 'Gerd Hoffmann'; 'Stefan Hajnoczi'
> Cc: Luonengjun; Huangweidong (Hardware); Yanqiangjun
> Subject: [PATCH] vnc: Fix qemu crash on vnc client disconnection
> 
> Hi,
> 
> I encount a qemu crash when the vnc client disconnection, and I got the next
> log:
> 
> qemu: qemu_mutex_lock: Invalid argument
> 
> and the backtrace listed:
> 
> Core was generated by
> `/mnt/sdd/gonglei/kvm/qemu-unstable/x86_64-softmmu/qemu-system-x86_64
> -name suse'.
> Program terminated with signal 6, Aborted.
> #0  0x00007fab8498ed95 in raise () from /lib64/libc.so.6
> (gdb) bt
> #0  0x00007fab8498ed95 in raise () from /lib64/libc.so.6
> #1  0x00007fab849902ab in abort () from /lib64/libc.so.6
> #2  0x00007fab87c22915 in error_exit (err=22, msg=0x7fab87c97a70
> <__func__.4762> "qemu_mutex_lock") at util/qemu-thread-posix.c:28
> #3  0x00007fab87c22a19 in qemu_mutex_lock (mutex=0x7fab8858e688) at
> util/qemu-thread-posix.c:59
> #4  0x00007fab87ae52ea in vnc_lock_output (vs=0x7fab885823f0) at
> ui/vnc-jobs.h:63
> #5  0x00007fab87ae5217 in vnc_jobs_consume_buffer (vs=0x7fab885823f0)
> at ui/vnc-jobs.c:166
> #6  0x00007fab87ae51dd in vnc_jobs_join (vs=0x7fab885823f0) at
> ui/vnc-jobs.c:159
> #7  0x00007fab87aea776 in vnc_update_client_sync (vs=0x7fab885823f0,
> has_dirty=1) at ui/vnc.c:880
> #8  0x00007fab87aea007 in vnc_dpy_copy (dcl=0x7fab8088f048, src_x=746,
> src_y=578, dst_x=772, dst_y=578, w=22, h=22) at ui/vnc.c:753
> #9  0x00007fab87ac8df6 in dpy_gfx_copy (con=0x7fab885cdb40, src_x=746,
> src_y=578, dst_x=772, dst_y=578, w=22, h=22)
>     at ui/console.c:1455
> #10 0x00007fab87ac9fd7 in qemu_console_copy (con=0x7fab885cdb40,
> src_x=746, src_y=578, dst_x=772, dst_y=578, w=22, h=22)
>     at ui/console.c:1837
> #11 0x00007fab8799bd91 in cirrus_do_copy (s=0x7fab885ff450, dst=1228339,
> src=1228287, w=22, h=22) at hw/display/cirrus_vga.c:738
> #12 0x00007fab8799bf03 in cirrus_bitblt_videotovideo_copy (s=0x7fab885ff450)
> at hw/display/cirrus_vga.c:757
> #13 0x00007fab8799c48c in cirrus_bitblt_videotovideo (s=0x7fab885ff450) at
> hw/display/cirrus_vga.c:879
> #14 0x00007fab8799cc00 in cirrus_bitblt_start (s=0x7fab885ff450) at
> hw/display/cirrus_vga.c:1020
> #15 0x00007fab8799cfbb in cirrus_write_bitblt (s=0x7fab885ff450, reg_value=2)
> at hw/display/cirrus_vga.c:1041
> #16 0x00007fab8799dedb in cirrus_vga_write_gr (s=0x7fab885ff450,
> reg_index=49, reg_value=2) at hw/display/cirrus_vga.c:1536
> #17 0x00007fab8799e721 in cirrus_mmio_blt_write (s=0x7fab885ff450,
> address=64, value=2 '\002') at hw/display/cirrus_vga.c:1890
> #18 0x00007fab879a068d in cirrus_mmio_write (opaque=0x7fab885ff450,
> addr=320, val=2, size=1) at hw/display/cirrus_vga.c:2670
> #19 0x00007fab87b77921 in memory_region_write_accessor
> (mr=0x7fab8860fe90, addr=320, value=0x7fab818d5cc8, size=1, shift=0, mask=
>     255) at /mnt/sdd/gonglei/kvm/qemu-unstable/memory.c:440
> #20 0x00007fab87b77a5d in access_with_adjusted_size (addr=320,
> value=0x7fab818d5cc8, size=4, access_size_min=1, access_size_max=1,
>     access=0x7fab87b77898 <memory_region_write_accessor>,
> mr=0x7fab8860fe90) at /mnt/sdd/gonglei/kvm/qemu-unstable/memory.c:477
> #21 0x00007fab87b7a8c0 in memory_region_dispatch_write
> (mr=0x7fab8860fe90, addr=320, data=18446744073709551362, size=4)
>     at /mnt/sdd/gonglei/kvm/qemu-unstable/memory.c:984
> #22 0x00007fab87b7e176 in io_mem_write (mr=0x7fab8860fe90, addr=320,
> val=18446744073709551362, size=4)
>     at /mnt/sdd/gonglei/kvm/qemu-unstable/memory.c:1748
> #23 0x00007fab87b0e91e in address_space_rw (as=0x7fab8848b960
> <address_space_memory>, addr=4273832256, buf=
>     0x7fab87830028 <Address 0x7fab87830028 out of bounds>, len=4,
> is_write=true) at /mnt/sdd/gonglei/kvm/qemu-unstable/exec.c:1963
> #24 0x00007fab87b0eec0 in cpu_physical_memory_rw (addr=4273832256,
> buf=0x7fab87830028 <Address 0x7fab87830028 out of bounds>, len=
>     4, is_write=1) at /mnt/sdd/gonglei/kvm/qemu-unstable/exec.c:2042
> #25 0x00007fab87b74b47 in kvm_cpu_exec (cpu=0x7fab88520c20) at
> /mnt/sdd/gonglei/kvm/qemu-unstable/kvm-all.c:1673
> #26 0x00007fab87b022e9 in qemu_kvm_cpu_thread_fn (arg=0x7fab88520c20)
> at /mnt/sdd/gonglei/kvm/qemu-unstable/cpus.c:785
> #27 0x00007fab85b07f05 in start_thread () from /lib64/libpthread.so.0
> #28 0x00007fab84a3353d in clone () from /lib64/libc.so.6
> 
> When Vnc client was disconnected, the vs->csock will be set to -1 in function
> vnc_disconnect_start. And on the next loop, in case of the function transfer:
> vnc_dpy_copy-->vnc_update_client_sync-->vnc_update_client-->vnc_disconnec
> t_finish(vs)
> and
> vnc_dpy_copy-->vnc_update_client_sync-->
> vnc_jobs_consume_buffer-->vnc_lock_output(vs)-->
> qemu_mutex_lock(&vs->output_mutex);
> because the vs has been freed, the qemu_mutex_lock(&vs->output_mutex) will
> cause qemu abort.
> 
> The patch fixed the bug:
> 
> when the vs object be freed, function vnc_update_client return -1,
> and vnc_update_client_sync do not deal with the situation avoid that
> qemu abort.
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  ui/vnc.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 5601cc3..2177704 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -876,7 +876,8 @@ static int find_and_clear_dirty_height(struct VncState
> *vs,
>  static int vnc_update_client_sync(VncState *vs, int has_dirty)
>  {
>      int ret = vnc_update_client(vs, has_dirty);
> -    vnc_jobs_join(vs);
> +    if (ret >= 0)
> +        vnc_jobs_join(vs);
>      return ret;
>  }
> 
> @@ -939,8 +940,10 @@ static int vnc_update_client(VncState *vs, int
> has_dirty)
>          return n;
>      }
> 
> -    if (vs->csock == -1)
> +    if (vs->csock == -1) {
>          vnc_disconnect_finish(vs);
> +	 return -1;
> +    }
> 
>      return 0;
>  }
> @@ -2737,6 +2740,7 @@ static void vnc_refresh(DisplayChangeListener *dcl)
>      VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
>      VncState *vs, *vn;
>      int has_dirty, rects = 0;
> +    int ret;
> 
>      graphic_hw_update(NULL);
> 
> @@ -2749,8 +2753,10 @@ static void vnc_refresh(DisplayChangeListener
> *dcl)
>      vnc_unlock_display(vd);
> 
>      QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
> -        rects += vnc_update_client(vs, has_dirty);
> +        ret = vnc_update_client(vs, has_dirty);
>          /* vs might be free()ed here */
> +        if (ret >= 0)
> +	    rects += ret;
>      }
> 
>      if (QTAILQ_EMPTY(&vd->clients)) {
> --
> 1.8.3.4
> 
> Best regards,
> -Gonglei

Re: [Qemu-devel] [PATCH] vnc: Fix qemu crash on vnc client disconnection

[Gerd Hoffmann]

  Hi,

> diff --git a/ui/vnc.c b/ui/vnc.c
> index 5601cc3..2177704 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -876,7 +876,8 @@ static int find_and_clear_dirty_height(struct VncState *vs,
>  static int vnc_update_client_sync(VncState *vs, int has_dirty)
>  {
>      int ret = vnc_update_client(vs, has_dirty);
> -    vnc_jobs_join(vs);
> +    if (ret >= 0)
> +        vnc_jobs_join(vs);

What happens with any running jobs if you skip the jouin call here?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] vnc: Fix qemu crash on vnc client disconnection

[Gonglei (Arei)]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Monday, October 28, 2013 3:53 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; Stefan Hajnoczi; Yanqiangjun; Luonengjun;
> Huangweidong (Hardware)
> Subject: Re: [Qemu-devel] [PATCH] vnc: Fix qemu crash on vnc client
> disconnection
> 
>   Hi,
> 
> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index 5601cc3..2177704 100644
> > --- a/ui/vnc.c
> > +++ b/ui/vnc.c
> > @@ -876,7 +876,8 @@ static int find_and_clear_dirty_height(struct
> VncState *vs,
> >  static int vnc_update_client_sync(VncState *vs, int has_dirty)
> >  {
> >      int ret = vnc_update_client(vs, has_dirty);
> > -    vnc_jobs_join(vs);
> > +    if (ret >= 0)
> > +        vnc_jobs_join(vs);
> 
> What happens with any running jobs if you skip the jouin call here?

Hi, Gerd. The other jobs are unaffected, and other clients still work.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH] vnc: Fix qemu crash on vnc client disconnection

[Gerd Hoffmann]

On Mo, 2013-10-28 at 08:47 +0000, Gonglei (Arei) wrote:
> > -----Original Message-----
> > From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> > Sent: Monday, October 28, 2013 3:53 PM
> > To: Gonglei (Arei)
> > Cc: qemu-devel@nongnu.org; Stefan Hajnoczi; Yanqiangjun; Luonengjun;
> > Huangweidong (Hardware)
> > Subject: Re: [Qemu-devel] [PATCH] vnc: Fix qemu crash on vnc client
> > disconnection
> > 
> >   Hi,
> > 
> > > diff --git a/ui/vnc.c b/ui/vnc.c
> > > index 5601cc3..2177704 100644
> > > --- a/ui/vnc.c
> > > +++ b/ui/vnc.c
> > > @@ -876,7 +876,8 @@ static int find_and_clear_dirty_height(struct
> > VncState *vs,
> > >  static int vnc_update_client_sync(VncState *vs, int has_dirty)
> > >  {
> > >      int ret = vnc_update_client(vs, has_dirty);
> > > -    vnc_jobs_join(vs);
> > > +    if (ret >= 0)
> > > +        vnc_jobs_join(vs);
> > 
> > What happens with any running jobs if you skip the jouin call here?
> 
> Hi, Gerd. The other jobs are unaffected, and other clients still work.

My concern is more that we might keep threads running which should not
run any more.

cheers,
  Gerd