[Spice-devel] [PATCH spice 1/5] spicec: Move setting of clipboard_owner to guest to platform code

[Arnon Gilboa]

ack. can't we use the lock for the other 2 win/x11 events as well?

Hans de Goede wrote:
> Atleast under x11 there is a race condition when setting the clipboard
> owner to guest from the RedClient code rather then doing it in Platform.
>
> After the XSetSelectionOwner() in Platform::on_clipboard_grab(), which runs
> from the main message loop thread, the x11 event thread can receive a
> SelectionRequest event from another x11 app, before the RedClient code
> has set the clipboard owner, which will trigger the owner != guest
> check in the SelectionRequest event handling code.
>
> By moving the setting of the owner in to Platform::on_clipboard_grab() it
> gets protected by the clipboard lock, which closes this tiny race.
> ---
>  client/platform.h           |    2 ++
>  client/red_client.cpp       |    1 -
>  client/windows/platform.cpp |    7 +++++++
>  client/x11/platform.cpp     |   10 ++++++++--
>  4 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/client/platform.h b/client/platform.h
> index af4a0f6..a9a1715 100644
> --- a/client/platform.h
> +++ b/client/platform.h
> @@ -132,6 +132,8 @@ public:
>      static int  get_clipboard_owner() { return _clipboard_owner; }
>  
>  private:
> +    static void set_clipboard_owner_unlocked(int new_owner);
> +
>      static int _clipboard_owner;
>  };
>  
> diff --git a/client/red_client.cpp b/client/red_client.cpp
> index 8e7dfe0..0650e35 100644
> --- a/client/red_client.cpp
> +++ b/client/red_client.cpp
> @@ -1118,7 +1118,6 @@ void RedClient::dispatch_agent_message(VDAgentMessage* msg, void* data)
>      case VD_AGENT_CLIPBOARD_GRAB:
>          Platform::on_clipboard_grab((uint32_t *)data,
>                                        msg->size / sizeof(uint32_t));
> -        Platform::set_clipboard_owner(Platform::owner_guest);
>          break;
>      case VD_AGENT_CLIPBOARD_REQUEST:
>          if (Platform::get_clipboard_owner() != Platform::owner_client) {
> diff --git a/client/windows/platform.cpp b/client/windows/platform.cpp
> index 580a40a..075d269 100644
> --- a/client/windows/platform.cpp
> +++ b/client/windows/platform.cpp
> @@ -859,6 +859,11 @@ void WinPlatform::exit_modal_loop()
>  
>  int Platform::_clipboard_owner = Platform::owner_none;
>  
> +void Platform::set_clipboard_owner_unlocked(int new_owner)
> +{
> +    set_clipboard_owner(new_owner);
> +}
> +
>  void Platform::set_clipboard_owner(int new_owner)
>  {
>      if (new_owner == owner_none) {
> @@ -885,6 +890,8 @@ bool Platform::on_clipboard_grab(uint32_t *types, uint32_t type_count)
>      EmptyClipboard();
>      SetClipboardData(format, NULL);
>      CloseClipboard();
> +
> +    set_clipboard_owner(owner_guest);
>      return true;
>  }
>  
> diff --git a/client/x11/platform.cpp b/client/x11/platform.cpp
> index 29b5f75..8f0665c 100644
> --- a/client/x11/platform.cpp
> +++ b/client/x11/platform.cpp
> @@ -3355,6 +3355,8 @@ bool Platform::on_clipboard_grab(uint32_t *types, uint32_t type_count)
>  
>      XSetSelectionOwner(x_display, clipboard_prop, platform_win, CurrentTime);
>      XFlush(x_display);
> +
> +    set_clipboard_owner_unlocked(owner_guest);
>      return true;
>  }
>  
> @@ -3362,12 +3364,16 @@ int Platform::_clipboard_owner = Platform::owner_none;
>  
>  void Platform::set_clipboard_owner(int new_owner)
>  {
> +        Lock lock(clipboard_lock);
> +        set_clipboard_owner_unlocked(new_owner);
> +}
> +
> +void Platform::set_clipboard_owner_unlocked(int new_owner)
> +{
>      const char * const owner_str[] = { "none", "guest", "client" };
>  
>      /* Clear pending requests and clipboard data */
>      {
> -        Lock lock(clipboard_lock);
> -
>          if (next_selection_request) {
>              LOG_INFO("selection requests pending upon clipboard owner change, clearing");
>              while (next_selection_request)
>