[Spice-devel] [PATCH 0/2] Make plugin version checking more robust
Christophe Fergeau
cfergeau at redhat.com
Mon Mar 26 16:47:08 UTC 2018
On Mon, Mar 26, 2018 at 11:27:19AM +0200, Christophe de Dinechin wrote:
>
>
> > On 26 Mar 2018, at 11:25, Christophe Fergeau <cfergeau at redhat.com> wrote:
> >
> > Hey,
> >
> > On Fri, Mar 23, 2018 at 01:05:22PM +0100, Christophe de Dinechin wrote:
> >> From: Christophe de Dinechin <dinechin at redhat.com>
> >>
> >> Ensure that plugin version checking cannot be bypassed.
> >> Implement version checking without violating the C++ ODR.
> >> Improve ABI version numbering when incompatibilites are detected post-release.
> >> Add macro to make it easier to generate the ABI interface.
> >>
> >> Christophe de Dinechin (2):
> >> Ensure that plugins cannot bypass version check
> >> Add a macro to deal with the boilerplate of writing a streaming agent
> >> plugin
> >
> > Could this be split in 4 patches each implementing one of these changes?
>
> I don’t see how. Any idea on how this could be done?
I would at least split the change regarding how the versioning is
managed, see attached patch for a proof of concept (most likely needs
some adjustment, no proper logs, ..). The ODR change/ensure that plugin
cannot bypass version check can probably be considered two faces of the
same coin).
Christophe
-------------- next part --------------
From f3b8d49752949c8b24efb3bc6515cb31cc81ebb0 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fziglio at redhat.com>
Date: Fri, 16 Mar 2018 11:31:41 +0100
Subject: [spice-server 1/3] Preparation work
---
server/stream-channel.c | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/server/stream-channel.c b/server/stream-channel.c
index 88f859f6d..3fab28ae1 100644
--- a/server/stream-channel.c
+++ b/server/stream-channel.c
@@ -508,17 +508,10 @@ data_item_free(RedPipeItem *base)
g_free(pipe_item);
}
-void
-stream_channel_send_data(StreamChannel *channel, const void *data, size_t size, uint32_t mm_time)
+static StreamDataItem*
+stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t mm_time)
{
- if (channel->stream_id < 0) {
- // this condition can happen if the guest didn't handle
- // the format stop that we send so think the stream is still
- // started
- return;
- }
-
- RedChannel *red_channel = RED_CHANNEL(channel);
+ stream_channel_unref_data_item(channel);
StreamDataItem *item = g_malloc(sizeof(*item) + size);
red_pipe_item_init_full(&item->base, RED_PIPE_ITEM_TYPE_STREAM_DATA,
@@ -527,9 +520,29 @@ stream_channel_send_data(StreamChannel *channel, const void *data, size_t size,
item->data.base.multi_media_time = mm_time;
item->data.data_size = size;
item->channel = channel;
- stream_channel_update_queue_stat(channel, 1, size);
+
+ return item;
+}
+
+void
+stream_channel_send_data(StreamChannel *channel, const void *data, size_t size, uint32_t mm_time)
+{
+ RedChannel *red_channel = RED_CHANNEL(channel);
+
+ if (channel->stream_id < 0) {
+ // this condition can happen if the guest didn't handle
+ // the format stop that we send so think the stream is still
+ // started
+ return;
+ }
+
+ StreamDataItem *item;
+
+ item = stream_channel_new_data_item(channel, size, mm_time);
+
// TODO try to optimize avoiding the copy
memcpy(item->data.data, data, size);
+ stream_channel_update_queue_stat(channel, 1, size);
red_channel_pipes_add(red_channel, &item->base);
}
--
2.14.3
From c8c204c5482e3bfa0038ea9829fb96923274ee29 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fziglio at redhat.com>
Date: Fri, 16 Mar 2018 11:44:21 +0100
Subject: [spice-server 2/3] more preparation work
---
server/stream-channel.c | 57 +++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 48 insertions(+), 9 deletions(-)
diff --git a/server/stream-channel.c b/server/stream-channel.c
index 3fab28ae1..b5652e5ff 100644
--- a/server/stream-channel.c
+++ b/server/stream-channel.c
@@ -44,6 +44,7 @@
typedef struct StreamChannelClient StreamChannelClient;
typedef struct StreamChannelClientClass StreamChannelClientClass;
+typedef struct StreamDataItem StreamDataItem;
/* we need to inherit from CommonGraphicsChannelClient
* to get buffer handling */
@@ -74,6 +75,10 @@ struct StreamChannel {
StreamQueueStat queue_stat;
+ /* pending partial data item */
+ StreamDataItem *data_item;
+ uint32_t data_item_pos;
+
/* callback to notify when a stream should be started or stopped */
stream_channel_start_proc start_cb;
void *start_opaque;
@@ -104,12 +109,12 @@ typedef struct StreamCreateItem {
SpiceMsgDisplayStreamCreate stream_create;
} StreamCreateItem;
-typedef struct StreamDataItem {
+struct StreamDataItem {
RedPipeItem base;
StreamChannel *channel;
// NOTE: this must be the last field in the structure
SpiceMsgDisplayStreamData data;
-} StreamDataItem;
+};
#define PRIMARY_SURFACE_ID 0
@@ -129,6 +134,16 @@ stream_channel_client_init(StreamChannelClient *client)
client->stream_id = -1;
}
+static void
+stream_channel_unref_data_item(StreamChannel *channel)
+{
+ if (channel->data_item) {
+ red_pipe_item_unref(&channel->data_item->base);
+ channel->data_item = NULL;
+ channel->data_item_pos = 0;
+ }
+}
+
static void
request_new_stream(StreamChannel *channel, StreamMsgStartStop *start)
{
@@ -152,6 +167,7 @@ stream_channel_client_on_disconnect(RedChannelClient *rcc)
channel->stream_id = -1;
channel->width = 0;
channel->height = 0;
+ stream_channel_unref_data_item(channel);
// send stream stop to device
StreamMsgStartStop stop = { 0, };
@@ -424,6 +440,16 @@ stream_channel_constructed(GObject *object)
reds_register_channel(reds, red_channel);
}
+static void
+stream_channel_finalize(GObject *object)
+{
+ StreamChannel *channel = STREAM_CHANNEL(object);
+
+ stream_channel_unref_data_item(channel);
+
+ G_OBJECT_CLASS(stream_channel_parent_class)->finalize(object);
+}
+
static void
stream_channel_class_init(StreamChannelClass *klass)
{
@@ -431,6 +457,7 @@ stream_channel_class_init(StreamChannelClass *klass)
RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
object_class->constructed = stream_channel_constructed;
+ object_class->finalize = stream_channel_finalize;
channel_class->parser = spice_get_client_channel_parser(SPICE_CHANNEL_DISPLAY, NULL);
channel_class->handle_message = handle_message;
@@ -503,7 +530,9 @@ data_item_free(RedPipeItem *base)
{
StreamDataItem *pipe_item = SPICE_UPCAST(StreamDataItem, base);
- stream_channel_update_queue_stat(pipe_item->channel, -1, -pipe_item->data.data_size);
+ if (pipe_item->channel->data_item != pipe_item) {
+ stream_channel_update_queue_stat(pipe_item->channel, -1, -pipe_item->data.data_size);
+ }
g_free(pipe_item);
}
@@ -521,6 +550,9 @@ stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t mm_ti
item->data.data_size = size;
item->channel = channel;
+ channel->data_item = item;
+ channel->data_item_pos = 0;
+
return item;
}
@@ -536,14 +568,20 @@ stream_channel_send_data(StreamChannel *channel, const void *data, size_t size,
return;
}
- StreamDataItem *item;
+ while (size) {
+ StreamDataItem *item = channel->data_item;
- item = stream_channel_new_data_item(channel, size, mm_time);
+ if (!item) {
+ item = stream_channel_new_data_item(channel, size, mm_time);
+ }
- // TODO try to optimize avoiding the copy
- memcpy(item->data.data, data, size);
- stream_channel_update_queue_stat(channel, 1, size);
- red_channel_pipes_add(red_channel, &item->base);
+ size_t copy_size = size;
+ // TODO try to optimize avoiding the copy
+ memcpy(item->data.data, data, copy_size);
+ size -= copy_size;
+ stream_channel_update_queue_stat(channel, 1, size);
+ red_channel_pipes_add(red_channel, &item->base);
+ }
}
void
@@ -583,6 +621,7 @@ stream_channel_reset(StreamChannel *channel)
channel->stream_id = -1;
channel->width = 0;
channel->height = 0;
+ stream_channel_unref_data_item(channel);
if (!red_channel_is_connected(red_channel)) {
return;
--
2.14.3
From 43b50f11428313d395c7c01bb3c93356c76f9ac8 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fziglio at redhat.com>
Date: Wed, 7 Mar 2018 12:01:49 +0000
Subject: [spice-server 3/3] stream-channel: Send the full frame in a single
message
The current implementation of server and client assumes that a single
data message contains an encoded frame.
This is not a problem for most encoding but for MJPEG this causes
the client to fail decoding.
Collapse frame data into a single message before sending to the client.
This is done in the channel as the channel code is responsible to take care
of client protocol details. This allows for instance to support chunked
transfer to client if implemented.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
---
server/red-stream-device.c | 10 ++++++----
server/stream-channel.c | 27 +++++++++++++++++++++++----
server/stream-channel.h | 12 ++++++++++++
3 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/server/red-stream-device.c b/server/red-stream-device.c
index c87c4899d..81336e549 100644
--- a/server/red-stream-device.c
+++ b/server/red-stream-device.c
@@ -271,11 +271,13 @@ handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance *sin)
if (n <= 0) {
break;
}
- // TODO collect all message ??
- // up: we send a single frame together
- // down: guest can cause a crash
- stream_channel_send_data(dev->stream_channel, buf, n, reds_get_mm_time());
+ uint32_t mm_time = reds_get_mm_time();
+ if (dev->msg_pos == 0) {
+ stream_channel_start_data(dev->stream_channel, dev->hdr.size, mm_time);
+ }
+ stream_channel_send_data(dev->stream_channel, buf, n, mm_time);
dev->hdr.size -= n;
+ dev->msg_pos += n;
}
return dev->hdr.size == 0;
diff --git a/server/stream-channel.c b/server/stream-channel.c
index b5652e5ff..75dd1f867 100644
--- a/server/stream-channel.c
+++ b/server/stream-channel.c
@@ -556,6 +556,20 @@ stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t mm_ti
return item;
}
+void
+stream_channel_start_data(StreamChannel *channel, size_t size, uint32_t mm_time)
+{
+ // see stream_channel_send_data comment
+ if (channel->stream_id < 0) {
+ return;
+ }
+
+ // TODO this collects all chunks in a single message
+ // up: we send a single frame together (more compatible)
+ // down: guest can cause a crash due to DoS. As a safe measure we limit the maximum message
+ stream_channel_new_data_item(channel, MIN(size, 32*1024*1024), mm_time);
+}
+
void
stream_channel_send_data(StreamChannel *channel, const void *data, size_t size, uint32_t mm_time)
{
@@ -575,12 +589,17 @@ stream_channel_send_data(StreamChannel *channel, const void *data, size_t size,
item = stream_channel_new_data_item(channel, size, mm_time);
}
- size_t copy_size = size;
+ size_t copy_size = item->data.data_size - channel->data_item_pos;
+ copy_size = MIN(copy_size, size);
// TODO try to optimize avoiding the copy
- memcpy(item->data.data, data, copy_size);
+ memcpy(item->data.data + channel->data_item_pos, data, copy_size);
size -= copy_size;
- stream_channel_update_queue_stat(channel, 1, size);
- red_channel_pipes_add(red_channel, &item->base);
+ channel->data_item_pos += copy_size;
+ if (channel->data_item_pos == item->data.data_size) {
+ channel->data_item = NULL;
+ stream_channel_update_queue_stat(channel, 1, item->data.data_size);
+ red_channel_pipes_add(red_channel, &item->base);
+ }
}
}
diff --git a/server/stream-channel.h b/server/stream-channel.h
index e8bec80bd..18a1bdead 100644
--- a/server/stream-channel.h
+++ b/server/stream-channel.h
@@ -60,6 +60,18 @@ struct StreamMsgStartStop;
void stream_channel_change_format(StreamChannel *channel,
const struct StreamMsgFormat *fmt);
+
+/**
+ * Tell the channel that a new data packet is starting.
+ * This can be used to group all chunks together.
+ */
+void stream_channel_start_data(StreamChannel *channel,
+ size_t size,
+ uint32_t mm_time);
+
+/**
+ * Send to channel a chunk of data.
+ */
void stream_channel_send_data(StreamChannel *channel,
const void *data, size_t size,
uint32_t mm_time);
--
2.14.3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180326/decdd0b9/attachment.sig>
More information about the Spice-devel
mailing list