Re: [Spice-devel] [PATCH spice-server v2 1/2] stream-device: handle cursor from device

[Frediano Ziglio]

> 
> I'm late to the party, but a one-line commit log for a patch adding 160
> lines of code is *not* acceptable.
> 
> Christophe
> 

Yes, was a mistake.

Frediano

> On Fri, Feb 09, 2018 at 09:10:48AM +, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/stream-device.c | 169
> >  +++--
> >  1 file changed, 162 insertions(+), 7 deletions(-)
> > 
> > Changes since v1:
> > - add some comments with some implementation explanation;
> > - better computation of max cursor size.
> > 
> > diff --git a/server/stream-device.c b/server/stream-device.c
> > index 735f2933..a5606d6a 100644
> > --- a/server/stream-device.c
> > +++ b/server/stream-device.c
> > @@ -23,6 +23,7 @@
> >  
> >  #include "char-device.h"
> >  #include "stream-channel.h"
> > +#include "cursor-channel.h"
> >  #include "reds.h"
> >  
> >  #define TYPE_STREAM_DEVICE stream_device_get_type()
> > @@ -45,13 +46,16 @@ struct StreamDevice {
> >  union {
> >  StreamMsgFormat format;
> >  StreamMsgCapabilities capabilities;
> > +StreamMsgCursorSet cursor_set;
> >  uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> > -} msg;
> > +} *msg;
> >  uint32_t msg_pos;
> > +uint32_t msg_len;
> >  bool has_error;
> >  bool opened;
> >  bool flow_stopped;
> >  StreamChannel *stream_channel;
> > +CursorChannel *cursor_channel;
> >  };
> >  
> >  struct StreamDeviceClass {
> > @@ -66,7 +70,7 @@ G_DEFINE_TYPE(StreamDevice, stream_device,
> > RED_TYPE_CHAR_DEVICE)
> >  typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance
> >  *sin)
> >  SPICE_GNUC_WARN_UNUSED_RESULT;
> >  
> > -static StreamMsgHandler handle_msg_format, handle_msg_data;
> > +static StreamMsgHandler handle_msg_format, handle_msg_data,
> > handle_msg_cursor_set;
> >  
> >  static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance
> >  *sin,
> > const char *error_msg)
> > SPICE_GNUC_WARN_UNUSED_RESULT;
> > @@ -108,6 +112,16 @@ stream_device_read_msg_from_dev(RedCharDevice *self,
> > SpiceCharDeviceInstance *si
> >  dev->hdr.type = GUINT16_FROM_LE(dev->hdr.type);
> >  dev->hdr.size = GUINT32_FROM_LE(dev->hdr.size);
> >  dev->msg_pos = 0;
> > +// reallocate to the minimum.
> > +// Currently the only message that requires resizing is
> > +// the cursor shape which is not expected to be sent so
> > +// often.
> > +// Avoid to use dev->hdr.size as this allows easily DoS
> > +// against the server.
> > +if (dev->msg_len > sizeof(*dev->msg)) {
> > +dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
> > +dev->msg_len = sizeof(*dev->msg);
> > +}
> >  }
> >  }
> >  
> > @@ -122,6 +136,9 @@ stream_device_read_msg_from_dev(RedCharDevice *self,
> > SpiceCharDeviceInstance *si
> >  case STREAM_TYPE_DATA:
> >  handled = handle_msg_data(dev, sin);
> >  break;
> > +case STREAM_TYPE_CURSOR_SET:
> > +handled = handle_msg_cursor_set(dev, sin);
> > +break;
> >  case STREAM_TYPE_CAPABILITIES:
> >  /* FIXME */
> >  default:
> > @@ -194,7 +211,7 @@ handle_msg_format(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >  spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> >  }
> >  
> > -int n = sif->read(sin, dev->msg.buf + dev->msg_pos,
> > sizeof(StreamMsgFormat) - dev->msg_pos);
> > +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);
> >  }
> > @@ -205,9 +222,9 @@ handle_msg_format(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >  return false;
> >  }
> >  
> > -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, >msg.format);
> > +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, >msg->format);
> >  return true;
> >  }
> >  
> > @@ -238,6 +255,116 @@ handle_msg_data(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >  return dev->hdr.size == 0;
> >  }
> >  
> > +/*
> > + * Returns number of bits required for a pixel of a given cursor type.
> > + *
> > + * Take into account mask bits.
> > + * Returns 0 on error.
> > + */
> > +static unsigned int
> > +get_cursor_type_bits(unsigned int cursor_type)
> > +{
> > +switch (cursor_type) {
> > +case SPICE_CURSOR_TYPE_ALPHA:
> > +// RGBA
> > +return 32;
> > +case 

Re: [Spice-devel] [PATCH spice-server v2 1/2] stream-device: handle cursor from device

[Christophe Fergeau]

I'm late to the party, but a one-line commit log for a patch adding 160
lines of code is *not* acceptable. 

Christophe

On Fri, Feb 09, 2018 at 09:10:48AM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/stream-device.c | 169 
> +++--
>  1 file changed, 162 insertions(+), 7 deletions(-)
> 
> Changes since v1:
> - add some comments with some implementation explanation;
> - better computation of max cursor size.
> 
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 735f2933..a5606d6a 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -23,6 +23,7 @@
>  
>  #include "char-device.h"
>  #include "stream-channel.h"
> +#include "cursor-channel.h"
>  #include "reds.h"
>  
>  #define TYPE_STREAM_DEVICE stream_device_get_type()
> @@ -45,13 +46,16 @@ struct StreamDevice {
>  union {
>  StreamMsgFormat format;
>  StreamMsgCapabilities capabilities;
> +StreamMsgCursorSet cursor_set;
>  uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> -} msg;
> +} *msg;
>  uint32_t msg_pos;
> +uint32_t msg_len;
>  bool has_error;
>  bool opened;
>  bool flow_stopped;
>  StreamChannel *stream_channel;
> +CursorChannel *cursor_channel;
>  };
>  
>  struct StreamDeviceClass {
> @@ -66,7 +70,7 @@ G_DEFINE_TYPE(StreamDevice, stream_device, 
> RED_TYPE_CHAR_DEVICE)
>  typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance 
> *sin)
>  SPICE_GNUC_WARN_UNUSED_RESULT;
>  
> -static StreamMsgHandler handle_msg_format, handle_msg_data;
> +static StreamMsgHandler handle_msg_format, handle_msg_data, 
> handle_msg_cursor_set;
>  
>  static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance 
> *sin,
> const char *error_msg) 
> SPICE_GNUC_WARN_UNUSED_RESULT;
> @@ -108,6 +112,16 @@ stream_device_read_msg_from_dev(RedCharDevice *self, 
> SpiceCharDeviceInstance *si
>  dev->hdr.type = GUINT16_FROM_LE(dev->hdr.type);
>  dev->hdr.size = GUINT32_FROM_LE(dev->hdr.size);
>  dev->msg_pos = 0;
> +// reallocate to the minimum.
> +// Currently the only message that requires resizing is
> +// the cursor shape which is not expected to be sent so
> +// often.
> +// Avoid to use dev->hdr.size as this allows easily DoS
> +// against the server.
> +if (dev->msg_len > sizeof(*dev->msg)) {
> +dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
> +dev->msg_len = sizeof(*dev->msg);
> +}
>  }
>  }
>  
> @@ -122,6 +136,9 @@ stream_device_read_msg_from_dev(RedCharDevice *self, 
> SpiceCharDeviceInstance *si
>  case STREAM_TYPE_DATA:
>  handled = handle_msg_data(dev, sin);
>  break;
> +case STREAM_TYPE_CURSOR_SET:
> +handled = handle_msg_cursor_set(dev, sin);
> +break;
>  case STREAM_TYPE_CAPABILITIES:
>  /* FIXME */
>  default:
> @@ -194,7 +211,7 @@ handle_msg_format(StreamDevice *dev, 
> SpiceCharDeviceInstance *sin)
>  spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
>  }
>  
> -int n = sif->read(sin, dev->msg.buf + dev->msg_pos, 
> sizeof(StreamMsgFormat) - dev->msg_pos);
> +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);
>  }
> @@ -205,9 +222,9 @@ handle_msg_format(StreamDevice *dev, 
> SpiceCharDeviceInstance *sin)
>  return false;
>  }
>  
> -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, >msg.format);
> +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, >msg->format);
>  return true;
>  }
>  
> @@ -238,6 +255,116 @@ handle_msg_data(StreamDevice *dev, 
> SpiceCharDeviceInstance *sin)
>  return dev->hdr.size == 0;
>  }
>  
> +/*
> + * Returns number of bits required for a pixel of a given cursor type.
> + *
> + * Take into account mask bits.
> + * Returns 0 on error.
> + */
> +static unsigned int
> +get_cursor_type_bits(unsigned int cursor_type)
> +{
> +switch (cursor_type) {
> +case SPICE_CURSOR_TYPE_ALPHA:
> +// RGBA
> +return 32;
> +case SPICE_CURSOR_TYPE_COLOR24:
> +// RGB + bitmask
> +return 24 + 1;
> +case SPICE_CURSOR_TYPE_COLOR32:
> +// RGBx + bitmask
> +return 32 + 1;
> +default:
> +return 0;
> +}
> +}
> +
> +static RedCursorCmd *
> +stream_msg_cursor_set_to_cursor_cmd(const StreamMsgCursorSet *msg, size_t 
> msg_size)

