> 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.
>
>> 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.
>
>> - 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.
>
>>
>>> + 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”.
> 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?
>
>>
>>
>>> +
>>> + // 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.
>
>>
>>> + 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.
> 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.
>
>>
>>> +
>>> 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
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel