diff --git a/.gitignore b/.gitignore index 9238945..36fc37b 100644 --- a/.gitignore +++ b/.gitignore @@ -16,3 +16,4 @@ spice-0.5.3.tar.bz2 /spice-0.12.0.tar.bz2 /spice-0.12.2.tar.bz2 /spice-0.12.3.tar.bz2 +/spice-0.12.4.tar.bz2 diff --git a/0001-red_channel-prevent-adding-and-pushing-pipe-items-af.patch b/0001-red_channel-prevent-adding-and-pushing-pipe-items-af.patch new file mode 100644 index 0000000..aab0d8e --- /dev/null +++ b/0001-red_channel-prevent-adding-and-pushing-pipe-items-af.patch @@ -0,0 +1,96 @@ +From 47e722b85ccd0b6876ca189a3d6f6f05289fe3c3 Mon Sep 17 00:00:00 2001 +From: Yonit Halperin +Date: Wed, 24 Jul 2013 14:54:23 -0400 +Subject: [PATCH 1/8] red_channel: prevent adding and pushing pipe items after + a channel_client has diconnected + +Fixes: leaks of pipe items & "red_client_destroy: assertion `rcc->send_data.size == 0'" + +red_channel_disconnect clears the pipe. It is called only once. After, +it was called, not items should be added to the pipe. + +An example of when this assert can occur: +on_new_cursor_channel (red_worker.c), pushes 2 pipe items. +When it pushes the first pipe item, if the client has disconnected, +it can hit a socket error, and then, red_channel_client_disconnect is called. +The second call to adding a pipe item, will add the item to +the pipe. red_channel_client_pipe_add_type also calls +red_channel_client_push, which will update the send_data.size. +Then, the push will also hit a socket error, but red_channel_client_disconnect +won't clear the pending pipe item again, since it was already called. +When red_client_destory is called, we hit assertion `rcc->send_data.size +== 0'. +Note that if a pipe item is added to the pipe after +red_channel_client_disconnect was called, but without pushing it, +we should hit "spice_assert(rcc->pipe_size == 0)". +--- + server/red_channel.c | 30 ++++++++++++++++++++++++------ + 1 file changed, 24 insertions(+), 6 deletions(-) + +diff --git a/server/red_channel.c b/server/red_channel.c +index 33af388..0d74413 100644 +--- a/server/red_channel.c ++++ b/server/red_channel.c +@@ -1527,9 +1527,23 @@ void red_channel_pipe_item_init(RedChannel *channel, PipeItem *item, int type) + item->type = type; + } + +-void red_channel_client_pipe_add(RedChannelClient *rcc, PipeItem *item) ++static inline int validate_pipe_add(RedChannelClient *rcc, PipeItem *item) + { + spice_assert(rcc && item); ++ if (SPICE_UNLIKELY(!red_channel_client_is_connected(rcc))) { ++ spice_debug("rcc is disconnected %p", rcc); ++ red_channel_client_release_item(rcc, item, FALSE); ++ return FALSE; ++ } ++ return TRUE; ++} ++ ++void red_channel_client_pipe_add(RedChannelClient *rcc, PipeItem *item) ++{ ++ ++ if (!validate_pipe_add(rcc, item)) { ++ return; ++ } + rcc->pipe_size++; + ring_add(&rcc->pipe, &item->link); + } +@@ -1543,10 +1557,10 @@ void red_channel_client_pipe_add_push(RedChannelClient *rcc, PipeItem *item) + void red_channel_client_pipe_add_after(RedChannelClient *rcc, + PipeItem *item, PipeItem *pos) + { +- spice_assert(rcc); + spice_assert(pos); +- spice_assert(item); +- ++ if (!validate_pipe_add(rcc, item)) { ++ return; ++ } + rcc->pipe_size++; + ring_add_after(&item->link, &pos->link); + } +@@ -1560,14 +1574,18 @@ int red_channel_client_pipe_item_is_linked(RedChannelClient *rcc, + void red_channel_client_pipe_add_tail_no_push(RedChannelClient *rcc, + PipeItem *item) + { +- spice_assert(rcc); ++ if (!validate_pipe_add(rcc, item)) { ++ return; ++ } + rcc->pipe_size++; + ring_add_before(&item->link, &rcc->pipe); + } + + void red_channel_client_pipe_add_tail(RedChannelClient *rcc, PipeItem *item) + { +- spice_assert(rcc); ++ if (!validate_pipe_add(rcc, item)) { ++ return; ++ } + rcc->pipe_size++; + ring_add_before(&item->link, &rcc->pipe); + red_channel_client_push(rcc); +-- +1.8.3.1 + diff --git a/0002-red_channel-add-ref-count-to-RedClient.patch b/0002-red_channel-add-ref-count-to-RedClient.patch new file mode 100644 index 0000000..10e24b9 --- /dev/null +++ b/0002-red_channel-add-ref-count-to-RedClient.patch @@ -0,0 +1,96 @@ +From aab45618cc12799d5f7351ef8832ae73b33057c7 Mon Sep 17 00:00:00 2001 +From: Yonit Halperin +Date: Fri, 26 Jul 2013 13:45:16 -0400 +Subject: [PATCH 2/8] red_channel: add ref count to RedClient + +--- + server/red_channel.c | 23 ++++++++++++++++++++--- + server/red_channel.h | 17 +++++++++++++++-- + 2 files changed, 35 insertions(+), 5 deletions(-) + +diff --git a/server/red_channel.c b/server/red_channel.c +index 0d74413..9168b8a 100644 +--- a/server/red_channel.c ++++ b/server/red_channel.c +@@ -1932,10 +1932,29 @@ RedClient *red_client_new(int migrated) + pthread_mutex_init(&client->lock, NULL); + client->thread_id = pthread_self(); + client->during_target_migrate = migrated; ++ client->refs = 1; + + return client; + } + ++RedClient *red_client_ref(RedClient *client) ++{ ++ spice_assert(client); ++ client->refs++; ++ return client; ++} ++ ++RedClient *red_client_unref(RedClient *client) ++{ ++ if (!--client->refs) { ++ spice_debug("release client=%p", client); ++ pthread_mutex_destroy(&client->lock); ++ free(client); ++ return NULL; ++ } ++ return client; ++} ++ + /* client mutex should be locked before this call */ + static void red_channel_client_set_migration_seamless(RedChannelClient *rcc) + { +@@ -2012,9 +2031,7 @@ void red_client_destroy(RedClient *client) + spice_assert(rcc->send_data.size == 0); + red_channel_client_destroy(rcc); + } +- +- pthread_mutex_destroy(&client->lock); +- free(client); ++ red_client_unref(client); + } + + /* client->lock should be locked */ +diff --git a/server/red_channel.h b/server/red_channel.h +index ba299b6..0dd73ea 100644 +--- a/server/red_channel.h ++++ b/server/red_channel.h +@@ -561,10 +561,25 @@ struct RedClient { + is called */ + int seamless_migrate; + int num_migrated_channels; /* for seamless - number of channels that wait for migrate data*/ ++ int refs; + }; + + RedClient *red_client_new(int migrated); + ++/* ++ * disconnects all the client's channels (should be called from the client's thread) ++ */ ++void red_client_destroy(RedClient *client); ++ ++RedClient *red_client_ref(RedClient *client); ++ ++/* ++ * releases the client resources when refs == 0. ++ * We assume the red_client_derstroy was called before ++ * we reached refs==0 ++ */ ++RedClient *red_client_unref(RedClient *client); ++ + MainChannelClient *red_client_get_main(RedClient *client); + // main should be set once before all the other channels are created + void red_client_set_main(RedClient *client, MainChannelClient *mcc); +@@ -580,7 +595,5 @@ void red_client_semi_seamless_migrate_complete(RedClient *client); /* dst side * + int red_client_during_migrate_at_target(RedClient *client); + + void red_client_migrate(RedClient *client); +-// disconnects all the client's channels (should be called from the client's thread) +-void red_client_destroy(RedClient *client); + + #endif +-- +1.8.3.1 + diff --git a/0003-main_dispatcher-add-ref-count-protection-to-RedClien.patch b/0003-main_dispatcher-add-ref-count-protection-to-RedClien.patch new file mode 100644 index 0000000..fab6d5c --- /dev/null +++ b/0003-main_dispatcher-add-ref-count-protection-to-RedClien.patch @@ -0,0 +1,51 @@ +From 06ba03b7b32a2f0c7f78c82d8f399242526a0b45 Mon Sep 17 00:00:00 2001 +From: Yonit Halperin +Date: Fri, 26 Jul 2013 13:49:24 -0400 +Subject: [PATCH 3/8] main_dispatcher: add ref count protection to RedClient + instances + +--- + server/main_dispatcher.c | 6 ++++-- + 1 file changed, 4 insertions(+), 2 deletions(-) + +diff --git a/server/main_dispatcher.c b/server/main_dispatcher.c +index e7a451a..bf160dd 100644 +--- a/server/main_dispatcher.c ++++ b/server/main_dispatcher.c +@@ -97,6 +97,7 @@ static void main_dispatcher_handle_migrate_complete(void *opaque, + MainDispatcherMigrateSeamlessDstCompleteMessage *mig_complete = payload; + + reds_on_client_seamless_migrate_complete(mig_complete->client); ++ red_client_unref(mig_complete->client); + } + + static void main_dispatcher_handle_mm_time_latency(void *opaque, +@@ -104,6 +105,7 @@ static void main_dispatcher_handle_mm_time_latency(void *opaque, + { + MainDispatcherMmTimeLatencyMessage *msg = payload; + reds_set_client_mm_time_latency(msg->client, msg->latency); ++ red_client_unref(msg->client); + } + + void main_dispatcher_seamless_migrate_dst_complete(RedClient *client) +@@ -115,7 +117,7 @@ void main_dispatcher_seamless_migrate_dst_complete(RedClient *client) + return; + } + +- msg.client = client; ++ msg.client = red_client_ref(client); + dispatcher_send_message(&main_dispatcher.base, MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE, + &msg); + } +@@ -129,7 +131,7 @@ void main_dispatcher_set_mm_time_latency(RedClient *client, uint32_t latency) + return; + } + +- msg.client = client; ++ msg.client = red_client_ref(client); + msg.latency = latency; + dispatcher_send_message(&main_dispatcher.base, MAIN_DISPATCHER_SET_MM_TIME_LATENCY, + &msg); +-- +1.8.3.1 + diff --git a/0004-decouple-disconnection-of-the-main-channel-from-clie.patch b/0004-decouple-disconnection-of-the-main-channel-from-clie.patch new file mode 100644 index 0000000..85d70ec --- /dev/null +++ b/0004-decouple-disconnection-of-the-main-channel-from-clie.patch @@ -0,0 +1,238 @@ +From 8490f83e1f51ac8dbcac3c68c97827e0acb9ec4e Mon Sep 17 00:00:00 2001 +From: Yonit Halperin +Date: Thu, 25 Jul 2013 14:19:21 -0400 +Subject: [PATCH 4/8] decouple disconnection of the main channel from client + destruction + +Fixes rhbz#918169 + +Some channels make direct calls to reds/main_channel routines. If +these routines try to read/write to the socket, and they get socket +error, main_channel_client_on_disconnect is called, and triggers +red_client_destroy. In order to prevent accessing expired references +to RedClient, RedChannelClient, or other objects (inside the original call, after +red_client_destroy has been called) I made the call to +red_client_destroy asynchronous with respect to main_channel_client_on_disconnect. +I added MAIN_DISPATCHER_CLIENT_DISCONNECT to main_dispatcher. +main_channel_client_on_disconnect pushes this msg to the dispatcher, +instead of calling directly to reds_client_disconnect. + +The patch uses RedClient ref-count in order to handle a case where +reds_client_disconnect is called directly (e.g., when a new client connects while +another one is connected), while there is already CLIENT_DISCONNECT msg +pending in the main_dispatcher. + +Examples: +(1) snd_worker.c + + snd_disconnect_channel() + channel->cleanup() //snd_playback_cleanup + reds_enable_mm_timer() + . + . + main_channel_push_multi_media_time()...socket_error + . + . + red_client_destory() + . + . + snd_disconnect_channel() + channel->cleanup() + celt051_encoder_destroy() + celt051_encoder_destory() // double release + +Note that this bug could have been solved by changing the order of +calls: e.g., channel->stream = NULL before calling cleanup, and +some other changes + reference counting. However, I found other +places in the code with similar problems, and I looked for a general +solution, at least till we redesign red_channel to handle reference +counting more consistently. + +(2) inputs_channel.c + + inputs_connect() + main_channel_client_push_notify()...socket_error + . + . + red_client_destory() + . + . + red_channel_client_create() // refers to client which is already destroyed + +(3) reds.c + + reds_handle_main_link() + main_channel_push_init() ...socket error + . + . + red_client_destory() + . + . + main_channel_client_start_net_test(mcc) // refers to mcc which is already destroyed + + This can explain the assert in rhbz#964136, comment #1 (but not the hang that occurred before). +--- + server/main_channel.c | 9 +++++---- + server/main_dispatcher.c | 37 +++++++++++++++++++++++++++++++++++++ + server/main_dispatcher.h | 7 +++++++ + server/reds.c | 1 + + server/reds.h | 2 ++ + 5 files changed, 52 insertions(+), 4 deletions(-) + +diff --git a/server/main_channel.c b/server/main_channel.c +index 233e633..fe032a6 100644 +--- a/server/main_channel.c ++++ b/server/main_channel.c +@@ -46,6 +46,7 @@ + #include "red_common.h" + #include "reds.h" + #include "migration_protocol.h" ++#include "main_dispatcher.h" + + #define ZERO_BUF_SIZE 4096 + +@@ -175,13 +176,13 @@ int main_channel_is_connected(MainChannel *main_chan) + return red_channel_is_connected(&main_chan->base); + } + +-// when disconnection occurs, let reds shutdown all channels. This will trigger the +-// real disconnection of main channel ++/* ++ * When the main channel is disconnected, disconnect the entire client. ++ */ + static void main_channel_client_on_disconnect(RedChannelClient *rcc) + { + spice_printerr("rcc=%p", rcc); +- reds_client_disconnect(rcc->client); +-// red_channel_client_disconnect(rcc); ++ main_dispatcher_client_disconnect(rcc->client); + } + + RedClient *main_channel_get_client_by_link_id(MainChannel *main_chan, uint32_t connection_id) +diff --git a/server/main_dispatcher.c b/server/main_dispatcher.c +index bf160dd..dbe1037 100644 +--- a/server/main_dispatcher.c ++++ b/server/main_dispatcher.c +@@ -41,6 +41,7 @@ enum { + MAIN_DISPATCHER_CHANNEL_EVENT = 0, + MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE, + MAIN_DISPATCHER_SET_MM_TIME_LATENCY, ++ MAIN_DISPATCHER_CLIENT_DISCONNECT, + + MAIN_DISPATCHER_NUM_MESSAGES + }; +@@ -59,6 +60,10 @@ typedef struct MainDispatcherMmTimeLatencyMessage { + uint32_t latency; + } MainDispatcherMmTimeLatencyMessage; + ++typedef struct MainDispatcherClientDisconnectMessage { ++ RedClient *client; ++} MainDispatcherClientDisconnectMessage; ++ + /* channel_event - calls core->channel_event, must be done in main thread */ + static void main_dispatcher_self_handle_channel_event( + int event, +@@ -108,6 +113,16 @@ static void main_dispatcher_handle_mm_time_latency(void *opaque, + red_client_unref(msg->client); + } + ++static void main_dispatcher_handle_client_disconnect(void *opaque, ++ void *payload) ++{ ++ MainDispatcherClientDisconnectMessage *msg = payload; ++ ++ spice_debug("client=%p", msg->client); ++ reds_client_disconnect(msg->client); ++ red_client_unref(msg->client); ++} ++ + void main_dispatcher_seamless_migrate_dst_complete(RedClient *client) + { + MainDispatcherMigrateSeamlessDstCompleteMessage msg; +@@ -137,6 +152,20 @@ void main_dispatcher_set_mm_time_latency(RedClient *client, uint32_t latency) + &msg); + } + ++void main_dispatcher_client_disconnect(RedClient *client) ++{ ++ MainDispatcherClientDisconnectMessage msg; ++ ++ if (!client->disconnecting) { ++ spice_debug("client %p", client); ++ msg.client = red_client_ref(client); ++ dispatcher_send_message(&main_dispatcher.base, MAIN_DISPATCHER_CLIENT_DISCONNECT, ++ &msg); ++ } else { ++ spice_debug("client %p already during disconnection", client); ++ } ++} ++ + static void dispatcher_handle_read(int fd, int event, void *opaque) + { + Dispatcher *dispatcher = opaque; +@@ -144,6 +173,11 @@ static void dispatcher_handle_read(int fd, int event, void *opaque) + dispatcher_handle_recv_read(dispatcher); + } + ++/* ++ * FIXME: ++ * Reds routines shouldn't be exposed. Instead reds.c should register the callbacks, ++ * and the corresponding operations should be made only via main_dispatcher. ++ */ + void main_dispatcher_init(SpiceCoreInterface *core) + { + memset(&main_dispatcher, 0, sizeof(main_dispatcher)); +@@ -160,4 +194,7 @@ void main_dispatcher_init(SpiceCoreInterface *core) + dispatcher_register_handler(&main_dispatcher.base, MAIN_DISPATCHER_SET_MM_TIME_LATENCY, + main_dispatcher_handle_mm_time_latency, + sizeof(MainDispatcherMmTimeLatencyMessage), 0 /* no ack */); ++ dispatcher_register_handler(&main_dispatcher.base, MAIN_DISPATCHER_CLIENT_DISCONNECT, ++ main_dispatcher_handle_client_disconnect, ++ sizeof(MainDispatcherClientDisconnectMessage), 0 /* no ack */); + } +diff --git a/server/main_dispatcher.h b/server/main_dispatcher.h +index 0c79ca8..522c7f9 100644 +--- a/server/main_dispatcher.h ++++ b/server/main_dispatcher.h +@@ -7,6 +7,13 @@ + void main_dispatcher_channel_event(int event, SpiceChannelEventInfo *info); + void main_dispatcher_seamless_migrate_dst_complete(RedClient *client); + void main_dispatcher_set_mm_time_latency(RedClient *client, uint32_t latency); ++/* ++ * Disconnecting the client is always executed asynchronously, ++ * in order to protect from expired references in the routines ++ * that triggered the client destruction. ++ */ ++void main_dispatcher_client_disconnect(RedClient *client); ++ + void main_dispatcher_init(SpiceCoreInterface *core); + + #endif //MAIN_DISPATCHER_H +diff --git a/server/reds.c b/server/reds.c +index 30d0652..c66ddc4 100644 +--- a/server/reds.c ++++ b/server/reds.c +@@ -537,6 +537,7 @@ void reds_client_disconnect(RedClient *client) + } + + if (!client || client->disconnecting) { ++ spice_debug("client %p already during disconnection", client); + return; + } + +diff --git a/server/reds.h b/server/reds.h +index c5c557d..1c5ae84 100644 +--- a/server/reds.h ++++ b/server/reds.h +@@ -136,6 +136,8 @@ void reds_handle_agent_mouse_event(const VDAgentMouseState *mouse_state); // use + extern struct SpiceCoreInterface *core; + + // Temporary measures to make splitting reds.c to inputs_channel.c easier ++ ++/* should be called only from main_dispatcher */ + void reds_client_disconnect(RedClient *client); + + // Temporary (?) for splitting main channel +-- +1.8.3.1 + diff --git a/0005-reds-s-red_client_disconnect-red_channel_client_shut.patch b/0005-reds-s-red_client_disconnect-red_channel_client_shut.patch new file mode 100644 index 0000000..035f462 --- /dev/null +++ b/0005-reds-s-red_client_disconnect-red_channel_client_shut.patch @@ -0,0 +1,56 @@ +From 46c2ce8f1af0170a2c6a335cc743a3ddff2d96d0 Mon Sep 17 00:00:00 2001 +From: Yonit Halperin +Date: Thu, 25 Jul 2013 14:25:24 -0400 +Subject: [PATCH 5/8] reds: s/red_client_disconnect/red_channel_client_shutdown + inside callbacks + +When we want to disconnect the main channel from a callback, it is +safer to use red_channel_client_shutdown, instead of directly +destroying the client. It is also more consistent with how other +channels treat errors. +red_channel_client_shutdown will trigger socket error in the main channel. +Then, main_channel_client_on_disconnect will be called, +and eventually, main_dispatcher_client_disconnect. + +I didn't replace calls to reds_disconnect/reds_client_disconnect in +places where those calls were safe && that might need immediate client +disconnection. +--- + server/reds.c | 7 ++++--- + 1 file changed, 4 insertions(+), 3 deletions(-) + +diff --git a/server/reds.c b/server/reds.c +index c66ddc4..ae87c90 100644 +--- a/server/reds.c ++++ b/server/reds.c +@@ -879,7 +879,8 @@ static void vdi_port_on_free_self_token(void *opaque) + + static void vdi_port_remove_client(RedClient *client, void *opaque) + { +- reds_client_disconnect(client); ++ red_channel_client_shutdown(main_channel_client_get_base( ++ red_client_get_main(client))); + } + + /****************************************************************************/ +@@ -1009,7 +1010,7 @@ void reds_on_main_agent_start(MainChannelClient *mcc, uint32_t num_tokens) + + if (!client_added) { + spice_warning("failed to add client to agent"); +- reds_client_disconnect(rcc->client); ++ red_channel_client_shutdown(rcc); + return; + } + } else { +@@ -1126,7 +1127,7 @@ void reds_on_main_agent_data(MainChannelClient *mcc, void *message, size_t size) + reds_on_main_agent_monitors_config(mcc, message, size); + return; + case AGENT_MSG_FILTER_PROTO_ERROR: +- reds_disconnect(); ++ red_channel_client_shutdown(main_channel_client_get_base(mcc)); + return; + } + +-- +1.8.3.1 + diff --git a/0006-snd_worker-fix-memory-leak-of-PlaybackChannel.patch b/0006-snd_worker-fix-memory-leak-of-PlaybackChannel.patch new file mode 100644 index 0000000..fc1ee51 --- /dev/null +++ b/0006-snd_worker-fix-memory-leak-of-PlaybackChannel.patch @@ -0,0 +1,42 @@ +From 134b7f310de5120b233670d18641d32204f31318 Mon Sep 17 00:00:00 2001 +From: Yonit Halperin +Date: Thu, 25 Jul 2013 14:49:33 -0400 +Subject: [PATCH 6/8] snd_worker: fix memory leak of PlaybackChannel + +When the sequence of calls bellow occurs, the PlaybackChannel +is not released (snd_channel_put is not called for the +samples that refer to the channel). + + spice_server_playback_get_buffer + snd_channel_disconnect + spice_server_playback_put_samples +--- + server/snd_worker.c | 9 ++++----- + 1 file changed, 4 insertions(+), 5 deletions(-) + +diff --git a/server/snd_worker.c b/server/snd_worker.c +index d6ec47a..849f002 100644 +--- a/server/snd_worker.c ++++ b/server/snd_worker.c +@@ -1097,14 +1097,13 @@ SPICE_GNUC_VISIBLE void spice_server_playback_put_samples(SpicePlaybackInstance + PlaybackChannel *playback_channel; + AudioFrame *frame; + +- if (!sin->st->worker.connection) { +- return; +- } +- + frame = SPICE_CONTAINEROF(samples, AudioFrame, samples); + playback_channel = frame->channel; +- if (!snd_channel_put(&playback_channel->base) || !playback_channel->base.worker->connection) { ++ spice_assert(playback_channel); ++ if (!snd_channel_put(&playback_channel->base) || ++ sin->st->worker.connection != &playback_channel->base) { + /* lost last reference, channel has been destroyed previously */ ++ spice_info("audio samples belong to a disconnected channel"); + return; + } + spice_assert(playback_channel->base.active); +-- +1.8.3.1 + diff --git a/0007-snd_worker-snd_disconnect_channel-don-t-call-snd_cha.patch b/0007-snd_worker-snd_disconnect_channel-don-t-call-snd_cha.patch new file mode 100644 index 0000000..fd461be --- /dev/null +++ b/0007-snd_worker-snd_disconnect_channel-don-t-call-snd_cha.patch @@ -0,0 +1,62 @@ +From 02f44c137df99ed2e89699e49b64c13673b0cd06 Mon Sep 17 00:00:00 2001 +From: Yonit Halperin +Date: Thu, 25 Jul 2013 15:07:43 -0400 +Subject: [PATCH 7/8] snd_worker/snd_disconnect_channel: don't call + snd_channel_put if the channel has already been disconnected + +The snd channels has one reference as long as their socket is active. +The playback channel has an additional reference for each frame that is +currently filled by the sound device. +Once the channel is disconnected (the socket has been freed and the +first reference is released) snd_disconnect_channel shouldn't release +a reference again. +--- + server/snd_worker.c | 20 ++++++++++---------- + 1 file changed, 10 insertions(+), 10 deletions(-) + +diff --git a/server/snd_worker.c b/server/snd_worker.c +index 849f002..3827416 100644 +--- a/server/snd_worker.c ++++ b/server/snd_worker.c +@@ -212,21 +212,20 @@ static void snd_disconnect_channel(SndChannel *channel) + { + SndWorker *worker; + +- if (!channel) { ++ if (!channel || !channel->stream) { + spice_debug("not connected"); + return; + } + spice_debug("%p", channel); + worker = channel->worker; +- if (channel->stream) { +- channel->cleanup(channel); +- red_channel_client_disconnect(worker->connection->channel_client); +- core->watch_remove(channel->stream->watch); +- channel->stream->watch = NULL; +- reds_stream_free(channel->stream); +- channel->stream = NULL; +- spice_marshaller_destroy(channel->send_data.marshaller); +- } ++ channel->cleanup(channel); ++ red_channel_client_disconnect(worker->connection->channel_client); ++ worker->connection->channel_client = NULL; ++ core->watch_remove(channel->stream->watch); ++ channel->stream->watch = NULL; ++ reds_stream_free(channel->stream); ++ channel->stream = NULL; ++ spice_marshaller_destroy(channel->send_data.marshaller); + snd_channel_put(channel); + worker->connection = NULL; + } +@@ -897,6 +896,7 @@ static SndChannel *__new_channel(SndWorker *worker, int size, uint32_t channel_i + int tos; + MainChannelClient *mcc = red_client_get_main(client); + ++ spice_assert(stream); + if ((flags = fcntl(stream->socket, F_GETFL)) == -1) { + spice_printerr("accept failed, %s", strerror(errno)); + goto error1; +-- +1.8.3.1 + diff --git a/0008-log-improve-debug-information-related-to-client-disc.patch b/0008-log-improve-debug-information-related-to-client-disc.patch new file mode 100644 index 0000000..77a04ba --- /dev/null +++ b/0008-log-improve-debug-information-related-to-client-disc.patch @@ -0,0 +1,99 @@ +From c2e46b926e0ee4226f0f93942e7fa2c5b409f73d Mon Sep 17 00:00:00 2001 +From: Yonit Halperin +Date: Fri, 26 Jul 2013 12:15:00 -0400 +Subject: [PATCH 8/8] log: improve debug information related to client + disconnection + +--- + server/red_channel.c | 9 ++++++--- + server/snd_worker.c | 7 ++++--- + 2 files changed, 10 insertions(+), 6 deletions(-) + +diff --git a/server/red_channel.c b/server/red_channel.c +index 9168b8a..d565634 100644 +--- a/server/red_channel.c ++++ b/server/red_channel.c +@@ -1112,6 +1112,7 @@ static void red_channel_client_ref(RedChannelClient *rcc) + static void red_channel_client_unref(RedChannelClient *rcc) + { + if (!--rcc->refs) { ++ spice_debug("destroy rcc=%p", rcc); + if (rcc->send_data.main.marshaller) { + spice_marshaller_destroy(rcc->send_data.main.marshaller); + } +@@ -1708,6 +1709,8 @@ static void red_channel_client_disconnect_dummy(RedChannelClient *rcc) + { + spice_assert(rcc->dummy); + if (ring_item_is_linked(&rcc->channel_link)) { ++ spice_printerr("rcc=%p (channel=%p type=%d id=%d)", rcc, rcc->channel, ++ rcc->channel->type, rcc->channel->id); + red_channel_remove_client(rcc); + } + rcc->dummy_connected = FALSE; +@@ -1715,8 +1718,6 @@ static void red_channel_client_disconnect_dummy(RedChannelClient *rcc) + + void red_channel_client_disconnect(RedChannelClient *rcc) + { +- spice_printerr("%p (channel %p type %d id %d)", rcc, rcc->channel, +- rcc->channel->type, rcc->channel->id); + if (rcc->dummy) { + red_channel_client_disconnect_dummy(rcc); + return; +@@ -1724,6 +1725,8 @@ void red_channel_client_disconnect(RedChannelClient *rcc) + if (!red_channel_client_is_connected(rcc)) { + return; + } ++ spice_printerr("rcc=%p (channel=%p type=%d id=%d)", rcc, rcc->channel, ++ rcc->channel->type, rcc->channel->id); + red_channel_client_pipe_clear(rcc); + if (rcc->stream->watch) { + rcc->channel->core->watch_remove(rcc->stream->watch); +@@ -2007,7 +2010,7 @@ void red_client_destroy(RedClient *client) + RingItem *link, *next; + RedChannelClient *rcc; + +- spice_printerr("destroy client with #channels %d", client->channels_num); ++ spice_printerr("destroy client %p with #channels=%d", client, client->channels_num); + if (!pthread_equal(pthread_self(), client->thread_id)) { + spice_warning("client->thread_id (0x%lx) != pthread_self (0x%lx)." + "If one of the threads is != io-thread && != vcpu-thread," +diff --git a/server/snd_worker.c b/server/snd_worker.c +index 3827416..ebddfcd 100644 +--- a/server/snd_worker.c ++++ b/server/snd_worker.c +@@ -201,8 +201,8 @@ static SndChannel *snd_channel_get(SndChannel *channel) + static SndChannel *snd_channel_put(SndChannel *channel) + { + if (!--channel->refs) { ++ spice_printerr("SndChannel=%p freed", channel); + free(channel); +- spice_printerr("sound channel freed"); + return NULL; + } + return channel; +@@ -216,7 +216,8 @@ static void snd_disconnect_channel(SndChannel *channel) + spice_debug("not connected"); + return; + } +- spice_debug("%p", channel); ++ spice_debug("SndChannel=%p rcc=%p type=%d", ++ channel, channel->channel_client, channel->channel_client->channel->type); + worker = channel->worker; + channel->cleanup(channel); + red_channel_client_disconnect(worker->connection->channel_client); +@@ -976,11 +977,11 @@ static void snd_disconnect_channel_client(RedChannelClient *rcc) + { + SndWorker *worker; + +- spice_debug(NULL); + spice_assert(rcc->channel); + spice_assert(rcc->channel->data); + worker = (SndWorker *)rcc->channel->data; + ++ spice_debug("channel-type=%d", rcc->channel->type); + if (worker->connection) { + spice_assert(worker->connection->channel_client == rcc); + snd_disconnect_channel(worker->connection); +-- +1.8.3.1 + diff --git a/sources b/sources index b8ba897..0f55d1d 100644 --- a/sources +++ b/sources @@ -1 +1 @@ -f33a682892f6793169f20298b2296449 spice-0.12.3.tar.bz2 +325b1c42ce24e75de45a75876b73a8bd spice-0.12.4.tar.bz2 diff --git a/spice.spec b/spice.spec index 8a0a3ac..9dbf964 100644 --- a/spice.spec +++ b/spice.spec @@ -1,11 +1,19 @@ Name: spice -Version: 0.12.3 -Release: 2%{?dist} +Version: 0.12.4 +Release: 1%{?dist} Summary: Implements the SPICE protocol Group: User Interface/Desktops License: LGPLv2+ URL: http://www.spice-space.org/ Source0: http://www.spice-space.org/download/releases/%{name}-%{version}.tar.bz2 +Patch1: 0001-red_channel-prevent-adding-and-pushing-pipe-items-af.patch +Patch2: 0002-red_channel-add-ref-count-to-RedClient.patch +Patch3: 0003-main_dispatcher-add-ref-count-protection-to-RedClien.patch +Patch4: 0004-decouple-disconnection-of-the-main-channel-from-clie.patch +Patch5: 0005-reds-s-red_client_disconnect-red_channel_client_shut.patch +Patch6: 0006-snd_worker-fix-memory-leak-of-PlaybackChannel.patch +Patch7: 0007-snd_worker-snd_disconnect_channel-don-t-call-snd_cha.patch +Patch8: 0008-log-improve-debug-information-related-to-client-disc.patch # https://bugzilla.redhat.com/show_bug.cgi?id=613529 %if 0%{?rhel} @@ -33,6 +41,7 @@ variety of machine architectures. %package server Summary: Implements the server side of the SPICE protocol Group: System Environment/Libraries +Obsoletes: spice-client < %{version}-%{release} %description server The Simple Protocol for Independent Computing Environments (SPICE) is @@ -48,7 +57,7 @@ to be a SPICE server. %package server-devel Summary: Header files, libraries and development documentation for spice-server Group: Development/Libraries -Requires: %{name}-server = %{version}-%{release} +Requires: %{name}-server%{?_isa} = %{version}-%{release} Requires: pkgconfig %description server-devel @@ -59,6 +68,14 @@ using spice-server, you will need to install spice-server-devel. %prep %setup -q +%patch1 -p1 +%patch2 -p1 +%patch3 -p1 +%patch4 -p1 +%patch5 -p1 +%patch6 -p1 +%patch7 -p1 +%patch8 -p1 %build @@ -75,8 +92,6 @@ mkdir -p %{buildroot}%{_libexecdir} %post server -p /sbin/ldconfig - - %postun server -p /sbin/ldconfig @@ -84,7 +99,6 @@ mkdir -p %{buildroot}%{_libexecdir} %doc COPYING README NEWS %{_libdir}/libspice-server.so.1* - %files server-devel %{_includedir}/spice-server %{_libdir}/libspice-server.so @@ -92,6 +106,11 @@ mkdir -p %{buildroot}%{_libexecdir} %changelog +* Fri Aug 2 2013 Hans de Goede - 0.12.4-1 +- New upstream bug-fix release 0.12.4 +- Add patches from upstream git to fix sound-channel-free crash (rhbz#986407) +- Add Obsoletes for dropped spice-client sub-package + * Thu May 23 2013 Christophe Fergeau 0.12.3-2 - Stop building spicec, it's obsolete and superseded by remote-viewer (part of virt-viewer)