[Spice-devel] [vdagent-win v2] VDService to notify VDAgent about session status
Frediano Ziglio
fziglio at redhat.com
Tue Dec 20 12:07:50 UTC 2016
>
> Hi,
>
> On Thu, Dec 15, 2016 at 12:10:46PM -0500, Frediano Ziglio wrote:
> > >
> > > 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 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
> > >
> > > 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
>
> Nowadays, isn't it one vdagent per system (not per user) ?
> What I have tested is tracking the lock/unlock state of vdagent while
> connecting/disconnecting in a locked/unlocked session.
>
> At the moment, I don't see other use cases. Let me know how can I
> trigger a bad state or weird behavior.
>
> All in all, I agree that having vdservice to inform the *session* state
> to the vdagent is bad but I don't feel like improving vdagent/vdservice
> design as this moment.
>
> So, either this patch is okay or we can come up with a better design in
> 2017 :)
>
> Cheers,
> toso
>
Could be an easier implementation to just use the event as a shared flag
instead of having to handle 2 new events?
I mean something like
Service:
event_locked = CreateEvent(NULL, TRUE, FALSE, <name>)
..
if (locked)
SetEvent(event_locked);
else
ResetEvent(event_locked);
Agent
event_locked = OpenEvent(... <name>);
..
bool is_locked() const
{ return WaitForSingleObject(event_locked, 0) != WAIT_TIMEOUT; }
and use is_locked() instead of _session_is_locked ?
By the way, what can happen is that the current session is not
locked but another session get locked after this one causing
_session_is_locked to be set to true and file transfer to be
disabled.
Could this happen connecting with RDP?
Could this happen switching used and calling an API to lock
the not visible session?
And worst, can another session be used to set this flag to
false while should be true?
Probably these are not regression but we really need to
understand these.
Current code (service) is:
case SERVICE_CONTROL_SESSIONCHANGE: {
DWORD session_id = ((WTSSESSION_NOTIFICATION*)event_data)->dwSessionId;
// TODO check possible overflow
vd_printf("Session %lu %s", session_id, session_events[event_type]);
SetServiceStatus(s->_status_handle, &s->_status);
if (event_type == WTS_CONSOLE_CONNECT) {
s->_session_id = session_id;
s->set_control_event(VD_CONTROL_RESTART_AGENT);
}
break;
what tell that the session_id is the current interactive session?
Could be remote or an hidden one.
Maybe we should check against WTSGetActiveConsoleSessionId?
Frediano
More information about the Spice-devel
mailing list