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

[Jonathon Jongsma]

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. 

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