Re: [vdagent-win v2] VDService to notify VDAgent about session status

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> 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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]