[Spice-devel] Graceful failure when VNC client connects to SPICE server ?

[Daniel P. Berrange]

On Wed, Jan 25, 2017 at 05:59:44PM +0000, Daniel P. Berrange wrote:
> On Wed, Jan 25, 2017 at 12:38:09PM -0500, Frediano Ziglio wrote:
> > > 
> > > On Wed, Jan 25, 2017 at 12:23:35PM -0500, Frediano Ziglio wrote:
> > > > > 
> > > > > When used with QEMU at least, both SPICE and VNC live in the same port
> > > > > range (5900+). It is thus not entirely uncommon for a user to mistakenly
> > > > > connect to a SPICE server with a VNC client, or vica-verca.
> > > > > 
> > > > > When connecting to VNC server with a SPICE client, you quickly get an
> > > > > error. This is because the VNC server sends its short greeting and then
> > > > > sees the RedLinkMess from the SPICE client, realizes its garbage and
> > > > > drops the connection.
> > > > > 
> > > > > When connecting to a SPICE server with a VNC client though, you get an
> > > > > indefinite hang. The VNC client is waiting for the VNC greeting, which
> > > > > the SPICE server will never send. The SPICE server is waiting for the
> > > > > RedLinkMess which the VNC client will never send.
> > > > > 
> > > > > Since VNC is a server sends first protocol and SPICE is a client sends
> > > > > first protocol, it would seem this problem is impossible to fix, but
> > > > > I think it is possible to tweak things so it can be more gracefully
> > > > > handled.
> > > > > 
> > > > > In VNC the protocol starts with the follow data sent:
> > > > > 
> > > > >  Server: "RFB 003.008\n"
> > > > >  Client: "RFB 003.008\n"
> > > > > 
> > > > > What I'm thinking is that we could easily change the VNC client so that
> > > > > it will send some bytes first. eg
> > > > > 
> > > > >  Client: "RFB "
> > > > >  Server: "RFB 003.008\n"
> > > > >  Client: "003.008\n"
> > > > > 
> > > > > From the VNC server POV, it'll still be receiving the same 12 bytes from
> > > > > the client - it just happens that 4 of them might arrive a tiny bit
> > > > > earlier than the other 8 of them. IOW nothing should break in the VNC
> > > > > server from this change.
> > > > > 
> > > > > I tried this, but we still get a hang in the SPICE server. The problem
> > > > > is that the SPICE server code is waiting for the full RedLinkMess
> > > > > message to be received before checking its content.
> > > > > 
> > > > > Can we change the SPICE server so that it reads the 'uint32 magic' first,
> > > > > validates that, and then reads the remainder of RedLinkMess ?
> > > > > 
> > > > > This would solve the problem, as the SPICE server would see 'RFB ' as the
> > > > > magic, realize it was garbage and so close the connection. The VNC client
> > > > > which is waiting for the VNC greeting will thus terminate rather than
> > > > > hanging
> > > > > forever.
> > > > > 
> > > > > Regards,
> > > > > Daniel
> > > > 
> > > > Perhaps another solution would be to add a timeout to spice server so
> > > > after, say, 2/3 seconds it would close the connection.
> > > > 
> > > > This would work even without changing the VNC client.
> > > 
> > > I'm not a fan of timeouts because this could negatively impact on
> > > genuine spice clients which hit a latency spike on the network
> > > for whatever reason. The incremental read of RedLinkMess by constrast
> > > is guaranteed to not have any chance of negative impact on spice clients.
> > > 
> > > Regards,
> > > Daniel
> > 
> > So you would like something like that:
> > 
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 8db70ee..50098df 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -2264,12 +2264,6 @@ static void reds_handle_read_header_done(void *opaque)
> >      header->minor_version = GUINT32_FROM_LE(header->minor_version);
> >      header->size = GUINT32_FROM_LE(header->size);
> >  
> > -    if (header->magic != SPICE_MAGIC) {
> > -        reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
> > -        reds_link_free(link);
> > -        return;
> > -    }
> > -
> >      if (header->major_version != SPICE_VERSION_MAJOR) {
> >          if (header->major_version > 0) {
> >              reds_send_link_error(link, SPICE_LINK_ERR_VERSION_MISMATCH);
> > @@ -2296,13 +2290,31 @@ static void reds_handle_read_header_done(void *opaque)
> >                             link);
> >  }
> >  
> > +static void reds_handle_read_magic_done(void *opaque)
> > +{
> > +    RedLinkInfo *link = (RedLinkInfo *)opaque;
> > +    const SpiceLinkHeader *header = &link->link_header;
> > +
> > +    if (header->magic != SPICE_MAGIC) {
> > +        reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
> > +        reds_link_free(link);
> > +        return;
> > +    }
> > +
> > +    reds_stream_async_read(link->stream,
> > +                           ((uint8_t *)&link->link_header) + sizeof(header->magic),
> > +                           sizeof(SpiceLinkHeader) - sizeof(header->magic),
> > +                           reds_handle_read_header_done,
> > +                           link);
> > +}
> > +
> >  static void reds_handle_new_link(RedLinkInfo *link)
> >  {
> >      reds_stream_set_async_error_handler(link->stream, reds_handle_link_error);
> >      reds_stream_async_read(link->stream,
> >                             (uint8_t *)&link->link_header,
> > -                           sizeof(SpiceLinkHeader),
> > -                           reds_handle_read_header_done,
> > +                           sizeof(link->link_header.magic),
> > +                           reds_handle_read_magic_done,
> >                             link);
> >  }
> >  
> > 
> > ?
> > 
> > I don't think the extra code execute is much of a problem.
> 
> Looks reasonable - let me give it a test locally to make sure the result
> is actually what I expect it to be.

Yes, that works out nicely when combined with my vnc client fix.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|