Re: [Spice-devel] [PATCH spice-server v2 1/2] stream-device: handle cursor from device

[Frediano Ziglio]

> 
> > On 9 Feb 2018, at 17:59, Frediano Ziglio  wrote:
> > 
>  
>  
> > +
> > +// read part of the message till we get all
> > +if (dev->msg_len < dev->hdr.size) {
> > +dev->msg = g_realloc(dev->msg, dev->hdr.size);
> > +dev->msg_len = dev->hdr.size;
> > +}
> > +int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size
> > -
> > dev->msg_pos);
> > +if (n <= 0) {
> > +return false;
> > +}
> > +dev->msg_pos += n;
> > +if (dev->msg_pos != dev->hdr.size) {
>  
>  Is it assumed you can all read in one go? I’m surprized there is a
>  “while”
>  loop for reading the header (which is small) but no while loop for
>  reading
>  the payload which is larger).
>  
> >>> 
> >>> When we return false is not an error, just there's no data left next time
> >>> will add more data. So there's no while but there's a loop anyway.
> >>> I suppose similarly we could avoid the while in the other function.
> >> 
> >> Oh, so you are saying that you think it’s OK to loop ouside, truncate the
> >> data that you g_realloc’d, and then start over?
> >> 
> >> Does not work. Say you started with 1K in buffer, then go to this inner
> >> read
> >> here, which does a g_realloc and reads 10K. Then we re-enter the above
> >> function, so I believe we will truncate to 1K again, and then re-extend to
> >> 10K and start reading at 10K. We lost 9K of data, unless I’m mistaken.
> >> 
> > 
> > No, the header is not read again until all message is processed.
> > There are also tests for this.
> 
> This is not what I am talking about. I’m saying you read this, where H is
> header and D is data.
> 
> You first read header, let’s say four items.
> 
> []
> 
> Then you realloc, which puts some undefined garbage in the area the data:
> 
> [][]
> 
> Then you read data, get 4 elements:
> 
> [][]
> 
> So you restart in the outer loop, where you truncate the header
> 

