[Spice-devel] [PATCH spice-server v2 1/2] stream-device: handle cursor from device
Christophe de Dinechin
cdupontd at redhat.com
Fri Feb 9 12:20:02 UTC 2018
> 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
> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list