| Message ID | 20211110103800.2266729-1-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | ui/vnc-clipboard: fix adding notifier twice | expand |
On 11/10/21 13:38, Vladimir Sementsov-Ogievskiy wrote: > vnc_server_cut_text_caps() is not guaranteed to be called only once. > > If it called twice, we finally call notifier_list_add() twice with same > element. Which leads to loopback QLIST. So, on next > notifier_list_notify() we'll loop forever and QEMU stuck. > > So, let's only register new notifier if it's not yet registered. > > Note, that similar check is used in vdagent_chr_recv_caps() (before > call qemu_clipboard_peer_register()), and also before > qemu_clipboard_peer_unregister() call in vdagent_disconnect() and in > vnc_disconnect_finish(). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > > Hi all! > > After backporting clipboard patches to our Rhel7-based downstream, we > faced Qemu stuck in notifier_list_notify(): > > (gdb) bt > #0 vnc_clipboard_notify (notifier=0x564427f283f8, data=0x564426c59a70) at ui/vnc-clipboard.c:193 > #1 0x0000564423455887 in notifier_list_notify (list=list@entry=0x564423d2b258 <clipboard_notifiers>, data=data@entry=0x564426c59a70) at util/notify.c:40 > #2 0x00005644233273bf in qemu_clipboard_update (info=info@entry=0x564426c59a70) at ui/clipboard.c:19 > #3 0x000056442334efd2 in vnc_client_cut_text_ext (vs=vs@entry=0x564427f18000, len=len@entry=4, flags=<optimized out>, > data=data@entry=0x5644263cc00c "\002\f\001\251\020\377\377\377!\377\377\377\314\376\377\377\315\376\377\377 \377\377\377\316\345\241\300\307\376\377\377\310\376\377\377\376\376\377\377\a") > at ui/vnc-clipboard.c:256 > #4 0x000056442333b172 in protocol_client_msg (vs=0x564427f18000, data=0x5644263cc000 "\006", len=<optimized out>) at ui/vnc.c:2396 > #5 0x0000564423338af6 in vnc_client_read (vs=0x564427f18000) at ui/vnc.c:1537 > #6 vnc_client_io (ioc=<optimized out>, condition=G_IO_IN, opaque=0x564427f18000) at ui/vnc.c:1559 > #7 0x00007f07b02cf147 in g_main_dispatch (context=0x564425546bb0) at gmain.c:3192 > #8 g_main_context_dispatch (context=context@entry=0x564425546bb0) at gmain.c:3845 > #9 0x00005644234468f7 in glib_pollfds_poll () at util/main-loop.c:215 > #10 os_host_main_loop_wait (timeout=<optimized out>) at util/main-loop.c:238 > #11 main_loop_wait (nonblocking=nonblocking@entry=0) at util/main-loop.c:497 > > > investigations shows, that notifier list has only one element which points to itself as next. So, we are in the endless loop. > > Seems that it's possible, if vnc_server_cut_text_caps() called twice. Then it registers notifier twice and it added to QLIST twice, which leads to the situation. > > > I don't have any reproducer and not sure that bug may be reproduced on > master. > > I'm not familiar with ui code - may be vnc_server_cut_text_caps() should > never be called twice? Or notifier should be removed somehow before the > second call? Maybe this patch just shadows another bug? > > But what I do know, is that we should not put same element into QLIST > twice. And if the check I propose is not needed we should add an > assertion instead: > > assert(!vs->cbpeer.update.notify); > > > ui/vnc-clipboard.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/ui/vnc-clipboard.c b/ui/vnc-clipboard.c > index 9f077965d0..67284b556c 100644 > --- a/ui/vnc-clipboard.c > +++ b/ui/vnc-clipboard.c > @@ -316,8 +316,10 @@ void vnc_server_cut_text_caps(VncState *vs) > caps[1] = 0; > vnc_clipboard_send(vs, 2, caps); > > - vs->cbpeer.name = "vnc"; > - vs->cbpeer.update.notify = vnc_clipboard_notify; > - vs->cbpeer.request = vnc_clipboard_request; > - qemu_clipboard_peer_register(&vs->cbpeer); > + if (!vs->cbpeer.update.notify) { > + vs->cbpeer.name = "vnc"; > + vs->cbpeer.update.notify = vnc_clipboard_notify; > + vs->cbpeer.request = vnc_clipboard_request; > + qemu_clipboard_peer_register(&vs->cbpeer); > + } > } Perhaps QLIST_IS_INSERTED will be suitable for such checks because I couldn't find any initialize of .notify pointer so it can potentially be UB.
21.11.2021 22:12, Nikta Lapshin wrote: > > On 11/10/21 13:38, Vladimir Sementsov-Ogievskiy wrote: >> vnc_server_cut_text_caps() is not guaranteed to be called only once. >> >> If it called twice, we finally call notifier_list_add() twice with same >> element. Which leads to loopback QLIST. So, on next >> notifier_list_notify() we'll loop forever and QEMU stuck. >> >> So, let's only register new notifier if it's not yet registered. >> >> Note, that similar check is used in vdagent_chr_recv_caps() (before >> call qemu_clipboard_peer_register()), and also before >> qemu_clipboard_peer_unregister() call in vdagent_disconnect() and in >> vnc_disconnect_finish(). >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> >> Hi all! >> >> After backporting clipboard patches to our Rhel7-based downstream, we >> faced Qemu stuck in notifier_list_notify(): >> >> (gdb) bt >> #0 vnc_clipboard_notify (notifier=0x564427f283f8, data=0x564426c59a70) at ui/vnc-clipboard.c:193 >> #1 0x0000564423455887 in notifier_list_notify (list=list@entry=0x564423d2b258 <clipboard_notifiers>, data=data@entry=0x564426c59a70) at util/notify.c:40 >> #2 0x00005644233273bf in qemu_clipboard_update (info=info@entry=0x564426c59a70) at ui/clipboard.c:19 >> #3 0x000056442334efd2 in vnc_client_cut_text_ext (vs=vs@entry=0x564427f18000, len=len@entry=4, flags=<optimized out>, >> data=data@entry=0x5644263cc00c "\002\f\001\251\020\377\377\377!\377\377\377\314\376\377\377\315\376\377\377 \377\377\377\316\345\241\300\307\376\377\377\310\376\377\377\376\376\377\377\a") >> at ui/vnc-clipboard.c:256 >> #4 0x000056442333b172 in protocol_client_msg (vs=0x564427f18000, data=0x5644263cc000 "\006", len=<optimized out>) at ui/vnc.c:2396 >> #5 0x0000564423338af6 in vnc_client_read (vs=0x564427f18000) at ui/vnc.c:1537 >> #6 vnc_client_io (ioc=<optimized out>, condition=G_IO_IN, opaque=0x564427f18000) at ui/vnc.c:1559 >> #7 0x00007f07b02cf147 in g_main_dispatch (context=0x564425546bb0) at gmain.c:3192 >> #8 g_main_context_dispatch (context=context@entry=0x564425546bb0) at gmain.c:3845 >> #9 0x00005644234468f7 in glib_pollfds_poll () at util/main-loop.c:215 >> #10 os_host_main_loop_wait (timeout=<optimized out>) at util/main-loop.c:238 >> #11 main_loop_wait (nonblocking=nonblocking@entry=0) at util/main-loop.c:497 >> >> >> investigations shows, that notifier list has only one element which points to itself as next. So, we are in the endless loop. >> >> Seems that it's possible, if vnc_server_cut_text_caps() called twice. Then it registers notifier twice and it added to QLIST twice, which leads to the situation. >> >> >> I don't have any reproducer and not sure that bug may be reproduced on >> master. >> >> I'm not familiar with ui code - may be vnc_server_cut_text_caps() should >> never be called twice? Or notifier should be removed somehow before the >> second call? Maybe this patch just shadows another bug? >> >> But what I do know, is that we should not put same element into QLIST >> twice. And if the check I propose is not needed we should add an >> assertion instead: >> >> assert(!vs->cbpeer.update.notify); >> >> >> ui/vnc-clipboard.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/ui/vnc-clipboard.c b/ui/vnc-clipboard.c >> index 9f077965d0..67284b556c 100644 >> --- a/ui/vnc-clipboard.c >> +++ b/ui/vnc-clipboard.c >> @@ -316,8 +316,10 @@ void vnc_server_cut_text_caps(VncState *vs) >> caps[1] = 0; >> vnc_clipboard_send(vs, 2, caps); >> - vs->cbpeer.name = "vnc"; >> - vs->cbpeer.update.notify = vnc_clipboard_notify; >> - vs->cbpeer.request = vnc_clipboard_request; >> - qemu_clipboard_peer_register(&vs->cbpeer); >> + if (!vs->cbpeer.update.notify) { >> + vs->cbpeer.name = "vnc"; >> + vs->cbpeer.update.notify = vnc_clipboard_notify; >> + vs->cbpeer.request = vnc_clipboard_request; >> + qemu_clipboard_peer_register(&vs->cbpeer); >> + } >> } > > > Perhaps QLIST_IS_INSERTED will be suitable for such checks because I couldn't find any initialize of .notify pointer so it can potentially be UB. > I think, vs structure should be initialized to zero at start. For example at start of vnc_connect(): "VncState *vs = g_new0(VncState, 1);", and I didn't find another place where it is allocated. Also similar checks are already used in the code, so I think better to behave similarly here.
diff --git a/ui/vnc-clipboard.c b/ui/vnc-clipboard.c index 9f077965d0..67284b556c 100644 --- a/ui/vnc-clipboard.c +++ b/ui/vnc-clipboard.c @@ -316,8 +316,10 @@ void vnc_server_cut_text_caps(VncState *vs) caps[1] = 0; vnc_clipboard_send(vs, 2, caps); - vs->cbpeer.name = "vnc"; - vs->cbpeer.update.notify = vnc_clipboard_notify; - vs->cbpeer.request = vnc_clipboard_request; - qemu_clipboard_peer_register(&vs->cbpeer); + if (!vs->cbpeer.update.notify) { + vs->cbpeer.name = "vnc"; + vs->cbpeer.update.notify = vnc_clipboard_notify; + vs->cbpeer.request = vnc_clipboard_request; + qemu_clipboard_peer_register(&vs->cbpeer); + } }
vnc_server_cut_text_caps() is not guaranteed to be called only once. If it called twice, we finally call notifier_list_add() twice with same element. Which leads to loopback QLIST. So, on next notifier_list_notify() we'll loop forever and QEMU stuck. So, let's only register new notifier if it's not yet registered. Note, that similar check is used in vdagent_chr_recv_caps() (before call qemu_clipboard_peer_register()), and also before qemu_clipboard_peer_unregister() call in vdagent_disconnect() and in vnc_disconnect_finish(). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- Hi all! After backporting clipboard patches to our Rhel7-based downstream, we faced Qemu stuck in notifier_list_notify(): (gdb) bt #0 vnc_clipboard_notify (notifier=0x564427f283f8, data=0x564426c59a70) at ui/vnc-clipboard.c:193 #1 0x0000564423455887 in notifier_list_notify (list=list@entry=0x564423d2b258 <clipboard_notifiers>, data=data@entry=0x564426c59a70) at util/notify.c:40 #2 0x00005644233273bf in qemu_clipboard_update (info=info@entry=0x564426c59a70) at ui/clipboard.c:19 #3 0x000056442334efd2 in vnc_client_cut_text_ext (vs=vs@entry=0x564427f18000, len=len@entry=4, flags=<optimized out>, data=data@entry=0x5644263cc00c "\002\f\001\251\020\377\377\377!\377\377\377\314\376\377\377\315\376\377\377 \377\377\377\316\345\241\300\307\376\377\377\310\376\377\377\376\376\377\377\a") at ui/vnc-clipboard.c:256 #4 0x000056442333b172 in protocol_client_msg (vs=0x564427f18000, data=0x5644263cc000 "\006", len=<optimized out>) at ui/vnc.c:2396 #5 0x0000564423338af6 in vnc_client_read (vs=0x564427f18000) at ui/vnc.c:1537 #6 vnc_client_io (ioc=<optimized out>, condition=G_IO_IN, opaque=0x564427f18000) at ui/vnc.c:1559 #7 0x00007f07b02cf147 in g_main_dispatch (context=0x564425546bb0) at gmain.c:3192 #8 g_main_context_dispatch (context=context@entry=0x564425546bb0) at gmain.c:3845 #9 0x00005644234468f7 in glib_pollfds_poll () at util/main-loop.c:215 #10 os_host_main_loop_wait (timeout=<optimized out>) at util/main-loop.c:238 #11 main_loop_wait (nonblocking=nonblocking@entry=0) at util/main-loop.c:497 investigations shows, that notifier list has only one element which points to itself as next. So, we are in the endless loop. Seems that it's possible, if vnc_server_cut_text_caps() called twice. Then it registers notifier twice and it added to QLIST twice, which leads to the situation. I don't have any reproducer and not sure that bug may be reproduced on master. I'm not familiar with ui code - may be vnc_server_cut_text_caps() should never be called twice? Or notifier should be removed somehow before the second call? Maybe this patch just shadows another bug? But what I do know, is that we should not put same element into QLIST twice. And if the check I propose is not needed we should add an assertion instead: assert(!vs->cbpeer.update.notify); ui/vnc-clipboard.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)