why truncate the header?

The 

while (dev->hdr_pos < sizeof(dev->hdr)) {

condition is not taken and we get back reading the partial message.

> []
> 
> and reallocate
> 
> [][]
> 
> and restart reading, not sure if we have reset msg_pos or not, so you get
> either
> 
> [][]
> 
> or
> 
> [][]
> 
> 
> None of which is correct. I’ve not seen in the code how we prevent this if we
> don’t loop in the inner read.
> 
> Maybe I missed the part that makes us restart correctly, but where did we
> keep the data?
> 
> 
> Regards
> Christophe
> 
> 

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v2 1/2] stream-device: handle cursor from device

[Christophe de Dinechin]

> On 9 Feb 2018, at 17:59, Frediano Ziglio  wrote:
> 
 
 
> +
> +// read part of the message till we get all
> +if (dev->msg_len < dev->hdr.size) {
> +dev->msg = g_realloc(dev->msg, dev->hdr.size);
> +dev->msg_len = dev->hdr.size;
> +}
> +int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size -
> dev->msg_pos);
> +if (n <= 0) {
> +return false;
> +}
> +dev->msg_pos += n;
> +if (dev->msg_pos != dev->hdr.size) {
 
 Is it assumed you can all read in one go? I’m surprized there is a “while”
 loop for reading the header (which is small) but no while loop for reading
 the payload which is larger).
 
>>> 
>>> When we return false is not an error, just there's no data left next time
>>> will add more data. So there's no while but there's a loop anyway.
>>> I suppose similarly we could avoid the while in the other function.
>> 
>> Oh, so you are saying that you think it’s OK to loop ouside, truncate the
>> data that you g_realloc’d, and then start over?
>> 
>> Does not work. Say you started with 1K in buffer, then go to this inner read
>> here, which does a g_realloc and reads 10K. Then we re-enter the above
>> function, so I believe we will truncate to 1K again, and then re-extend to
>> 10K and start reading at 10K. We lost 9K of data, unless I’m mistaken.
>> 
> 
> No, the header is not read again until all message is processed.
> There are also tests for this.

This is not what I am talking about. I’m saying you read this, where H is 
header and D is data.

You first read header, let’s say four items.

[]

Then you realloc, which puts some undefined garbage in the area the data:

[][]

Then you read data, get 4 elements:

[][]

So you restart in the outer loop, where you truncate the header

[]

and reallocate

[][]

and restart reading, not sure if we have reset msg_pos or not, so you get either

[][]

or

[][]


None of which is correct. I’ve not seen in the code how we prevent this if we 
don’t loop in the inner read.

Maybe I missed the part that makes us restart correctly, but where did we keep 
the data?


Regards
Christophe

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v2 1/2] stream-device: handle cursor from device

[Frediano Ziglio]

