Re: [PULL 09/11] ui/vnc: clipboard support

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

21.05.2021 15:51, Gerd Hoffmann wrote:
> This patch adds support for cut+paste to the qemu vnc server, which
> allows the vnc client exchange clipbaord data with qemu and other peers
> like the qemu vdagent implementation.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Message-id: 20210519053940.1888907-1-kraxel@redhat.com
> Message-Id: <20210519053940.1888907-8-kraxel@redhat.com>
> ---

[..]

> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -25,6 +25,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu-common.h"
>   #include "vnc.h"
>   #include "vnc-jobs.h"
>   #include "trace.h"
> @@ -1352,6 +1353,9 @@ void vnc_disconnect_finish(VncState *vs)
>           /* last client gone */
>           vnc_update_server_surface(vs->vd);
>       }
> +    if (vs->cbpeer.update.notify) {
> +        qemu_clipboard_peer_unregister(&vs->cbpeer);
> +    }
>   
>       vnc_unlock_output(vs);
>   

Hi Gerd!

Something is wrong here.

We are already under mutex, but calling qemu_clipboard_peer_unregister may trigger taking this mutex again in same call stack:

(gdb) bt
#0  0x00007f4c979b77b0 in __lll_lock_wait () at /lib64/libpthread.so.0
#1  0x00007f4c979b0553 in pthread_mutex_lock () at /lib64/libpthread.so.0
#2  0x0000558fb33bbb72 in qemu_mutex_lock_impl (mutex=0x558fb5416598, file=0x558fb3447fb7 "../ui/vnc-jobs.h", line=60) at ../util/qemu-thread-posix.c:80
#3  0x0000558fb2ca16c6 in vnc_lock_output (vs=0x558fb540a400) at ../ui/vnc-jobs.h:60
#4  0x0000558fb2ca1ab0 in vnc_clipboard_send (vs=0x558fb540a400, count=1, dwords=0x7ffe37f5f968) at ../ui/vnc-clipboard.c:138
#5  0x0000558fb2ca1f17 in vnc_clipboard_notify (notifier=0x558fb541a700, data=0x558fb524ee70) at ../ui/vnc-clipboard.c:209
#6  0x0000558fb33c25f0 in notifier_list_notify (list=0x558fb3a142f0 <clipboard_notifiers>, data=0x558fb524ee70) at ../util/notify.c:39
#7  0x0000558fb2c6d601 in qemu_clipboard_update (info=0x558fb524ee70) at ../ui/clipboard.c:50
#8  0x0000558fb2c6d570 in qemu_clipboard_peer_release (peer=0x558fb541a6f8, selection=QEMU_CLIPBOARD_SELECTION_CLIPBOARD) at ../ui/clipboard.c:41
#9  0x0000558fb2c6d4b6 in qemu_clipboard_peer_unregister (peer=0x558fb541a6f8) at ../ui/clipboard.c:19
#10 0x0000558fb2c8518a in vnc_disconnect_finish (vs=0x558fb540a400) at ../ui/vnc.c:1358
#11 0x0000558fb2c848b0 in vnc_update_client (vs=0x558fb540a400, has_dirty=0) at ../ui/vnc.c:1167
#12 0x0000558fb2c8a1ce in vnc_refresh (dcl=0x558fb5882610) at ../ui/vnc.c:3207
#13 0x0000558fb2c72dec in dpy_refresh (s=0x558fb4fe7970) at ../ui/console.c:1673
#14 0x0000558fb2c6ebe4 in gui_update (opaque=0x558fb4fe7970) at ../ui/console.c:158
#15 0x0000558fb33e862b in timerlist_run_timers (timer_list=0x558fb45ed200) at ../util/qemu-timer.c:573
#16 0x0000558fb33e86d5 in qemu_clock_run_timers (type=QEMU_CLOCK_REALTIME) at ../util/qemu-timer.c:587
#17 0x0000558fb33e899a in qemu_clock_run_all_timers () at ../util/qemu-timer.c:669
#18 0x0000558fb33e26ec in main_loop_wait (nonblocking=0) at ../util/main-loop.c:542
#19 0x0000558fb3099c75 in qemu_main_loop () at ../softmmu/runstate.c:726
#20 0x0000558fb2c62ae6 in main (argc=5, argv=0x7ffe37f5fdf8, envp=0x7ffe37f5fe28) at ../softmmu/main.c:50
(gdb) fr 2
#2  0x0000558fb33bbb72 in qemu_mutex_lock_impl (mutex=0x558fb5416598, file=0x558fb3447fb7 "../ui/vnc-jobs.h", line=60) at ../util/qemu-thread-posix.c:80
80          err = pthread_mutex_lock(&mutex->lock);
(gdb) p *mutex
$1 = {lock = {__data = {__lock = 2, __count = 0, __owner = 1549902, __nusers = 1, __kind = 0, __spins = 0, __elision = 0, __list = {__prev = 0x0, __next = 0x0}},
     __size = "\002\000\000\000\000\000\000\000N\246\027\000\001", '\000' <repeats 26 times>, __align = 2}, file = 0x558fb3442aaf "../ui/vnc-jobs.h", line = 60, initialized = true}
(gdb) info thr
   Id   Target Id                                             Frame
* 1    Thread 0x7f4c89ba8fc0 (LWP 1549902) "qemu-system-x86" 0x00007f4c979b77b0 in __lll_lock_wait () from /lib64/libpthread.so.0
   2    Thread 0x7f4c89a36640 (LWP 1549903) "qemu-system-x86" 0x00007f4c96ad8e0d in syscall () from /lib64/libc.so.6
   3    Thread 0x7f4c890b3640 (LWP 1549904) "qemu-system-x86" 0x00007f4c96ad35bf in poll () from /lib64/libc.so.6
   4    Thread 0x7f4c03fff640 (LWP 1549905) "qemu-system-x86" 0x00007f4c979baa8a in __futex_abstimed_wait_common64 () from /lib64/libpthread.so.0
   5    Thread 0x7f4c01bff640 (LWP 1549906) "qemu-system-x86" 0x00007f4c979baa8a in __futex_abstimed_wait_common64 () from /lib64/libpthread.so.0


Reproduce is simple:

On master branch, run something like this:

   ./build/qemu-system-x86_64 -vnc :7 --qmp stdio

Then, in two different terminals start connecting loops:

   while true; do vncviewer <vnc server ip>:5907; done

Then wait. If vncviewer loop stack for some reason, when qemu is not yet dead-locked, just restart the loop. Qemu screen becoming black - good sign of dead-lock happen, go into gdb and check.
By default vncviewer does non-shared connection, so previous client is closed when new one is connected. So we trigger many disconnects and connects in a loop.


I don't know this code, so I'm not sure that it would be safe just move qemu_clipboard_peer_unregister() out of the critical section...

-- 
Best regards,
Vladimir