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

[Frediano Ziglio]

> 
> 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 ?
why a buffer 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