[PATCH] ui/vnc-clipboard: fix adding notifier twice

[PATCH] ui/vnc-clipboard: fix adding notifier twice

[Vladimir Sementsov-Ogievskiy]

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);
+    }
 }
-- 
2.31.1

Re: [PATCH] ui/vnc-clipboard: fix adding notifier twice

[Nikta Lapshin]

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.

Re: [PATCH] ui/vnc-clipboard: fix adding notifier twice

[Vladimir Sementsov-Ogievskiy]

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.

-- 
Best regards,
Vladimir