[Spice-commits] 8 commits - server/char_device.c server/char_device.h server/reds.c server/red_worker.c server/smartcard.c
Yonit Halperin
yhalperi at kemper.freedesktop.org
Mon Nov 26 08:16:51 PST 2012
server/char_device.c | 68 +++++++++++++++++++++++++++++++++++++++-----------
server/char_device.h | 1
server/red_worker.c | 69 +++++++++++++++++++++++++++++++--------------------
server/reds.c | 24 ++++++++++-------
server/smartcard.c | 2 -
5 files changed, 111 insertions(+), 53 deletions(-)
New commits:
commit 4c1a2ad3f1b9d9ebdd578c5eaccf58ca845362fe
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Fri Nov 16 15:11:54 2012 -0500
red_worker.c: fix memory corruption when data from client is bigger than 1024 bytes
Previously, there was no check for the size of the message received from
the client, and all messages were read into a buffer of size 1024.
However, migration data can be bigger than 1024. In such cases, memory
corruption occurred.
diff --git a/server/red_worker.c b/server/red_worker.c
index d27aa7e..54cad53 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -1597,12 +1597,24 @@ static uint8_t *common_alloc_recv_buf(RedChannelClient *rcc, uint16_t type, uint
{
CommonChannel *common = SPICE_CONTAINEROF(rcc->channel, CommonChannel, base);
+ /* SPICE_MSGC_MIGRATE_DATA is the only client message whose size is dynamic */
+ if (type == SPICE_MSGC_MIGRATE_DATA) {
+ return spice_malloc(size);
+ }
+
+ if (size > RECIVE_BUF_SIZE) {
+ spice_critical("unexpected message size %u (max is %d)", size, RECIVE_BUF_SIZE);
+ return NULL;
+ }
return common->recv_buf;
}
static void common_release_recv_buf(RedChannelClient *rcc, uint16_t type, uint32_t size,
uint8_t* msg)
{
+ if (type == SPICE_MSGC_MIGRATE_DATA) {
+ free(msg);
+ }
}
#define CLIENT_PIXMAPS_CACHE
commit 16b38ec84ecc47880282aacc99eeaa6becc3eac7
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Thu Nov 15 15:18:08 2012 -0500
red_worker.c: fix not sending all pending messages when the device is stopped
red_wait_outgoing_item only waits till the currently outgoing msg is
completely sent.
red_wait_outgoing_items does the same for multi-clients. handle_dev_stop erroneously called
red_wait_outgoing_items, instead of waiting till all the items in the
pipes are sent.
This waiting is necessary because after drawables are sent to the client, we release them from the
device. The device might have been stopped due to moving to the non-live
phase of migration. Accessing the device memory during this phase can lead
to inconsistencies.
Also, MSG_MIGRATE should be the last message sent to the client, before
MSG_MIGRATE_DATA. Due to this bug, msgs were marshalled and sent after
handle_dev_stop and after handle_dev_display_migrate which sometimes led
to the release of surfaces, and inserting MSG_DISPLAY_DESTROY_SURFACE
after MSG_MIGRATE.
This patch also removes the calls to red_wait_outgoing_items, from
dev_flush_surfaces. They were unnecessary.
diff --git a/server/red_worker.c b/server/red_worker.c
index 092d45c..d27aa7e 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -2068,7 +2068,6 @@ static void red_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
}
static void red_wait_outgoing_item(RedChannelClient *rcc);
-static void red_wait_outgoing_items(RedChannel *channel);
static void red_clear_surface_drawables_from_pipes(RedWorker *worker, int surface_id,
int force, int wait_for_outgoing_item)
@@ -10605,37 +10604,39 @@ static void red_wait_outgoing_item(RedChannelClient *rcc)
}
}
-static void rcc_shutdown_if_blocked(RedChannelClient *rcc)
+static void rcc_shutdown_if_pending_send(RedChannelClient *rcc)
{
- if (red_channel_client_blocked(rcc)) {
+ if (red_channel_client_blocked(rcc) || rcc->pipe_size > 0) {
red_channel_client_shutdown(rcc);
} else {
spice_assert(red_channel_client_no_item_being_sent(rcc));
}
}
-static void red_wait_outgoing_items(RedChannel *channel)
+static void red_wait_all_sent(RedChannel *channel)
{
uint64_t end_time;
- int blocked;
-
- if (!red_channel_any_blocked(channel)) {
- return;
- }
+ uint32_t max_pipe_size;
+ int blocked = FALSE;
end_time = red_now() + DETACH_TIMEOUT;
- spice_info("blocked");
- do {
+ red_channel_push(channel);
+ while (((max_pipe_size = red_channel_max_pipe_size(channel)) ||
+ (blocked = red_channel_any_blocked(channel))) &&
+ red_now() < end_time) {
+ spice_debug("pipe-size %u blocked %d", max_pipe_size, blocked);
usleep(DETACH_SLEEP_DURATION);
red_channel_receive(channel);
red_channel_send(channel);
- } while ((blocked = red_channel_any_blocked(channel)) && red_now() < end_time);
+ red_channel_push(channel);
+ }
- if (blocked) {
- spice_warning("timeout");
- red_channel_apply_clients(channel, rcc_shutdown_if_blocked);
- } else {
+ if (max_pipe_size || blocked) {
+ spice_printerr("timeout: pending out messages exist (pipe-size %u, blocked %d)",
+ max_pipe_size, blocked);
+ red_channel_apply_clients(channel, rcc_shutdown_if_pending_send);
+ } else {
spice_assert(red_channel_no_item_being_sent(channel));
}
}
@@ -10843,7 +10844,7 @@ static inline void red_cursor_reset(RedWorker *worker)
if (!worker->cursor_channel->common.during_target_migrate) {
red_pipes_add_verb(&worker->cursor_channel->common.base, SPICE_MSG_CURSOR_RESET);
}
- red_wait_outgoing_items(&worker->cursor_channel->common.base);
+ red_wait_all_sent(&worker->cursor_channel->common.base);
}
}
@@ -11108,8 +11109,6 @@ static void dev_flush_surfaces(RedWorker *worker)
{
flush_all_qxl_commands(worker);
flush_all_surfaces(worker);
- red_wait_outgoing_items(&worker->display_channel->common.base);
- red_wait_outgoing_items(&worker->cursor_channel->common.base);
}
void handle_dev_flush_surfaces(void *opaque, void *payload)
@@ -11135,8 +11134,13 @@ void handle_dev_stop(void *opaque, void *payload)
worker->running = FALSE;
red_display_clear_glz_drawables(worker->display_channel);
flush_all_surfaces(worker);
- red_wait_outgoing_items(&worker->display_channel->common.base);
- red_wait_outgoing_items(&worker->cursor_channel->common.base);
+ /* todo: when the waiting is expected to take long (slow connection and
+ * overloaded pipe), don't wait, and in case of migration,
+ * purge the pipe, send destroy_all_surfaces
+ * to the client (there is no such message right now), and start
+ * from scratch on the destination side */
+ red_wait_all_sent(&worker->display_channel->common.base);
+ red_wait_all_sent(&worker->cursor_channel->common.base);
}
static int display_channel_wait_for_migrate_data(DisplayChannel *display)
commit 7785a005d43b5ad6e8fe1c9409a8d9d767e3eb00
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue Nov 13 13:14:16 2012 -0500
smartcard.c: avoid marshalling migration data with reference to a memory that might be released before send has completed
The current solution just copy the buffer. Currently data that is read
from the guest is always copied before sending it to the client. When we
will have ref count for these buffers, we can also use it for marshalling
the migration data.
diff --git a/server/smartcard.c b/server/smartcard.c
index a7e81d5..f1e6244 100644
--- a/server/smartcard.c
+++ b/server/smartcard.c
@@ -508,7 +508,7 @@ static void smartcard_channel_send_migrate_data(RedChannelClient *rcc,
spice_marshaller_add_uint8(m, state->reader_added);
spice_marshaller_add_uint32(m, state->buf_used);
m2 = spice_marshaller_get_ptr_submarshaller(m, 0);
- spice_marshaller_add_ref(m2, state->buf, state->buf_used);
+ spice_marshaller_add(m2, state->buf, state->buf_used);
spice_debug("reader added %d partial read size %u", state->reader_added, state->buf_used);
}
}
commit d8bad0f999da6ccb4aebcc0763a46b2c0624a5a5
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue Nov 13 12:19:32 2012 -0500
red_worker.c: fix marshalling of migration data
fix calling spice_marhsaller_add_ref with memory on stack
diff --git a/server/red_worker.c b/server/red_worker.c
index 18ac949..092d45c 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -8433,7 +8433,7 @@ static void display_channel_marshall_migrate_data_surfaces(DisplayChannelClient
*num_surfaces_created = 0;
for (i = 0; i < NUM_SURFACES; i++) {
SpiceRect lossy_rect;
- SpiceMigrateDataRect lossy_rect_marshall;
+
if (!dcc->surface_client_created[i]) {
continue;
}
@@ -8444,11 +8444,10 @@ static void display_channel_marshall_migrate_data_surfaces(DisplayChannelClient
continue;
}
region_extents(&dcc->surface_client_lossy_region[i], &lossy_rect);
- lossy_rect_marshall.left = lossy_rect.left;
- lossy_rect_marshall.top = lossy_rect.top;
- lossy_rect_marshall.right = lossy_rect.right;
- lossy_rect_marshall.bottom = lossy_rect.bottom;
- spice_marshaller_add_ref(m2, (uint8_t *)&lossy_rect_marshall, sizeof(lossy_rect_marshall));
+ spice_marshaller_add_int32(m2, lossy_rect.left);
+ spice_marshaller_add_int32(m2, lossy_rect.top);
+ spice_marshaller_add_int32(m2, lossy_rect.right);
+ spice_marshaller_add_int32(m2, lossy_rect.bottom);
}
}
commit ea97fbb6296ae5f6938f6319c7c07bae5b319739
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue Nov 13 11:51:59 2012 -0500
reds.c: fix calls to spice_marshaller_add_ref with ptr to memory that might be released before sending
diff --git a/server/reds.c b/server/reds.c
index 98c8706..b99d01f 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -1171,16 +1171,20 @@ void reds_marshall_migrate_data(SpiceMarshaller *m)
spice_marshaller_add_uint32(m, SPICE_MIGRATE_DATA_MAIN_VERSION);
if (!vdagent) {
+ uint8_t *null_agent_mig_data;
+
spice_assert(!agent_state->base); /* MSG_AGENT_CONNECTED_TOKENS is supported by the client
(see spice_server_migrate_connect), so SpiceCharDeviceState
is destroyed when the agent is disconnected and
there is no need to track the client tokens
(see reds_reset_vdp) */
spice_char_device_state_migrate_data_marshall_empty(m);
- spice_marshaller_add_ref(m,
- (uint8_t *)&mig_data + sizeof(SpiceMigrateDataCharDevice),
- sizeof(SpiceMigrateDataMain) - sizeof(SpiceMigrateDataCharDevice)
- );
+ null_agent_mig_data = spice_marshaller_reserve_space(m,
+ sizeof(SpiceMigrateDataMain) -
+ sizeof(SpiceMigrateDataCharDevice));
+ memset(null_agent_mig_data,
+ 0,
+ sizeof(SpiceMigrateDataMain) - sizeof(SpiceMigrateDataCharDevice));
return;
}
@@ -1196,7 +1200,7 @@ void reds_marshall_migrate_data(SpiceMarshaller *m)
mig_data.agent2client.msg_header_done = FALSE;
mig_data.agent2client.msg_header_partial_len = 0;
- spice_assert(!agent_state->read_filter.msg_data_to_read );
+ spice_assert(!agent_state->read_filter.msg_data_to_read);
} else {
mig_data.agent2client.chunk_header_size = sizeof(VDIChunkHeader);
mig_data.agent2client.chunk_header.size = agent_state->message_recive_len;
@@ -1214,14 +1218,14 @@ void reds_marshall_migrate_data(SpiceMarshaller *m)
}
}
spice_marshaller_add_uint32(m, mig_data.agent2client.chunk_header_size);
- spice_marshaller_add_ref(m,
- (uint8_t *)&mig_data.agent2client.chunk_header,
- sizeof(VDIChunkHeader));
+ spice_marshaller_add(m,
+ (uint8_t *)&mig_data.agent2client.chunk_header,
+ sizeof(VDIChunkHeader));
spice_marshaller_add_uint8(m, mig_data.agent2client.msg_header_done);
spice_marshaller_add_uint32(m, mig_data.agent2client.msg_header_partial_len);
m2 = spice_marshaller_get_ptr_submarshaller(m, 0);
- spice_marshaller_add_ref(m2, agent_state->current_read_buf->data,
- mig_data.agent2client.msg_header_partial_len);
+ spice_marshaller_add(m2, agent_state->current_read_buf->data,
+ mig_data.agent2client.msg_header_partial_len);
spice_marshaller_add_uint32(m, mig_data.agent2client.msg_remaining);
spice_marshaller_add_uint8(m, mig_data.agent2client.msg_filter_result);
commit 0ca75b02350522bedb7b98ab679be9e3851cca76
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Mon Nov 19 17:22:51 2012 -0500
char_device.c: when the state is destroyed, also free the buffer that is being written to the device
diff --git a/server/char_device.c b/server/char_device.c
index d7f5360..1d5bed9 100644
--- a/server/char_device.c
+++ b/server/char_device.c
@@ -689,6 +689,9 @@ void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
core->timer_remove(char_dev->write_to_dev_timer);
write_buffers_queue_free(&char_dev->write_queue);
write_buffers_queue_free(&char_dev->write_bufs_pool);
+ if (char_dev->cur_write_buf) {
+ spice_char_device_write_buffer_free(char_dev->cur_write_buf);
+ }
while (!ring_is_empty(&char_dev->clients)) {
RingItem *item = ring_get_tail(&char_dev->clients);
commit d6b3f73102926c299934c5845bfc26f30af5c719
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue Nov 13 10:39:16 2012 -0500
char_device.c: add ref count for write-to-device buffers
The ref count is used in order to keep buffers that were in the write
queue and now are part of migration data, in case the char_device state
is destroyed before we complete sending the migration data.
diff --git a/server/char_device.c b/server/char_device.c
index 141ec88..d7f5360 100644
--- a/server/char_device.c
+++ b/server/char_device.c
@@ -86,6 +86,14 @@ typedef struct SpiceCharDeviceMsgToClientItem {
SpiceCharDeviceMsgToClient *msg;
} SpiceCharDeviceMsgToClientItem;
+static void spice_char_device_write_buffer_free(SpiceCharDeviceWriteBuffer *buf)
+{
+ if (--buf->refs == 0) {
+ free(buf->buf);
+ free(buf);
+ }
+}
+
static void write_buffers_queue_free(Ring *write_queue)
{
while (!ring_is_empty(write_queue)) {
@@ -94,18 +102,21 @@ static void write_buffers_queue_free(Ring *write_queue)
ring_remove(item);
buf = SPICE_CONTAINEROF(item, SpiceCharDeviceWriteBuffer, link);
- free(buf->buf);
- free(buf);
+ spice_char_device_write_buffer_free(buf);
}
}
static void spice_char_device_write_buffer_pool_add(SpiceCharDeviceState *dev,
SpiceCharDeviceWriteBuffer *buf)
{
- buf->buf_used = 0;
- buf->origin = WRITE_BUFFER_ORIGIN_NONE;
- buf->client = NULL;
- ring_add(&dev->write_bufs_pool, &buf->link);
+ if (buf->refs == 1) {
+ buf->buf_used = 0;
+ buf->origin = WRITE_BUFFER_ORIGIN_NONE;
+ buf->client = NULL;
+ ring_add(&dev->write_bufs_pool, &buf->link);
+ } else {
+ --buf->refs;
+ }
}
static void spice_char_device_client_send_queue_free(SpiceCharDeviceState *dev,
@@ -530,6 +541,7 @@ static SpiceCharDeviceWriteBuffer *__spice_char_device_write_buffer_get(SpiceCha
}
ret->token_price = migrated_data_tokens ? migrated_data_tokens : 1;
+ ret->refs = 1;
return ret;
error:
ring_add(&dev->write_bufs_pool, &ret->link);
@@ -542,6 +554,15 @@ SpiceCharDeviceWriteBuffer *spice_char_device_write_buffer_get(SpiceCharDeviceSt
{
return __spice_char_device_write_buffer_get(dev, client, size, 0);
}
+
+static SpiceCharDeviceWriteBuffer *spice_char_device_write_buffer_ref(SpiceCharDeviceWriteBuffer *write_buf)
+{
+ spice_assert(write_buf);
+
+ write_buf->refs++;
+ return write_buf;
+}
+
void spice_char_device_write_buffer_add(SpiceCharDeviceState *dev,
SpiceCharDeviceWriteBuffer *write_buf)
{
@@ -677,7 +698,7 @@ void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
spice_char_device_client_free(char_dev, dev_client);
}
char_dev->running = FALSE;
-
+
spice_char_device_state_unref(char_dev);
}
@@ -826,6 +847,13 @@ void spice_char_device_state_migrate_data_marshall_empty(SpiceMarshaller *m)
mig_data->connected = FALSE;
}
+static void migrate_data_marshaller_write_buffer_free(uint8_t *data, void *opaque)
+{
+ SpiceCharDeviceWriteBuffer *write_buf = (SpiceCharDeviceWriteBuffer *)opaque;
+
+ spice_char_device_write_buffer_free(write_buf);
+}
+
void spice_char_device_state_migrate_data_marshall(SpiceCharDeviceState *dev,
SpiceMarshaller *m)
{
@@ -857,8 +885,10 @@ void spice_char_device_state_migrate_data_marshall(SpiceCharDeviceState *dev,
if (dev->cur_write_buf) {
uint32_t buf_remaining = dev->cur_write_buf->buf + dev->cur_write_buf->buf_used -
dev->cur_write_buf_pos;
-
- spice_marshaller_add_ref(m2, dev->cur_write_buf_pos, buf_remaining);
+ spice_marshaller_add_ref_full(m2, dev->cur_write_buf_pos, buf_remaining,
+ migrate_data_marshaller_write_buffer_free,
+ spice_char_device_write_buffer_ref(dev->cur_write_buf)
+ );
*write_to_dev_size_ptr += buf_remaining;
if (dev->cur_write_buf->origin == WRITE_BUFFER_ORIGIN_CLIENT) {
spice_assert(dev->cur_write_buf->client == client_state->client);
@@ -870,7 +900,10 @@ void spice_char_device_state_migrate_data_marshall(SpiceCharDeviceState *dev,
SpiceCharDeviceWriteBuffer *write_buf;
write_buf = SPICE_CONTAINEROF(item, SpiceCharDeviceWriteBuffer, link);
- spice_marshaller_add_ref(m2, write_buf->buf, write_buf->buf_used);
+ spice_marshaller_add_ref_full(m2, write_buf->buf, write_buf->buf_used,
+ migrate_data_marshaller_write_buffer_free,
+ spice_char_device_write_buffer_ref(write_buf)
+ );
*write_to_dev_size_ptr += write_buf->buf_used;
if (write_buf->origin == WRITE_BUFFER_ORIGIN_CLIENT) {
spice_assert(write_buf->client == client_state->client);
diff --git a/server/char_device.h b/server/char_device.h
index d6d75e3..8bfe4ec 100644
--- a/server/char_device.h
+++ b/server/char_device.h
@@ -73,6 +73,7 @@ typedef struct SpiceCharDeviceWriteBuffer {
uint32_t buf_size;
uint32_t buf_used;
uint32_t token_price;
+ uint32_t refs;
} SpiceCharDeviceWriteBuffer;
typedef void SpiceCharDeviceMsgToClient;
commit 4cd4e7cf19d7fc074510685a95255a3ed785b577
Author: Yonit Halperin <yhalperi at redhat.com>
Date: Tue Nov 13 10:38:34 2012 -0500
char_device.c: fix call to spice_marshaller_add_ref with memory on stack
rhbz#862352
diff --git a/server/char_device.c b/server/char_device.c
index ac2632d..141ec88 100644
--- a/server/char_device.c
+++ b/server/char_device.c
@@ -816,12 +816,14 @@ void spice_char_device_wakeup(SpiceCharDeviceState *dev)
void spice_char_device_state_migrate_data_marshall_empty(SpiceMarshaller *m)
{
- SpiceMigrateDataCharDevice mig_data;
-
- memset(&mig_data, 0, sizeof(mig_data));
- mig_data.version = SPICE_MIGRATE_DATA_CHAR_DEVICE_VERSION;
- mig_data.connected = FALSE;
- spice_marshaller_add_ref(m, (uint8_t *)&mig_data, sizeof(SpiceMigrateDataCharDevice));
+ SpiceMigrateDataCharDevice *mig_data;
+
+ spice_debug(NULL);
+ mig_data = (SpiceMigrateDataCharDevice *)spice_marshaller_reserve_space(m,
+ sizeof(*mig_data));
+ memset(mig_data, 0, sizeof(*mig_data));
+ mig_data->version = SPICE_MIGRATE_DATA_CHAR_DEVICE_VERSION;
+ mig_data->connected = FALSE;
}
void spice_char_device_state_migrate_data_marshall(SpiceCharDeviceState *dev,
More information about the Spice-commits
mailing list