[Spice-devel] [PATCH spice-server] StreamDevice: Handle incomplete reads of StreamMsgFormat

[Frediano Ziglio]

> 
> On Wed, 2017-11-01 at 05:02 -0400, Frediano Ziglio wrote:
> > > 
> > > This is currently unlikely to happen since we communicate over a
> > > pipe
> > > and the pipe buffer is sufficiently large to avoid splitting the
> > > message. But for completeness, we should handle this scenario.
> > > 
> > > Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> > > ---
> > >  server/stream-device.c | 33 +++++++++++++++++++++++----------
> > >  1 file changed, 23 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/server/stream-device.c b/server/stream-device.c
> > > index fc5b50659..259527246 100644
> > > --- a/server/stream-device.c
> > > +++ b/server/stream-device.c
> > > @@ -42,6 +42,12 @@ struct StreamDevice {
> > >  
> > >      StreamDevHeader hdr;
> > >      uint8_t hdr_pos;
> > > +    union {
> > > +        StreamMsgFormat format;
> > > +        StreamMsgCapabilities capabilities;
> > > +        uint8_t buf[1024];
> > 
> > why capabilities ?
> 
> Just because it's another potential message (that we're not handling
> yet). I was just preparing for the future when we implement this
> message. I considered adding StreamMsgData to this union as well, but
> thought that perhaps 16*1024 buffer was larger than we wanted within
> the object?
> 
> > why a buffer of 1024 ?
> 
> The 1024 was chosen simply because the StreamMsgCapabilities message
> specified to have a limit of 1024.
> 

Would be great to have a mnemonic in stream-device.h.

> > 
> > > +    } msg;
> > > +    uint8_t msg_pos;
> > 
> > uint8_t is not big enough for the size of msg.
> > 
> > >      bool has_error;
> > >      bool opened;
> > >      bool flow_stopped;
> > > @@ -155,20 +161,27 @@ handle_msg_invalid(StreamDevice *dev,
> > > SpiceCharDeviceInstance *sin, const char *
> > >  static bool
> > >  handle_msg_format(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> > >  {
> > > -    StreamMsgFormat fmt;
> > >      SpiceCharDeviceInterface *sif =
> > > spice_char_device_get_interface(sin);
> > > -    int n = sif->read(sin, (uint8_t *) &fmt, sizeof(fmt));
> > > -    if (n == 0) {
> > > -        return false;
> > > -    }
> > > -    if (n != sizeof(fmt)) {
> > > +
> > > +    spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> > > +    spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> > > +
> > > +    int n = sif->read(sin, dev->msg.buf + dev->msg_pos,
> > > sizeof(StreamMsgFormat) - dev->msg_pos);
> > > +    if (n < 0) {
> > >          return handle_msg_invalid(dev, sin, NULL);
> > >      }
> > > -    fmt.width = GUINT32_FROM_LE(fmt.width);
> > > -    fmt.height = GUINT32_FROM_LE(fmt.height);
> > > -    stream_channel_change_format(dev->stream_channel, &fmt);
> > >  
> > > -    return true;
> > > +    dev->msg_pos += n;
> > > +
> > > +    if (dev->msg_pos >= sizeof(StreamMsgFormat)) {
> > 
> > I would do a "if (dev->msg_pos < sizeof(StreamMsgFormat)) return
> > false" instead.
> > 
> > > +        dev->msg.format.width = GUINT32_FROM_LE(dev-
> > > >msg.format.width);
> > > +        dev->msg.format.height = GUINT32_FROM_LE(dev-
> > > >msg.format.height);
> > > +        stream_channel_change_format(dev->stream_channel, &dev-
> > > >msg.format);
> > > +        dev->msg_pos = 0;
> > 
> > should this reset be in the main code ? I think so.
> > 
> > > +        return true;
> > > +    }
> > > +
> > > +    return false;
> > >  }
> > >  
> > >  static bool
> > 

Frediano