[Spice-devel] [vdagent-win v2] VDService to notify VDAgent about session status
Frediano Ziglio
fziglio at redhat.com
Wed Dec 14 22:07:17 UTC 2016
>
> Commit 5907b6cbb5c724f9729da59a644271b4258d122e started to handle
> Lock/Unlock events from Session at VDAgent. That seemed to work fine
> but as pointed by Andrei at [0], it does not cover the following
> situation:
>
> > 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
>
> There is also race between receiving sessions events (like
> WTS_SESSION_LOCK/UNLOCK) and the CONTROL_DESKTOP_SWITCH control
> action, which eventually calls WTSUnRegisterSessionNotification().
>
> To solve this situation, we need VDService to inform VDAgent if
> Session is Locked or not, always.
>
> - We have two new events to trigger both lock/unlock events between
> VDService and VDAgent.
> - VDAgent is always initialized with session_is_locked = true
>
> 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 | 2 ++
> vdagent/vdagent.cpp | 34 ++++++++++++++++++++++++++++------
> vdservice/vdservice.cpp | 20 ++++++++++++++++++++
> 3 files changed, 50 insertions(+), 6 deletions(-)
>
> diff --git a/common/vdcommon.h b/common/vdcommon.h
> index c1920e9..6b2b5f1 100644
> --- a/common/vdcommon.h
> +++ b/common/vdcommon.h
> @@ -66,6 +66,8 @@ typedef Mutex 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")
> +#define VD_AGENT_SESSION_UNLOCKED_EVENT
> TEXT("Global\\vdagent_session_unlocked_event")
>
> #if defined __GNUC__
> #define ALIGN_GCC __attribute__ ((packed))
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index e3ba14b..95f7eef 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -148,6 +148,8 @@ private:
> DWORD _input_time;
> HANDLE _control_event;
> HANDLE _stop_event;
> + HANDLE _session_locked_event;
> + HANDLE _session_unlocked_event;
> VDAgentMessage* _in_msg;
> uint32_t _in_msg_pos;
> bool _pending_input;
> @@ -207,11 +209,13 @@ VDAgent::VDAgent()
> , _input_time (0)
> , _control_event (NULL)
> , _stop_event (NULL)
> + , _session_locked_event (NULL)
> + , _session_unlocked_event (NULL)
> , _in_msg (NULL)
> , _in_msg_pos (0)
> , _pending_input (false)
> , _running (false)
> - , _session_is_locked (false)
> + , _session_is_locked (true)
> , _desktop_switch (false)
> , _desktop_layout (NULL)
> , _display_setting (VD_AGENT_REGISTRY_KEY)
> @@ -309,6 +313,8 @@ 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);
> + _session_unlocked_event = OpenEvent(SYNCHRONIZE, FALSE,
> VD_AGENT_SESSION_UNLOCKED_EVENT);
> memset(&wcls, 0, sizeof(wcls));
> wcls.lpfnWndProc = &VDAgent::wnd_proc;
> wcls.lpszClassName = VD_AGENT_WINCLASS_NAME;
> @@ -479,13 +485,15 @@ void VDAgent::input_desktop_message_loop()
>
> void VDAgent::event_dispatcher(DWORD timeout, DWORD wake_mask)
> {
> - HANDLE events[2];
> + HANDLE events[4];
> DWORD event_count = 1;
> DWORD wait_ret;
> MSG msg;
> enum {
> CONTROL_ACTION,
> STOP_ACTION,
> + SESSION_LOCKED_ACTION,
> + SESSION_UNLOCKED_ACTION,
> } actions[SPICE_N_ELEMENTS(events)], action;
>
> events[0] = _control_event;
> @@ -496,6 +504,16 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> wake_mask)
> event_count++;
> }
>
> + if (_session_unlocked_event && _session_locked_event) {
> + events[event_count] = _session_locked_event;
> + actions[event_count] = SESSION_LOCKED_ACTION;
> + event_count++;
> +
> + events[event_count] = _session_unlocked_event;
> + actions[event_count] = SESSION_UNLOCKED_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)) {
> @@ -520,6 +538,14 @@ 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;
> + case SESSION_UNLOCKED_ACTION:
> + vd_printf("%s: received session unlocked event", __func__);
> + _session_is_locked = false;
> + break;
> default:
> vd_printf("%s: action not handled (%d)", __func__, action);
> _running = false;
> @@ -1516,10 +1542,6 @@ LRESULT CALLBACK VDAgent::wnd_proc(HWND hwnd, UINT
> message, WPARAM wparam, LPARA
> case WM_WTSSESSION_CHANGE:
> if (wparam == WTS_SESSION_LOGON) {
> a->set_control_event(CONTROL_LOGON);
> - } else if (wparam == WTS_SESSION_LOCK) {
> - a->_session_is_locked = true;
> - } else if (wparam == WTS_SESSION_UNLOCK) {
> - a->_session_is_locked = false;
> }
> break;
> default:
> diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
> index 1892b72..9a11d87 100644
> --- a/vdservice/vdservice.cpp
> +++ b/vdservice/vdservice.cpp
> @@ -91,6 +91,8 @@ private:
> PROCESS_INFORMATION _agent_proc_info;
> HANDLE _control_event;
> HANDLE _agent_stop_event;
> + HANDLE _session_locked_event;
> + HANDLE _session_unlocked_event;
> HANDLE* _events;
> TCHAR _agent_path[MAX_PATH];
> VDControlQueue _control_queue;
> @@ -103,6 +105,7 @@ private:
> int _system_version;
> bool _agent_alive;
> bool _running;
> + bool _session_is_locked;
> VDLog* _log;
> unsigned _events_count;
> };
> @@ -116,6 +119,7 @@ VDService::VDService()
> , _agent_restarts (0)
> , _agent_alive (false)
> , _running (false)
> + , _session_is_locked (false)
> , _log (NULL)
> , _events_count(0)
> {
> @@ -123,11 +127,15 @@ 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);
> + _session_unlocked_event = CreateEvent(NULL, FALSE, FALSE,
> VD_AGENT_SESSION_UNLOCKED_EVENT);
> _agent_path[0] = wchar_t('\0');
> }
>
> VDService::~VDService()
> {
> + CloseHandle(_session_unlocked_event);
> + CloseHandle(_session_locked_event);
> CloseHandle(_agent_stop_event);
> CloseHandle(_control_event);
> delete[] _events;
> @@ -290,6 +298,12 @@ 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;
> + SetEvent(s->_session_locked_event);
> + } else if (event_type == WTS_SESSION_UNLOCK) {
> + s->_session_is_locked = false;
> + SetEvent(s->_session_unlocked_event);
> }
> break;
> }
> @@ -728,6 +742,12 @@ bool VDService::launch_agent()
> return false;
> }
> _agent_alive = true;
> +
> + if (!_session_is_locked) {
> + /* For security reasons, VDAgent always starts thinking its session
> is
> + * locked. We should notify that's not true */
> + SetEvent(_session_unlocked_event);
> + }
> return true;
> }
>
Not against this patch but the more I dig into the Windows agent the
less I understand the separation design.
I though was to separate process users but in both Windows 7 and
Windows 10 agent and service are run as SYSTEM.
I though in the past was to have multiple agents but there can
be only one running.
System services can interact with desktop so in theory you can do
everything with a single process if security separation is not
required. Maybe the developer that start all this ignored the
SERVICE_INTERACTIVE_PROCESS flag registering the service?
Or maybe all this weird code is to support some really old version
of Windows? Maybe Windows 2000 terminal server? However the actual
code seems to support from Windows XP onward.
Frediano
More information about the Spice-devel
mailing list