> 
> > On 9 Feb 2018, at 14:20, Frediano Ziglio  wrote:
> > 
> >>> 
> >>> On 9 Feb 2018, at 10:10, Frediano Ziglio  wrote:
> >>> 
> >>> Signed-off-by: Frediano Ziglio 
> >>> ---
> >>> server/stream-device.c | 169
> >>> +++--
> >>> 1 file changed, 162 insertions(+), 7 deletions(-)
> >>> 
> >>> Changes since v1:
> >>> - add some comments with some implementation explanation;
> >>> - better computation of max cursor size.
> >>> 
> >>> diff --git a/server/stream-device.c b/server/stream-device.c
> >>> index 735f2933..a5606d6a 100644
> >>> --- a/server/stream-device.c
> >>> +++ b/server/stream-device.c
> >>> @@ -23,6 +23,7 @@
> >>> 
> >>> #include "char-device.h"
> >>> #include "stream-channel.h"
> >>> +#include "cursor-channel.h"
> >>> #include "reds.h"
> >>> 
> >>> #define TYPE_STREAM_DEVICE stream_device_get_type()
> >>> @@ -45,13 +46,16 @@ struct StreamDevice {
> >>>union {
> >>>StreamMsgFormat format;
> >>>StreamMsgCapabilities capabilities;
> >>> +StreamMsgCursorSet cursor_set;
> >>>uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> >>> -} msg;
> >>> +} *msg;
> >>>uint32_t msg_pos;
> >>> +uint32_t msg_len;
> >>>bool has_error;
> >>>bool opened;
> >>>bool flow_stopped;
> >>>StreamChannel *stream_channel;
> >>> +CursorChannel *cursor_channel;
> >>> };
> >>> 
> >>> struct StreamDeviceClass {
> >>> @@ -66,7 +70,7 @@ G_DEFINE_TYPE(StreamDevice, stream_device,
> >>> RED_TYPE_CHAR_DEVICE)
> >>> typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance
> >>> *sin)
> >>>SPICE_GNUC_WARN_UNUSED_RESULT;
> >>> 
> >>> -static StreamMsgHandler handle_msg_format, handle_msg_data;
> >>> +static StreamMsgHandler handle_msg_format, handle_msg_data,
> >>> handle_msg_cursor_set;
> >>> 
> >>> static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance
> >>> *sin,
> >>>   const char *error_msg)
> >>>   SPICE_GNUC_WARN_UNUSED_RESULT;
> >>> @@ -108,6 +112,16 @@ stream_device_read_msg_from_dev(RedCharDevice *self,
> >>> SpiceCharDeviceInstance *si
> >>>dev->hdr.type = GUINT16_FROM_LE(dev->hdr.type);
> >>>dev->hdr.size = GUINT32_FROM_LE(dev->hdr.size);
> >> 
> >> [Not in your patch, but looking at the code made me wonder]
> >> 
> >> How do we re-sync reads when n <= 0 and we return NULL? Do we close
> >> everything and reopen? I’m asking because I don’t see
> >> 
> > 
> > Is weird but is the CharDevice interface, we should return an
> > item to be sent back, in this case we are not using this "feature"
> > so we return NULL.
> > Here currently the read never returns < 0 (Qemu code) and 0 means
> > no data in the queue. Obviously safer to check also for <0 in case
> > Qemu decide to change it (I suppose they will also notify the close
> > event we already handle).
> 
> OK, thanks
> 
> > 
> >> 
> >>>dev->msg_pos = 0;
> >>> +// reallocate to the minimum.
> >> (Why not capitalized?)
> >>> +// Currently the only message that requires resizing is
> >>> +// the cursor shape which is not expected to be sent so
> >>> +// often.
> >>> +// Avoid to use dev->hdr.size as this allows easily DoS
> >>> +// against the server.
> >> 
> >> This is very strangely truncated. If our limit is 100 chars per line, what
> >> about:
> >> 
> >> +// Reallocate to the minimum.
> >> +// Currently the only message that requires resizing is the
> >> cursor shape,
> >> +// which is not expected to be sent so often.
> >> +// Avoid to use dev->hdr.size as this allows easily DoS
> >> against
> >> the server.
> >> 
> > 
> > I'll have a look at my settings, didn't notice, thanks
> > 
> >> (reads easier if you cut at grammatical boundaries)
> >> 
> >> 
> >>> +if (dev->msg_len > sizeof(*dev->msg)) {
> >>> +dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
> >> 
> >> As it is, it looks to me like you are allocating 1K
> >> (STREAM_MSG_CAPABILITIES_MAX_BYTES) for packets that are typically much
> >> smaller. If you are concerned about DoS, why not just use
> >> min(dev->hdr.size,
> >> sizeof(*dev->msg)) ?
> >> 
> > 
> > The min would just cause to call g_realloc more often, you are not
> > preventing
> > a DoS. The DoS happens if the guest try to send a large packet (GB) not a
> > small one.
> 
> This is the reason I used ‘min’. But as I said, the code as written is not
> clear at all. If the purpose is to restore to default size after handlers
> that reallocate, then that’s how it should be done.
> 

I don't want to resize to potentially 0 bytes so I'll have to resize again
for next small message.
Do you mean  min(dev->hdr.size, dev->msg_len)  perhaps ?

> > 
> >> So I guess I’m not very sure what the objective is here. It looks 

Re: [Spice-devel] [PATCH spice-server v2 1/2] stream-device: handle cursor from device

[Christophe de Dinechin]

> On 9 Feb 2018, at 14:20, Frediano Ziglio  wrote:
> 
>>> 
>>> On 9 Feb 2018, at 10:10, Frediano Ziglio  wrote:
>>> 
>>> Signed-off-by: Frediano Ziglio 
>>> ---
>>> server/stream-device.c | 169
>>> +++--
>>> 1 file changed, 162 insertions(+), 7 deletions(-)
>>> 
>>> Changes since v1:
>>> - add some comments with some implementation explanation;
>>> - better computation of max cursor size.
>>> 
>>> diff --git a/server/stream-device.c b/server/stream-device.c
>>> index 735f2933..a5606d6a 100644
>>> --- a/server/stream-device.c
>>> +++ b/server/stream-device.c
>>> @@ -23,6 +23,7 @@
>>> 
>>> #include "char-device.h"
>>> #include "stream-channel.h"
>>> +#include "cursor-channel.h"
>>> #include "reds.h"
>>> 
>>> #define TYPE_STREAM_DEVICE stream_device_get_type()
>>> @@ -45,13 +46,16 @@ struct StreamDevice {
>>>union {
>>>StreamMsgFormat format;
>>>StreamMsgCapabilities capabilities;
>>> +StreamMsgCursorSet cursor_set;
>>>uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
>>> -} msg;
>>> +} *msg;
>>>uint32_t msg_pos;
>>> +uint32_t msg_len;
>>>bool has_error;
>>>bool opened;
>>>bool flow_stopped;
>>>StreamChannel *stream_channel;
>>> +CursorChannel *cursor_channel;
>>> };
>>> 
>>> struct StreamDeviceClass {
>>> @@ -66,7 +70,7 @@ G_DEFINE_TYPE(StreamDevice, stream_device,
>>> RED_TYPE_CHAR_DEVICE)
>>> typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance
>>> *sin)
>>>SPICE_GNUC_WARN_UNUSED_RESULT;
>>> 
>>> -static StreamMsgHandler handle_msg_format, handle_msg_data;
>>> +static StreamMsgHandler handle_msg_format, handle_msg_data,
>>> handle_msg_cursor_set;
>>> 
>>> static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance
>>> *sin,
>>>   const char *error_msg)
>>>   SPICE_GNUC_WARN_UNUSED_RESULT;
>>> @@ -108,6 +112,16 @@ stream_device_read_msg_from_dev(RedCharDevice *self,
>>> SpiceCharDeviceInstance *si
>>>dev->hdr.type = GUINT16_FROM_LE(dev->hdr.type);
>>>dev->hdr.size = GUINT32_FROM_LE(dev->hdr.size);
>> 
>> [Not in your patch, but looking at the code made me wonder]
>> 
>> How do we re-sync reads when n <= 0 and we return NULL? Do we close
>> everything and reopen? I’m asking because I don’t see
>> 
> 
> Is weird but is the CharDevice interface, we should return an
> item to be sent back, in this case we are not using this "feature"
> so we return NULL.
> Here currently the read never returns < 0 (Qemu code) and 0 means
> no data in the queue. Obviously safer to check also for <0 in case
> Qemu decide to change it (I suppose they will also notify the close
> event we already handle).

OK, thanks

> 
>> 
>>>dev->msg_pos = 0;
>>> +// reallocate to the minimum.
>> (Why not capitalized?)
>>> +// Currently the only message that requires resizing is
>>> +// the cursor shape which is not expected to be sent so
>>> +// often.
>>> +// Avoid to use dev->hdr.size as this allows easily DoS
>>> +// against the server.
>> 
>> This is very strangely truncated. If our limit is 100 chars per line, what
>> about:
>> 
>> +// Reallocate to the minimum.
>> +// Currently the only message that requires resizing is the
>> cursor shape,
>> +// which is not expected to be sent so often.
>> +// Avoid to use dev->hdr.size as this allows easily DoS against
>> the server.
>> 
> 
> I'll have a look at my settings, didn't notice, thanks
> 
>> (reads easier if you cut at grammatical boundaries)
>> 
>> 
>>> +if (dev->msg_len > sizeof(*dev->msg)) {
>>> +dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
>> 
>> As it is, it looks to me like you are allocating 1K
>> (STREAM_MSG_CAPABILITIES_MAX_BYTES) for packets that are typically much
>> smaller. If you are concerned about DoS, why not just use min(dev->hdr.size,
>> sizeof(*dev->msg)) ?
>> 
> 
> The min would just cause to call g_realloc more often, you are not preventing
> a DoS. The DoS happens if the guest try to send a large packet (GB) not a 
> small one.

This is the reason I used ‘min’. But as I said, the code as written is not 
clear at all. If the purpose is to restore to default size after handlers that 
reallocate, then that’s how it should be done.

> 
>> So I guess I’m not very sure what the objective is here. It looks like the
>> code did not really decide whether the allocation was to be done here or to
>> be done by the handler function. I suggest that we
>> 
>> - Either decide that the buffer is by default 1024 bytes, and we only realloc
>> for specific messages, in which case the handler ought to do the
>> reallocation, then clean-up
> 
> Not much extensible, every possible handler that has to support more that
> the default would 

Re: [Spice-devel] [PATCH spice-server v2 1/2] stream-device: handle cursor from device

[Frediano Ziglio]

> 
> > On 9 Feb 2018, at 10:10, Frediano Ziglio  wrote:
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> > server/stream-device.c | 169
> > +++--
> > 1 file changed, 162 insertions(+), 7 deletions(-)
> > 
> > Changes since v1:
> > - add some comments with some implementation explanation;
> > - better computation of max cursor size.
> > 
> > diff --git a/server/stream-device.c b/server/stream-device.c
> > index 735f2933..a5606d6a 100644
> > --- a/server/stream-device.c
> > +++ b/server/stream-device.c
> > @@ -23,6 +23,7 @@
> > 
> > #include "char-device.h"
> > #include "stream-channel.h"
> > +#include "cursor-channel.h"
> > #include "reds.h"
> > 
> > #define TYPE_STREAM_DEVICE stream_device_get_type()
> > @@ -45,13 +46,16 @@ struct StreamDevice {
> > union {
> > StreamMsgFormat format;
> > StreamMsgCapabilities capabilities;
> > +StreamMsgCursorSet cursor_set;
> > uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> > -} msg;
> > +} *msg;
> > uint32_t msg_pos;
> > +uint32_t msg_len;
> > bool has_error;
> > bool opened;
> > bool flow_stopped;
> > StreamChannel *stream_channel;
> > +CursorChannel *cursor_channel;
> > };
> > 
> > struct StreamDeviceClass {
> > @@ -66,7 +70,7 @@ G_DEFINE_TYPE(StreamDevice, stream_device,
> > RED_TYPE_CHAR_DEVICE)
> > typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance
> > *sin)
> > SPICE_GNUC_WARN_UNUSED_RESULT;
> > 
> > -static StreamMsgHandler handle_msg_format, handle_msg_data;
> > +static StreamMsgHandler handle_msg_format, handle_msg_data,
> > handle_msg_cursor_set;
> > 
> > static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance
> > *sin,
> >const char *error_msg)
> >SPICE_GNUC_WARN_UNUSED_RESULT;
> > @@ -108,6 +112,16 @@ stream_device_read_msg_from_dev(RedCharDevice *self,
> > SpiceCharDeviceInstance *si
> > dev->hdr.type = GUINT16_FROM_LE(dev->hdr.type);
> > dev->hdr.size = GUINT32_FROM_LE(dev->hdr.size);
> 
> [Not in your patch, but looking at the code made me wonder]
> 
> How do we re-sync reads when n <= 0 and we return NULL? Do we close
> everything and reopen? I’m asking because I don’t see
> 

Is weird but is the CharDevice interface, we should return an
item to be sent back, in this case we are not using this "feature"
so we return NULL.
Here currently the read never returns < 0 (Qemu code) and 0 means
no data in the queue. Obviously safer to check also for <0 in case
Qemu decide to change it (I suppose they will also notify the close
event we already handle).

> 
> > dev->msg_pos = 0;
> > +// reallocate to the minimum.
> (Why not capitalized?)
> > +// Currently the only message that requires resizing is
> > +// the cursor shape which is not expected to be sent so
> > +// often.
> > +// Avoid to use dev->hdr.size as this allows easily DoS
> > +// against the server.
> 
> This is very strangely truncated. If our limit is 100 chars per line, what
> about:
> 
> +// Reallocate to the minimum.
> +// Currently the only message that requires resizing is the
> cursor shape,
> +// which is not expected to be sent so often.
> +// Avoid to use dev->hdr.size as this allows easily DoS against
> the server.
> 

I'll have a look at my settings, didn't notice, thanks

> (reads easier if you cut at grammatical boundaries)
> 
> 
> > +if (dev->msg_len > sizeof(*dev->msg)) {
> > +dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
> 
> As it is, it looks to me like you are allocating 1K
> (STREAM_MSG_CAPABILITIES_MAX_BYTES) for packets that are typically much
> smaller. If you are concerned about DoS, why not just use min(dev->hdr.size,
> sizeof(*dev->msg)) ?
> 

The min would just cause to call g_realloc more often, you are not preventing
a DoS. The DoS happens if the guest try to send a large packet (GB) not a small 
one.

> So I guess I’m not very sure what the objective is here. It looks like the
> code did not really decide whether the allocation was to be done here or to
> be done by the handler function. I suggest that we
> 
> - Either decide that the buffer is by default 1024 bytes, and we only realloc
> for specific messages, in which case the handler ought to do the
> reallocation, then clean-up

Not much extensible, every possible handler that has to support more that
the default would have to do it.

> - Or that the allocation has to be done here, in which case I would compute a
> max size which is either 1024 for all messages, or larger for cursor
> messages (unless I’m mistaken, 1024 is not enough for all cursors, it’s
> 16x16x4 or 32x32x1, seems low to me)
> 

Currently cursors can be 1024x1024 (I think is a reasonable limit) so 

Re: [Spice-devel] [PATCH spice-server v2 1/2] stream-device: handle cursor from device

[Christophe de Dinechin]

> On 9 Feb 2018, at 10:10, Frediano Ziglio  wrote:
> 
> Signed-off-by: Frediano Ziglio 
> ---
> server/stream-device.c | 169 +++--
> 1 file changed, 162 insertions(+), 7 deletions(-)
> 
> Changes since v1:
> - add some comments with some implementation explanation;
> - better computation of max cursor size.
> 
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 735f2933..a5606d6a 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -23,6 +23,7 @@
> 
> #include "char-device.h"
> #include "stream-channel.h"
> +#include "cursor-channel.h"
> #include "reds.h"
> 
> #define TYPE_STREAM_DEVICE stream_device_get_type()
> @@ -45,13 +46,16 @@ struct StreamDevice {
> union {
> StreamMsgFormat format;
> StreamMsgCapabilities capabilities;
> +StreamMsgCursorSet cursor_set;
> uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> -} msg;
> +} *msg;
> uint32_t msg_pos;
> +uint32_t msg_len;
> bool has_error;
> bool opened;
> bool flow_stopped;
> StreamChannel *stream_channel;
> +CursorChannel *cursor_channel;
> };
> 
> struct StreamDeviceClass {
> @@ -66,7 +70,7 @@ G_DEFINE_TYPE(StreamDevice, stream_device, 
> RED_TYPE_CHAR_DEVICE)
> typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> SPICE_GNUC_WARN_UNUSED_RESULT;
> 
> -static StreamMsgHandler handle_msg_format, handle_msg_data;
> +static StreamMsgHandler handle_msg_format, handle_msg_data, 
> handle_msg_cursor_set;
> 
> static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance 
> *sin,
>const char *error_msg) 
> SPICE_GNUC_WARN_UNUSED_RESULT;
> @@ -108,6 +112,16 @@ stream_device_read_msg_from_dev(RedCharDevice *self, 
> SpiceCharDeviceInstance *si
> dev->hdr.type = GUINT16_FROM_LE(dev->hdr.type);
> dev->hdr.size = GUINT32_FROM_LE(dev->hdr.size);

[Not in your patch, but looking at the code made me wonder]

How do we re-sync reads when n <= 0 and we return NULL? Do we close everything 
and reopen? I’m asking because I don’t see 


> dev->msg_pos = 0;
> +// reallocate to the minimum.
(Why not capitalized?)
> +// Currently the only message that requires resizing is
> +// the cursor shape which is not expected to be sent so
> +// often.
> +// Avoid to use dev->hdr.size as this allows easily DoS
> +// against the server.

This is very strangely truncated. If our limit is 100 chars per line, what 
about:

+// Reallocate to the minimum.
+// Currently the only message that requires resizing is the cursor 
shape,
+// which is not expected to be sent so often.
+// Avoid to use dev->hdr.size as this allows easily DoS against 
the server.

(reads easier if you cut at grammatical boundaries)


> +if (dev->msg_len > sizeof(*dev->msg)) {
> +dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));

As it is, it looks to me like you are allocating 1K 
(STREAM_MSG_CAPABILITIES_MAX_BYTES) for packets that are typically much 
smaller. If you are concerned about DoS, why not just use min(dev->hdr.size, 
sizeof(*dev->msg)) ?

So I guess I’m not very sure what the objective is here. It looks like the code 
did not really decide whether the allocation was to be done here or to be done 
by the handler function. I suggest that we

- Either decide that the buffer is by default 1024 bytes, and we only realloc 
for specific messages, in which case the handler ought to do the reallocation, 
then clean-up
- Or that the allocation has to be done here, in which case I would compute a 
max size which is either 1024 for all messages, or larger for cursor messages 
(unless I’m mistaken, 1024 is not enough for all cursors, it’s 16x16x4 or 
32x32x1, seems low to me)

Finally, I suppose that g_realloc may return NULL, however unlikely. That 
should be tested.


> +dev->msg_len = sizeof(*dev->msg);
> +}
> }
> }
> 
> @@ -122,6 +136,9 @@ stream_device_read_msg_from_dev(RedCharDevice *self, 
> SpiceCharDeviceInstance *si
> case STREAM_TYPE_DATA:
> handled = handle_msg_data(dev, sin);
> break;
> +case STREAM_TYPE_CURSOR_SET:
> +handled = handle_msg_cursor_set(dev, sin);
> +break;
> case STREAM_TYPE_CAPABILITIES:
> /* FIXME */
> default:
> @@ -194,7 +211,7 @@ handle_msg_format(StreamDevice *dev, 
> SpiceCharDeviceInstance *sin)
> spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> }
> 
> -int n = sif->read(sin, dev->msg.buf + dev->msg_pos, 
> sizeof(StreamMsgFormat) - dev->msg_pos);
> +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);
>   

