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

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

 



> 
> > On 9 Feb 2018, at 14:20, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> > 
> >>> 
> >>> On 9 Feb 2018, at 10:10, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> >>> 
> >>> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> >>> ---
> >>> 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 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.
> 
> Not a problem. Replace the wrapper I suggested with one that takes the inner
> handler as a function pointer. Really easy. But right now, that’s the
> minority case, so you should not optimize for the rare case as you did.
> 

Is not an optimization, but yes, make sense and actually your is an
optimization (it shrink only on cursor set).

> > 
> >> - 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.
> 
> OK.
> 
> > 
> >> Finally, I suppose that g_realloc may return NULL, however unlikely. That
> >> should be tested.
> >> 
> > 
> > g_realloc never returns NULL, it calls abort.
> 
> That’s a crazy way to deal with out-of-memory errors. I was hoping it was not
> that crazy.
> 

I complained years ago about it when Qemu moved to Glib but I didn't
have much followers.

> > 
> >> 
> >>> +                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”.
> 
> Sorry, I was confused by “max_cursor_set_size”, which I initially read as
> “max size set by the cursor”, since this is a valid way to parse it English.
> You can fix that by replacing “set”, which is not very useful, by “msg” or
> “msg_set”.
> 

Didn't get the full name suggestion.
Not a native English speaker, maybe somebody else can help here.

> 
> > 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.
> 
> Does it have to be? “Cursor size is invalid” would do, no?
> 

Ah ah... yes. Previous version had only an if with just NULL
(which I think is "Protocol error").

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

> > 
> >> 
> >>> +        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.
> 
> No, just make the wrapper generic and add a function pointer. Or add the “if”
> code after calling the handlers in the switch statement above if you prefer.
> 

Make sense. I think there's already that if.

> > 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).
> 
> Would work too.
> 

This would also share the code to check for size.
But looks like a bit overwhelming, can be a follow up.

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

I'll put the shrink after the handler in the if then.

Frediano
_______________________________________________
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]