> > Hi, > > On Wed, Dec 14, 2016 at 05:07:17PM -0500, Frediano Ziglio wrote: > > > > > > 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@xxxxxxxxxx> > > > Reported-by: Andrei Stepanov <astepano@xxxxxxxxxx> > > > --- > > > 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 > > I'm all in for a new design. I did not try to understand why things are > like they are, I was only trying to provide a solution to > not-really-big-problem :) > > I think we should make some room to discuss improvements in vdagent for > both, linux and windows guests. > > Unless it is proven to be bad, I wish we could come up with something > like spice-gtk for the agents. A codebase that is used for > windows/linux. > > While we are not there... Anything you could think of that might be a > problem in this patch? :) > > Cheers, > toso > There is a base design flow on these patches. They assume that there is a single session with a lock/unlock state. This is not true considering switching users. In this case there are multiple sessions each one with a possible lock/unlock state. Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel