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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2018-01-16 at 12:28 +0000, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
>  server/stream-device.c | 154
> ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 147 insertions(+), 7 deletions(-)
> 
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 735f2933..c15742c6 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,10 @@ 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;
> +            if (dev->msg_len > sizeof(*dev->msg)) {
> +                dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
> +                dev->msg_len = sizeof(*dev->msg);
> +            }

So if the last message was bigger than the union (in other words: the
last message was a cursor set command), we downsize it to the size of
the union. But if this command is also a cursor set command, we'll just
realloc it larger again when we call handle_msg_cursor_set(). 

Wouldn't it be better to realloc to the size of dev->hdr.size here
instead (with some upper limit on message size)? That would avoid
unnecessary memory churn.

>          }
>      }
>  
> @@ -122,6 +130,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 +205,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 +216,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;
>  }
>  
> @@ -238,6 +249,107 @@ handle_msg_data(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>      return dev->hdr.size == 0;
>  }
>  
> +/*
> + * Returns number of bits required for 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:
> +        return 32;
> +    case SPICE_CURSOR_TYPE_COLOR24:
> +        return 25;
> +    case SPICE_CURSOR_TYPE_COLOR32:
> +        return 33;
> +    default:
> +        return 0;
> +    }
> +}

Where do these numbers come from?

> +
> +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)
> +{
> +    const unsigned int max_cursor_set_size =
> +        STREAM_MSG_CURSOR_SET_MAX_WIDTH *
> STREAM_MSG_CURSOR_SET_MAX_HEIGHT * 5;

Where does the 5 come from?

> +
> +    SpiceCharDeviceInterface *sif =
> spice_char_device_get_interface(sin);
> +
> +    if (dev->hdr.size >= max_cursor_set_size || dev->hdr.size <
> sizeof(StreamMsgCursorSet)) {
> +        return handle_msg_invalid(dev, sin, NULL);
> +    }

So if the guest sends us a cursor that's larger than our max, we'll go
to an error state and stop streaming. Do we want to handle that more
gracefully?

> +
> +    // 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;
> +    }
> +
> +    // trasform the message to a cursor command and process it

typo: transform

> +    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)
>  {
> @@ -335,6 +447,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
> @@ -345,13 +473,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);
> @@ -413,6 +550,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;
> @@ -422,8 +560,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 *


Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>

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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]