[Spice-devel] [PATCH spice-server v2 1/2] stream-device: handle cursor from device

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/stream-device.c | 169 +++--
 1 file changed, 162 insertions(+), 7 deletions(-)

Changes since v1:
- add some comments with some implementation explanation;
- better computation of max cursor size.

diff --git a/server/stream-device.c b/server/stream-device.c
index 735f2933..a5606d6a 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -23,6 +23,7 @@
 
 #include "char-device.h"
 #include "stream-channel.h"
+#include "cursor-channel.h"
 #include "reds.h"
 
 #define TYPE_STREAM_DEVICE stream_device_get_type()
@@ -45,13 +46,16 @@ struct StreamDevice {
 union {
 StreamMsgFormat format;
 StreamMsgCapabilities capabilities;
+StreamMsgCursorSet cursor_set;
 uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
-} msg;
+} *msg;
 uint32_t msg_pos;
+uint32_t msg_len;
 bool has_error;
 bool opened;
 bool flow_stopped;
 StreamChannel *stream_channel;
+CursorChannel *cursor_channel;
 };
 
 struct StreamDeviceClass {
@@ -66,7 +70,7 @@ G_DEFINE_TYPE(StreamDevice, stream_device, 
RED_TYPE_CHAR_DEVICE)
 typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance *sin)
 SPICE_GNUC_WARN_UNUSED_RESULT;
 
