[Spice-devel] [vdagent-win v2 2/2] vdagent-win: start vdagent with lock info from session

[Frediano Ziglio]

> 
> Commit 5907b6cbb5c724f9729da59a644271b4258d122e started to handle
> Lock/Unlock events from Session at VDAgent. Although that works just
> fine, it does not cover all the situations as pointed by Andrei at [0]
> and I quote:
> 
> > It fails for next test-case:
> >
> > * Connect with RV to VM
> > * Lock VM (ctrl-alt-del)
> > * Close RV
> > * Connect with RV to the VM again
> > * Do not unlock session. DnD files from client to locked VM.
> >
> > Result : Files are copied to VM.
> 
> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1323628#c6
> 
> To solve this situation, we need VDService to inform VDAgent if
> Session is Locked or not upon its initialization.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1323628
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> Reported-by: Andrei Stepanov <astepano at redhat.com>
> ---
>  common/vdcommon.h       |  1 +
>  vdagent/vdagent.cpp     | 16 +++++++++++++++-
>  vdservice/vdservice.cpp | 15 +++++++++++++++
>  3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/common/vdcommon.h b/common/vdcommon.h
> index f4859e2..8539c38 100644
> --- a/common/vdcommon.h
> +++ b/common/vdcommon.h
> @@ -35,6 +35,7 @@ typedef CRITICAL_SECTION mutex_t;
>  
>  #define VD_AGENT_REGISTRY_KEY "SOFTWARE\\Red Hat\\Spice\\vdagent\\"
>  #define VD_AGENT_STOP_EVENT   TEXT("Global\\vdagent_stop_event")
> +#define VD_AGENT_SESSION_LOCKED_EVENT
> TEXT("Global\\vdagent_session_locked_event")
>  
>  #if defined __GNUC__
>  #define ALIGN_GCC __attribute__ ((packed))
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index d3cbd5f..06d6574 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -142,6 +142,7 @@ private:
>      DWORD _input_time;
>      HANDLE _control_event;
>      HANDLE _stop_event;
> +    HANDLE _session_locked_event;
>      VDAgentMessage* _in_msg;
>      uint32_t _in_msg_pos;
>      bool _pending_input;
> @@ -202,6 +203,7 @@ VDAgent::VDAgent()
>      , _input_time (0)
>      , _control_event (NULL)
>      , _stop_event (NULL)
> +    , _session_locked_event (NULL)
>      , _in_msg (NULL)
>      , _in_msg_pos (0)
>      , _pending_input (false)
> @@ -308,6 +310,7 @@ bool VDAgent::run()
>          return false;
>      }
>      _stop_event = OpenEvent(SYNCHRONIZE, FALSE, VD_AGENT_STOP_EVENT);
> +    _session_locked_event = OpenEvent(SYNCHRONIZE, FALSE,
> VD_AGENT_SESSION_LOCKED_EVENT);
>      memset(&wcls, 0, sizeof(wcls));
>      wcls.lpfnWndProc = &VDAgent::wnd_proc;
>      wcls.lpszClassName = VD_AGENT_WINCLASS_NAME;
> @@ -482,7 +485,7 @@ void VDAgent::input_desktop_message_loop()
>  
>  void VDAgent::event_dispatcher(DWORD timeout, DWORD wake_mask)
>  {
> -    HANDLE events[2];
> +    HANDLE events[3];
>      DWORD event_count = 1;
>      DWORD wait_ret;
>      DWORD action;
> @@ -490,6 +493,7 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> wake_mask)
>      enum {
>          CONTROL_ACTION,
>          STOP_ACTION,
> +        SESSION_LOCKED_ACTION,
>      } actions[G_N_ELEMENTS(events)];
>  
>      events[0] = _control_event;
> @@ -500,6 +504,12 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> wake_mask)
>          event_count++;
>      }
>  
> +    if (_session_locked_event) {
> +        events[event_count] = _session_locked_event;
> +        actions[event_count] = SESSION_LOCKED_ACTION;
> +        event_count++;
> +    }
> +
>      wait_ret = MsgWaitForMultipleObjectsEx(event_count, events, timeout,
>      wake_mask, MWMO_ALERTABLE);
>      if (wait_ret == WAIT_OBJECT_0 + event_count) {
>          while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {
> @@ -524,6 +534,10 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> wake_mask)
>          vd_printf("%s: received stop event", __func__);
>          _running = false;
>          break;
> +    case SESSION_LOCKED_ACTION:
> +        vd_printf("%s: received session locked event", __func__);
> +        _session_is_locked = true;
> +        break;
>      default:
>          vd_printf("%s: action not handled (%lu)", __func__, action);
>          _running = false;
> diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
> index 89e0dbb..1f53d97 100644
> --- a/vdservice/vdservice.cpp
> +++ b/vdservice/vdservice.cpp
> @@ -93,6 +93,7 @@ private:
>      PROCESS_INFORMATION _agent_proc_info;
>      HANDLE _control_event;
>      HANDLE _agent_stop_event;
> +    HANDLE _session_locked_event;
>      HANDLE* _events;
>      TCHAR _agent_path[MAX_PATH];
>      VDControlQueue _control_queue;
> @@ -105,6 +106,7 @@ private:
>      int _system_version;
>      bool _agent_alive;
>      bool _running;
> +    bool _session_is_locked;
>      VDLog* _log;
>      unsigned _events_count;
>  };
> @@ -128,6 +130,7 @@ VDService::VDService()
>      , _agent_restarts (0)
>      , _agent_alive (false)
>      , _running (false)
> +    , _session_is_locked (false)
>      , _log (NULL)
>      , _events_count(0)
>  {
> @@ -135,6 +138,7 @@ VDService::VDService()
>      _system_version = supported_system_version();
>      _control_event = CreateEvent(NULL, FALSE, FALSE, NULL);
>      _agent_stop_event = CreateEvent(NULL, FALSE, FALSE,
>      VD_AGENT_STOP_EVENT);
> +    _session_locked_event = CreateEvent(NULL, FALSE, FALSE,
> VD_AGENT_SESSION_LOCKED_EVENT);
>      _agent_path[0] = wchar_t('\0');
>      MUTEX_INIT(_agent_mutex);
>      MUTEX_INIT(_control_mutex);
> @@ -143,6 +147,7 @@ VDService::VDService()
>  
>  VDService::~VDService()
>  {
> +    CloseHandle(_session_locked_event);
>      CloseHandle(_agent_stop_event);
>      CloseHandle(_control_event);
>      delete _events;
> @@ -307,6 +312,10 @@ DWORD WINAPI VDService::control_handler(DWORD control,
> DWORD event_type, LPVOID
>          if (event_type == WTS_CONSOLE_CONNECT) {
>              s->_session_id = session_id;
>              s->set_control_event(VD_CONTROL_RESTART_AGENT);
> +        } else if (event_type == WTS_SESSION_LOCK) {
> +            s->_session_is_locked = true;
> +        } else if (event_type == WTS_SESSION_UNLOCK) {
> +            s->_session_is_locked = false;
>          }
>          break;
>      }
> @@ -744,6 +753,12 @@ bool VDService::launch_agent()
>          return false;
>      }
>      _agent_alive = true;
> +
> +    if (_session_is_locked) {
> +        /* If we just create a new VDAgent but the session is locked, we
> must
> +         * notify it so it can handle requests from client correctly */
> +        SetEvent(_session_locked_event);
> +    }
>      return true;
>  }
>  

I don't know why but this smells like a race condition and an
hacker smiling :)
Basically you need to pass the current lock state to the agent you are
launching. A command line option could work too.

Frediano