[Spice-devel] [vdagent-win v2 2/2] vdagent-win: start vdagent with lock info from session
Victor Toso
victortoso at redhat.com
Thu Aug 11 15:03:04 UTC 2016
Hi,
On Thu, Aug 11, 2016 at 10:27:40AM -0400, Frediano Ziglio wrote:
> >
> > 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
I thought a little bit about it too but you know, this is already a
corner case of the corner case and so I thought it would be fine.
> and an hacker smiling :)
0:-)
A more 'hacker smile proof' could be the VDAgent lock state to start set
as true, so no file-transfer could happen till the 'unlock' event has
been received by either VDService or the Session.
> Basically you need to pass the current lock state to the agent you are
> launching. A command line option could work too.
Yeah but that would be a weird line argument.
vdagent.exe --session-locked=yes
>
> Frediano
What do you think, is it better to change the state on VDAgent?
Thanks for the review,
toso
More information about the Spice-devel
mailing list