[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 <fziglio at redhat.com> wrote:
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at 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
> 

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 4Mb+128K.

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

g_realloc never returns NULL, it calls abort.

> 
> > +                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”?
> 

Is the message which is cursor set, was inherit from the protocol messages.
So should be read as "stream message", "cursor set", "max_height".
The constant is defined in stream-device.h (spice-protocol).

I need to add sizeof(StreamMsgCursorSet), I'll fix it

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

Typo "on the other hand".. I'll fix

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

The message sent is different.

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

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

Oh, so basically this is implementing the shrink on handler.
Has the same issue, if another handler would require it you have to
write this code again.
But yes, it would make sure of the shrink.
We could actually write a "shrink_msg(StreamDevice *dev, bool handled)"
function to avoid duplications.
I was also thinking about an array of ranges that the different messages
support so main code could handle resize and shrink and size check
(for instance for StreamMsgFormat which is constant the minimum and
the maximum would be the same).

> 
> > +
> > 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 *

Frediano