Re: [PATCH] ui/clipboard: avoid crash upon request when clipboard peer is not initialized

[Marc-André Lureau]

Hi

On Mon, Jan 15, 2024 at 2:45 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Am 14.01.24 um 14:51 schrieb Marc-André Lureau:
> >>
> >> diff --git a/ui/clipboard.c b/ui/clipboard.c
> >> index 3d14bffaf8..c13b54d2e9 100644
> >> --- a/ui/clipboard.c
> >> +++ b/ui/clipboard.c
> >> @@ -129,7 +129,8 @@ void qemu_clipboard_request(QemuClipboardInfo *info,
> >>      if (info->types[type].data ||
> >>          info->types[type].requested ||
> >>          !info->types[type].available ||
> >> -        !info->owner)
> >> +        !info->owner ||
> >> +        !info->owner->request)
> >>          return;
> >
> > While that fixes the crash, I think we should handle the situation
> > earlier. A clipboard peer shouldn't be allowed to hold the clipboard
> > if it doesn't have the data available or a "request" callback set.
> >
>
> Where should initialization of the cbpeer happen so that we are
> guaranteed to do it also for clients that do not set the
> VNC_FEATURE_CLIPBOARD_EXT feature? Can the vnc_clipboard_request
> function be re-used for clients without that feature or will it be
> necessary to add some kind of "dummy" request callback for those clients?

qemu_clipboard_update() shouldn't accept info as current clipboard if
the owner doesn't have the data available or a "request" callback set.
This should also be assert() somehow and handled earlier.

In vnc_client_cut_text_ext() we could detect that situation, but with
Daniel's "[PATCH] ui: reject extended clipboard message if not
activated", this shouldn't happen anymore iiuc.

>
> > Iow, we should have an assert(info->owner->request != NULL) here instead.
> >
> Your choice of course, but it would be a crash again should the issue
> ever re-appear. Would error message (so the issue gets noticed) + return
> be an option too?
>
> Best Regards,
> Fiona
>


-- 
Marc-André Lureau