-static StreamMsgHandler handle_msg_format, handle_msg_data;
+static StreamMsgHandler handle_msg_format, handle_msg_data, 
handle_msg_cursor_set;
 
 static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance *sin,
const char *error_msg) 
SPICE_GNUC_WARN_UNUSED_RESULT;
@@ -108,6 +112,16 @@ stream_device_read_msg_from_dev(RedCharDevice *self, 
SpiceCharDeviceInstance *si
 dev->hdr.type = GUINT16_FROM_LE(dev->hdr.type);
 dev->hdr.size = GUINT32_FROM_LE(dev->hdr.size);
 dev->msg_pos = 0;
+// reallocate to the minimum.
+// Currently the only message that requires resizing is
+// the cursor shape which is not expected to be sent so
+// often.
+// Avoid to use dev->hdr.size as this allows easily DoS
+// against the server.
+if (dev->msg_len > sizeof(*dev->msg)) {
+dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
+dev->msg_len = sizeof(*dev->msg);
+}
 }
 }
 
@@ -122,6 +136,9 @@ stream_device_read_msg_from_dev(RedCharDevice *self, 
SpiceCharDeviceInstance *si
 case STREAM_TYPE_DATA:
 handled = handle_msg_data(dev, sin);
 break;
+case STREAM_TYPE_CURSOR_SET:
+handled = handle_msg_cursor_set(dev, sin);
+break;
 case STREAM_TYPE_CAPABILITIES:
 /* FIXME */
 default:
@@ -194,7 +211,7 @@ handle_msg_format(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
 }
 
-int n = sif->read(sin, dev->msg.buf + dev->msg_pos, 
sizeof(StreamMsgFormat) - dev->msg_pos);
+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);
 }
