On Fri, 2018-02-16 at 17:17 -0500, Frediano Ziglio wrote:
> > On Tue, 2018-02-13 at 13:58 +0000, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > ---
> > > server/stream-device.c | 165
> > > ++++++++++++++++++++++++++++++++++++++++++++++---
> > > 1 file changed, 158 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/server/stream-device.c b/server/stream-device.c
> > > index b7553c67..f6fd9108 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;
> > > @@ -121,6 +125,9 @@ stream_device_partial_read(StreamDevice *dev,
> > > SpiceCharDeviceInstance *sin)
> > > 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:
> > > @@ -132,6 +139,14 @@ stream_device_partial_read(StreamDevice
> > > *dev,
> > > SpiceCharDeviceInstance *sin)
> > > * the next message */
> > > if (handled) {
> > > dev->hdr_pos = 0;
> > > +
> > > + // Reallocate message buffer to the minimum.
> > > + // Currently the only message that requires resizing is
> > > the
> > > cursor shape,
> > > + // which is not expected to be sent so often.
> > > + if (dev->msg_len > sizeof(*dev->msg)) {
> > > + dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
> > > + dev->msg_len = sizeof(*dev->msg);
> > > + }
> > > }
> > >
> > > if (handled || dev->has_error) {
> > > @@ -204,7 +219,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);
> > > }
> > > @@ -215,9 +230,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, &dev-
> > > > 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, &dev->msg-
> > > > format);
> > >
> > > return true;
> > > }
> > >
> > > @@ -248,6 +263,114 @@ 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 = &cmd->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 = GUINT16_FROM_LE(msg-
> > > >hot_spot_x);
> > > + cursor->header.hot_spot_y = GUINT16_FROM_LE(msg-
> > > >hot_spot_y);
> > > +
> > > + /* Limit cursor size to prevent DoS */
> > > + if (cursor->header.width > STREAM_MSG_CURSOR_SET_MAX_WIDTH
> > > ||
> > > + cursor->header.height >
> > > STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
> > > + g_free(cmd);
> > > + return NULL;
> > > + }
> > > +
> > > + const unsigned int cursor_bits =
> > > get_cursor_type_bits(cursor-
> > > > header.type);
> > >
> > > + if (cursor_bits == 0) {
> > > + g_free(cmd);
> > > + return NULL;
> > > + }
> > > +
> > > + /* Check that enough data has been sent for the cursor.
> > > + * Note that these computations can't overflow due to sizes
> > > checks
> > > + * above. */
> > > + size_t size_required = cursor->header.width * cursor-
> > > > header.height;
> > >
> > > + size_required = SPICE_ALIGN(size_required * cursor_bits, 8)
> > > /
> > > 8u;
> > > + if (msg_size < sizeof(StreamMsgCursorSet) + size_required) {
> > > + g_free(cmd);
> > > + return NULL;
> > > + }
> > > + cursor->data_size = size_required;
> > > + cursor->data = spice_memdup(msg->data, size_required);
> > > + return cmd;
> > > +}
> > > +
> > > +static bool
> > > +handle_msg_cursor_set(StreamDevice *dev, SpiceCharDeviceInstance
> > > *sin)
> > > +{
> > > + // maximum size taking into account 32 bit image with
> > > bitmask
> > > + const unsigned int max_cursor_set_size =
> > > + sizeof(StreamMsgCursorSet) +
> > > + (STREAM_MSG_CURSOR_SET_MAX_WIDTH * 4 +
> > > (STREAM_MSG_CURSOR_SET_MAX_WIDTH + 7)/8)
> > > + * STREAM_MSG_CURSOR_SET_MAX_HEIGHT;
> >
> >
> > So, in the previous patch, you had:
> >
> > const unsigned int max_cursor_set_size =
> > STREAM_MSG_CURSOR_SET_MAX_WIDTH *
> > STREAM_MSG_CURSOR_SET_MAX_HEIGHT * 5;
> >
> > Ignoring the addition of the 'sizeof(StreamMsgCursorSet)' (which
> > looks
> > correct to me), the size of the data portion has changed
> > significantly:
> >
> > The old code set the max size to
> > 1024 * 1024 * 5 = 5242880
> >
> > The new code sets the max size to
> > (1024*4 + (1024 + 7)/8) * 1024 =>
> > (1024*4 + 128) * 1024 =>
> > 4224 * 1024 = 4325376
> >
> > So you're trying to calculate the maximum size of the data required
> > for
> > the largest image (1024x1024) using the format that requires the
> > largest number of bits per pixel (SPICE_CURSOR_TYPE_COLOR32 = 33
> > bits).
> > Furthermore, it looks like you're assuming that the pixels are
> > packed
> > tightly and span byte boundaries, e.g. for pixels A, B and C:
> > AAAAAAAA AAAAAAAA AAAAAAAA AAAAAAAA
> > ABBBBBBB BBBBBBBB BBBBBBBB BBBBBBBB
> > BBCCCCCC CCCCCCCC CCCCCCCC CCCCCCCC
> > CCCDDDDD ....
> >
> > Maybe that's obvious to everyone else, but I feel that it might not
> > be
> > obvious to me when I look at this piece of code a year from now. So
> > I
> > still think a clearer comment above might be good.
> >
> > Maybe:
> >
> > "Calculate the maximum size required to send the pixel data for a
> > cursor that is the maximum size using the format that requires the
> > largest number of bits per pixel (SPICE_CURSOR_TYPE_COLOR32
> > requires 33
> > bits per pixel, see get_cursor_type_bits())"
> >
> > ?
> >
>
> I'll do.
> The *5 was just a size bigger but surely holding a 1024x1024 cursor.
>
> >
> > > +
> > > + SpiceCharDeviceInterface *sif =
> > > spice_char_device_get_interface(sin);
> > > +
> > > + if (dev->hdr.size < sizeof(StreamMsgCursorSet) || dev-
> > > >hdr.size
> > > > max_cursor_set_size) {
> > >
> > > + // we could skip the message but on the other hand the
> > > client
> >
> > Did you mean client here? Or did you mean agent?
> >
>
> I think got confuse by client/server terminology.
>
> What about "guest" ?
Sure, I think "guest" is fine
>
> > > + // is buggy in any case
> > > + return handle_msg_invalid(dev, sin, "Cursor size is
> > > invalid");
> > > + }
> > > +
> > > + // 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) {
> > > + return false;
> > > + }
> > > +
> > > + // transform the message to a cursor command and process it
> > > + RedCursorCmd *cmd =
> > > stream_msg_cursor_set_to_cursor_cmd(&dev-
> > > > msg->cursor_set, dev->msg_pos);
> > >
> > > + if (!cmd) {
> > > + return handle_msg_invalid(dev, sin, NULL);
> > > + }
> > > + cursor_channel_process_cmd(dev->cursor_channel, cmd);
> > > +
> > > + return true;
> > > +}
> > > +
> > > static void
> > > stream_device_send_msg_to_client(RedCharDevice *self,
> > > RedPipeItem
> > > *msg, RedClient *client)
> > > {
> > > @@ -345,6 +468,22 @@ stream_device_dispose(GObject *object)
> > > red_channel_destroy(RED_CHANNEL(dev->stream_channel));
> > > dev->stream_channel = NULL;
> > > }
> > > + if (dev->cursor_channel) {
> > > + // close all current connections and drop the reference
> > > + red_channel_destroy(RED_CHANNEL(dev->cursor_channel));
> > > + dev->cursor_channel = NULL;
> > > + }
> > > +}
> > > +
> > > +static void
> > > +stream_device_finalize(GObject *object)
> > > +{
> > > + StreamDevice *dev = STREAM_DEVICE(object);
> > > +
> > > + g_free(dev->msg);
> > > + dev->msg = NULL;
> > > + dev->msg_len = 0;
> > > + dev->msg_pos = 0;
> > > }
> > >
> > > static void
> > > @@ -355,13 +494,22 @@ allocate_channels(StreamDevice *dev)
> > > }
> > >
> > > SpiceServer* reds =
> > > red_char_device_get_server(RED_CHAR_DEVICE(dev));
> > > + SpiceCoreInterfaceInternal* core =
> > > reds_get_core_interface(reds);
> > >
> > > int id = reds_get_free_channel_id(reds,
> > > SPICE_CHANNEL_DISPLAY);
> > > g_return_if_fail(id >= 0);
> > >
> > > StreamChannel *stream_channel = stream_channel_new(reds,
> > > id);
> > >
> > > + CursorChannel *cursor_channel = cursor_channel_new(reds, id,
> > > core);
> > > + ClientCbs client_cbs = { NULL, };
> > > + client_cbs.connect = (channel_client_connect_proc)
> > > cursor_channel_connect;
> > > + client_cbs.migrate = cursor_channel_client_migrate;
> > > + red_channel_register_client_cbs(RED_CHANNEL(cursor_channel),
> > > &client_cbs, NULL);
> > > + reds_register_channel(reds, RED_CHANNEL(cursor_channel));
> > > +
> > > dev->stream_channel = stream_channel;
> > > + dev->cursor_channel = cursor_channel;
> > >
> > > stream_channel_register_start_cb(stream_channel,
> > > stream_device_stream_start, dev);
> > > stream_channel_register_queue_stat_cb(stream_channel,
> > > stream_device_stream_queue_stat, dev);
> > > @@ -423,6 +571,7 @@ stream_device_class_init(StreamDeviceClass
> > > *klass)
> > > RedCharDeviceClass *char_dev_class =
> > > RED_CHAR_DEVICE_CLASS(klass);
> > >
> > > object_class->dispose = stream_device_dispose;
> > > + object_class->finalize = stream_device_finalize;
> > >
> > > char_dev_class->read_one_msg_from_device =
> > > stream_device_read_msg_from_dev;
> > > char_dev_class->send_msg_to_client =
> > > stream_device_send_msg_to_client;
> > > @@ -432,8 +581,10 @@ stream_device_class_init(StreamDeviceClass
> > > *klass)
> > > }
> > >
> > > static void
> > > -stream_device_init(StreamDevice *self)
> > > +stream_device_init(StreamDevice *dev)
> > > {
> > > + dev->msg = g_malloc(sizeof(*dev->msg));
> > > + dev->msg_len = sizeof(*dev->msg);
> > > }
> > >
> > > static StreamDevice *
> >
> >
> > Aside from comments, I think it looks good.
> >
> > Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> >
> >
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel