> On 9 Feb 2018, at 10:10, Frediano Ziglio <fzig...@redhat.com> wrote:
> 
> Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
> ---
> 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);
>     }
> @@ -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, &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 +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 = &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 =
> +        (STREAM_MSG_CURSOR_SET_MAX_WIDTH * 4 + 
> (STREAM_MSG_CURSOR_SET_MAX_WIDTH + 7)/8)
> +        * STREAM_MSG_CURSOR_SET_MAX_HEIGHT;

Name is slightly confusing (“set” can be name, adjective, verb). What about 
"max_cursor_msg_size”?

> +
> +    SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> +
> +    if (dev->hdr.size < sizeof(StreamMsgCursorSet)) {
> +        return handle_msg_invalid(dev, sin, NULL);
> +    }
> +    if (dev->hdr.size > max_cursor_set_size) {
> +        // we could skip the message but on the other end the client
> +        // is buggy in any case
> +        return handle_msg_invalid(dev, sin, "Cursor sent is too big");
> +    }

The two conditions are really similar. I would write it as:

    const unsigned int min_cursor_msg_size = sizeof(StreamMsgCursorSet);
    // We could skip the message but on the other end the client is buggy
    if (dev->hdr.size < min_cursor_msg_size || dev->hdr.size > 
max_cursor_msg_size) {
        return handle_msg_invalid(dev, sin, NULL);
    }



> +
> +    // 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).


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

What I would do to manage the memory is wrap like this:

- rename above function as handle_msg_cursor_set_internal

- Add a wrapper that deals with buffer allocation

static bool
handle_msg_cursor_set(StreamDevice *dev, SpiceCharDeviceInstance *sin)
{
    bool handled = handle_msg_cursor_set_internal(dev, sin);
    // Since we may have grown device buffer, truncate it back to normal size
   if (dev->msg_len > sizeof(*dev->msg)) {
       dev->msg_len = sizeof(*dev->msg);
       dev->msg = g_realloc(dev->msg, dev->msg_len);
       if (dev->msg == NULL) { … report … }
    }
    return handled;
}


> +
> static void
> stream_device_send_msg_to_client(RedCharDevice *self, RedPipeItem *msg, 
> RedClient *client)
> {
> @@ -335,6 +462,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 +488,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 +565,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 +575,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 *
> -- 
> 2.14.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

Reply via email to