@@ -205,9 +222,9 @@ handle_msg_format(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 return false;
 }
 
-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, >msg.format);
+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, >msg->format);
 return true;
 }
 
@@ -238,6 +255,116 @@ handle_msg_data(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 return dev->hdr.size == 0;
 }
 
+/*
+ * Returns number of bits required for a pixel of a given cursor type.
+ *
+ * Take into account mask bits.
+ * Returns 0 on error.
+ */
+static unsigned int
+get_cursor_type_bits(unsigned int cursor_type)
+{
+switch (cursor_type) {
+case SPICE_CURSOR_TYPE_ALPHA:
+// RGBA
+return 32;
+case SPICE_CURSOR_TYPE_COLOR24:
+// RGB + bitmask
+return 24 + 1;
+case SPICE_CURSOR_TYPE_COLOR32:
+// RGBx + bitmask
+return 32 + 1;
+default:
+return 0;
+}
+}
+
+static RedCursorCmd *
+stream_msg_cursor_set_to_cursor_cmd(const StreamMsgCursorSet *msg, size_t 
msg_size)
+{
+RedCursorCmd *cmd = g_new0(RedCursorCmd, 1);
+cmd->type = QXL_CURSOR_SET;
+cmd->u.set.position.x = 0; // TODO
+cmd->u.set.position.y = 0; // TODO
+cmd->u.set.visible = 1; // TODO
+SpiceCursor *cursor = >u.set.shape;
+cursor->header.unique = 0;
+cursor->header.type = msg->type;
+cursor->header.width = GUINT16_FROM_LE(msg->width);
+cursor->header.height = GUINT16_FROM_LE(msg->height);
+cursor->header.hot_spot_x =