From 708002ccbc39dd6fa9b1f99da654ecc474b2967a Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Mon, 21 Jan 2013 16:09:59 +0100 Subject: [PATCH] Add a number of misc. bug-fixes from upstream - Add a number of misc. bug-fixes from upstream --- ..._client_capabilities-protect-against.patch | 36 ++++ ...ert-a-drawable-to-its-position-in-th.patch | 166 ++++++++++++++++++ ...aring-the-stream-vis_region-after-it.patch | 39 ++++ ...libspice-server-with-libm-libpthread.patch | 32 ++++ ...Worker-CRITICAL-red_worker.c-10968-r.patch | 77 ++++++++ ...nitors_config-Drop-bogus-real_count-.patch | 56 ++++++ spice.spec | 17 +- 7 files changed, 422 insertions(+), 1 deletion(-) create mode 100644 0001-server-guest_set_client_capabilities-protect-against.patch create mode 100644 0002-red_worker.c-insert-a-drawable-to-its-position-in-th.patch create mode 100644 0003-red_worker.c-clearing-the-stream-vis_region-after-it.patch create mode 100644 0004-link-libspice-server-with-libm-libpthread.patch create mode 100644 0005-server-Fix-SpiceWorker-CRITICAL-red_worker.c-10968-r.patch create mode 100644 0006-worker_update_monitors_config-Drop-bogus-real_count-.patch diff --git a/0001-server-guest_set_client_capabilities-protect-against.patch b/0001-server-guest_set_client_capabilities-protect-against.patch new file mode 100644 index 0000000..171b799 --- /dev/null +++ b/0001-server-guest_set_client_capabilities-protect-against.patch @@ -0,0 +1,36 @@ +From 1ff42341629948c591621f0a8ddf2859543ca05d Mon Sep 17 00:00:00 2001 +From: Uri Lublin +Date: Mon, 17 Dec 2012 18:34:43 +0200 +Subject: [PATCH spice 1/6] server: guest_set_client_capabilities: protect + against NULL worker->display_channel + +Reported-by: Michal Luscon + +Found by a Coverity scan: + in handle_dev_start - + Checking "worker->display_channel" implies that "worker->display_channel" + might be NULL. + Passing "worker" to function "guest_set_client_capabilities" + in guest_set_client_capabilities - + Directly dereferencing parameter "worker->display_channel" +--- + server/red_worker.c | 3 ++- + 1 file changed, 2 insertions(+), 1 deletion(-) + +diff --git a/server/red_worker.c b/server/red_worker.c +index e5e3d05..1a9c375 100644 +--- a/server/red_worker.c ++++ b/server/red_worker.c +@@ -10342,7 +10342,8 @@ static void guest_set_client_capabilities(RedWorker *worker) + worker->set_client_capabilities_pending = 1; + return; + } +- if (worker->display_channel->common.base.clients_num == 0) { ++ if ((worker->display_channel == NULL) || ++ (worker->display_channel->common.base.clients_num == 0)) { + worker->qxl->st->qif->set_client_capabilities(worker->qxl, FALSE, caps); + } else { + // Take least common denominator +-- +1.8.1 + diff --git a/0002-red_worker.c-insert-a-drawable-to-its-position-in-th.patch b/0002-red_worker.c-insert-a-drawable-to-its-position-in-th.patch new file mode 100644 index 0000000..7df84b1 --- /dev/null +++ b/0002-red_worker.c-insert-a-drawable-to-its-position-in-th.patch @@ -0,0 +1,166 @@ +From b22e82ad578c835996e184cefe5a26a70378714a Mon Sep 17 00:00:00 2001 +From: Yonit Halperin +Date: Tue, 8 Jan 2013 10:47:53 -0500 +Subject: [PATCH spice 2/6] red_worker.c: insert a drawable to its position in + the current tree before calling red_detach_streams_behind + +resolves: rhbz#891326 + +Starting from commit 81fe00b08ad4f, red_detach_streams_behind can +trigger modifications in the current tree (by update_area calls). Thus, +after calling red_detach_streams_behind it is not safe to access tree +entries that were calculated before the call. +This patch inserts the drawable to the tree before the call to +red_detach_streams_behind. This change also requires making sure +that rendering operations that can be triggered by +red_detach_streams_behind will not include this drawable (which is now part of the tree). +--- + server/red_worker.c | 55 ++++++++++++++++++++++++++++++++++++++++------------- + 1 file changed, 42 insertions(+), 13 deletions(-) + +diff --git a/server/red_worker.c b/server/red_worker.c +index 1a9c375..5e00cb6 100644 +--- a/server/red_worker.c ++++ b/server/red_worker.c +@@ -1022,6 +1022,8 @@ static void red_current_flush(RedWorker *worker, int surface_id); + #else + static void red_draw_drawable(RedWorker *worker, Drawable *item); + static void red_update_area(RedWorker *worker, const SpiceRect *area, int surface_id); ++static void red_update_area_till(RedWorker *worker, const SpiceRect *area, int surface_id, ++ Drawable *last); + #endif + static void red_release_cursor(RedWorker *worker, CursorItem *cursor); + static inline void release_drawable(RedWorker *worker, Drawable *item); +@@ -2615,7 +2617,9 @@ static int red_display_drawable_is_in_pipe(DisplayChannelClient *dcc, Drawable * + * after red_display_detach_stream_gracefully is called for all the display channel clients, + * red_detach_stream should be called. See comment (1). + */ +-static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dcc, Stream *stream) ++static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dcc, ++ Stream *stream, ++ Drawable *update_area_limit) + { + int stream_id = get_stream_id(dcc->common.worker, stream); + StreamAgent *agent = &dcc->stream_agents[stream_id]; +@@ -2669,27 +2673,41 @@ static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dc + spice_debug("stream %d: upgrade by screenshot. has current %d. box ==>", + stream_id, stream->current != NULL); + rect_debug(&upgrade_area); +- red_update_area(dcc->common.worker, &upgrade_area, 0); ++ if (update_area_limit) { ++ red_update_area_till(dcc->common.worker, &upgrade_area, 0, update_area_limit); ++ } else { ++ red_update_area(dcc->common.worker, &upgrade_area, 0); ++ } + red_add_surface_area_image(dcc, 0, &upgrade_area, NULL, FALSE); + } + + } + +-static inline void red_detach_stream_gracefully(RedWorker *worker, Stream *stream) ++static inline void red_detach_stream_gracefully(RedWorker *worker, Stream *stream, ++ Drawable *update_area_limit) + { + RingItem *item; + DisplayChannelClient *dcc; + + WORKER_FOREACH_DCC(worker, item, dcc) { +- red_display_detach_stream_gracefully(dcc, stream); ++ red_display_detach_stream_gracefully(dcc, stream, update_area_limit); + } + if (stream->current) { + red_detach_stream(worker, stream, TRUE); + } + } + +-// region should be a primary surface region +-static void red_detach_streams_behind(RedWorker *worker, QRegion *region) ++/* ++ * region : a primary surface region. Streams that intersects with the given ++ * region will be detached. ++ * drawable: If detaching the stream is triggered by the addition of a new drawable ++ * that is dependent on the given region, and the drawable is already a part ++ * of the "current tree", the drawable parameter should be set with ++ * this drawable, otherwise, it should be NULL. Then, if detaching the stream ++ * involves sending an upgrade image to the client, this drawable won't be rendered ++ * (see red_display_detach_stream_gracefully). ++ */ ++static void red_detach_streams_behind(RedWorker *worker, QRegion *region, Drawable *drawable) + { + Ring *ring = &worker->streams; + RingItem *item = ring_get_head(ring); +@@ -2706,7 +2724,7 @@ static void red_detach_streams_behind(RedWorker *worker, QRegion *region) + StreamAgent *agent = &dcc->stream_agents[get_stream_id(worker, stream)]; + + if (region_intersects(&agent->vis_region, region)) { +- red_display_detach_stream_gracefully(dcc, stream); ++ red_display_detach_stream_gracefully(dcc, stream, drawable); + detach_stream = 1; + spice_debug("stream %d", get_stream_id(worker, stream)); + } +@@ -2798,7 +2816,7 @@ static inline void red_handle_streams_timout(RedWorker *worker) + Stream *stream = SPICE_CONTAINEROF(item, Stream, link); + item = ring_next(ring, item); + if (now >= (stream->last_time + RED_STREAM_TIMOUT)) { +- red_detach_stream_gracefully(worker, stream); ++ red_detach_stream_gracefully(worker, stream, NULL); + red_stop_stream(worker, stream); + } + } +@@ -3507,13 +3525,24 @@ static inline int red_current_add(RedWorker *worker, Ring *ring, Drawable *drawa + exclude_region(worker, ring, exclude_base, &exclude_rgn, NULL, drawable); + red_use_stream_trace(worker, drawable); + red_streams_update_visible_region(worker, drawable); ++ /* ++ * Performing the insertion after exclude_region for ++ * safety (todo: Not sure if exclude_region can affect the drawable ++ * if it is added to the tree before calling exclude_region). ++ */ ++ __current_add_drawable(worker, drawable, ring); + } else { ++ /* ++ * red_detach_streams_behind can affect the current tree since it may ++ * trigger calls to update_area. Thus, the drawable should be added to the tree ++ * before calling red_detach_streams_behind ++ */ ++ __current_add_drawable(worker, drawable, ring); + if (drawable->surface_id == 0) { +- red_detach_streams_behind(worker, &drawable->tree_item.base.rgn); ++ red_detach_streams_behind(worker, &drawable->tree_item.base.rgn, drawable); + } + } + region_destroy(&exclude_rgn); +- __current_add_drawable(worker, drawable, ring); + stat_add(&worker->add_stat, start_time); + return TRUE; + } +@@ -3567,7 +3596,7 @@ static inline int red_current_add_with_shadow(RedWorker *worker, Ring *ring, Dra + + // only primary surface streams are supported + if (is_primary_surface(worker, item->surface_id)) { +- red_detach_streams_behind(worker, &shadow->base.rgn); ++ red_detach_streams_behind(worker, &shadow->base.rgn, NULL); + } + ring_add(ring, &shadow->base.siblings_link); + __current_add_drawable(worker, item, ring); +@@ -3579,7 +3608,7 @@ static inline int red_current_add_with_shadow(RedWorker *worker, Ring *ring, Dra + red_streams_update_visible_region(worker, item); + } else { + if (item->surface_id == 0) { +- red_detach_streams_behind(worker, &item->tree_item.base.rgn); ++ red_detach_streams_behind(worker, &item->tree_item.base.rgn, item); + } + } + stat_add(&worker->add_stat, start_time); +@@ -3911,7 +3940,7 @@ static inline int red_handle_surfaces_dependencies(RedWorker *worker, Drawable * + QRegion depend_region; + region_init(&depend_region); + region_add(&depend_region, &drawable->red_drawable->surfaces_rects[x]); +- red_detach_streams_behind(worker, &depend_region); ++ red_detach_streams_behind(worker, &depend_region, NULL); + } + } + } +-- +1.8.1 + diff --git a/0003-red_worker.c-clearing-the-stream-vis_region-after-it.patch b/0003-red_worker.c-clearing-the-stream-vis_region-after-it.patch new file mode 100644 index 0000000..a6e87f2 --- /dev/null +++ b/0003-red_worker.c-clearing-the-stream-vis_region-after-it.patch @@ -0,0 +1,39 @@ +From 4eb172f6fe9dffe2f3f3045189e84375e54c6cad Mon Sep 17 00:00:00 2001 +From: Yonit Halperin +Date: Tue, 8 Jan 2013 10:51:26 -0500 +Subject: [PATCH spice 3/6] red_worker.c: clearing the stream vis_region, after + it has been detached + +The stream vis_region should be cleared after the stream region was sent +to the client losslessly. Otherwise, we might send redundant stream upgrades +if we process more drawables that are dependent on the stream region. +--- + server/red_worker.c | 5 +++-- + 1 file changed, 3 insertions(+), 2 deletions(-) + +diff --git a/server/red_worker.c b/server/red_worker.c +index 5e00cb6..085e3e6 100644 +--- a/server/red_worker.c ++++ b/server/red_worker.c +@@ -2646,7 +2646,7 @@ static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dc + spice_debug("stream %d: upgrade by linked drawable. sized %d, box ==>", + stream_id, stream->current->sized_stream != NULL); + rect_debug(&stream->current->red_drawable->bbox); +- return; ++ goto clear_vis_region; + } + spice_debug("stream %d: upgrade by drawable. sized %d, box ==>", + stream_id, stream->current->sized_stream != NULL); +@@ -2680,7 +2680,8 @@ static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dc + } + red_add_surface_area_image(dcc, 0, &upgrade_area, NULL, FALSE); + } +- ++clear_vis_region: ++ region_clear(&agent->vis_region); + } + + static inline void red_detach_stream_gracefully(RedWorker *worker, Stream *stream, +-- +1.8.1 + diff --git a/0004-link-libspice-server-with-libm-libpthread.patch b/0004-link-libspice-server-with-libm-libpthread.patch new file mode 100644 index 0000000..3662bdc --- /dev/null +++ b/0004-link-libspice-server-with-libm-libpthread.patch @@ -0,0 +1,32 @@ +From cf8ebbc48491cf5178e4edc57f49ceded20ead55 Mon Sep 17 00:00:00 2001 +From: Michael Tokarev +Date: Sat, 2 Jun 2012 13:33:42 +0000 +Subject: [PATCH spice 4/6] link libspice server with libm libpthread + +server/Makefile apparently forgot to link libspice-server +with -lm -lpthread, but it uses symbols from these libraries +directly. These libs are detected by configure and stored in +$(SPICE_NONPKGCONFIG_LIBS) make variable, but this variable +is never referenced at link time. Add it to server/Makefile.am, +to libspice_server_la_LIBADD variable. + +Signed-off-By: Michael Tokarev +--- + server/Makefile.am | 1 + + 1 file changed, 1 insertion(+) + +diff --git a/server/Makefile.am b/server/Makefile.am +index b3fcd2c..8b380fc 100644 +--- a/server/Makefile.am ++++ b/server/Makefile.am +@@ -40,6 +40,7 @@ libspice_server_la_LIBADD = \ + $(SLIRP_LIBS) \ + $(SSL_LIBS) \ + $(Z_LIBS) \ ++ $(SPICE_NONPKGCONFIG_LIBS) \ + $(NULL) + + libspice_server_la_SOURCES = \ +-- +1.8.1 + diff --git a/0005-server-Fix-SpiceWorker-CRITICAL-red_worker.c-10968-r.patch b/0005-server-Fix-SpiceWorker-CRITICAL-red_worker.c-10968-r.patch new file mode 100644 index 0000000..9e44eeb --- /dev/null +++ b/0005-server-Fix-SpiceWorker-CRITICAL-red_worker.c-10968-r.patch @@ -0,0 +1,77 @@ +From d2e1f939fec9b4d852a009cc55c4bbb3d2a94d3b Mon Sep 17 00:00:00 2001 +From: Hans de Goede +Date: Thu, 10 Jan 2013 22:55:51 +0100 +Subject: [PATCH spice 5/6] server: Fix SpiceWorker-CRITICAL **: + red_worker.c:10968:red_push_monitors_config: condition `monitors_config != + NULL' failed + +During my dynamic monitor support testing today, I hit the following assert +in red_worker.c: +"red_push_monitors_config: condition `monitors_config != NULL' failed" + +This is caused by the following scenario: +1) Guest causes handle_dev_monitors_config_async() to be called +2) handle_dev_monitors_config_async() calls worker_update_monitors_config() +3) handle_dev_monitors_config_async() pushes worker->monitors_config, this + takes a ref on the current monitors_config +4) Guest causes handle_dev_monitors_config_async() to be called *again* +5) handle_dev_monitors_config_async() calls worker_update_monitors_config() +6) worker_update_monitors_config() does a decref on worker->monitors_config, + releasing the workers reference, this monitor_config from step 2 is + not yet free-ed though as the pipe-item still holds a ref +7) worker_update_monitors_config() creates a new monitors_config with an + initial ref-count of 1 and stores that in worker->monitors_config +8) The pipe-item of the *first* monitors_config is send, upon completion + a decref is done on the monitors_config, and monitors_config_decref not + only frees the monitor_config, but *also* sets worker->monitors_config + to NULL, even though worker->monitors_config no longer refers to the + monitor_config being freed, it refers to the 2nd monitor_config! +9) The client which was connected when this all happened disconnects +10) A new client connects, leading to the assert: + at red_worker.c:9519 + num_common_caps=1, common_caps=0x5555569b6f60, migrate=0, + stream=, client=, worker=) + at red_worker.c:10423 + at red_worker.c:11301 + +Note that red_worker.c:9519 is: + red_push_monitors_config(dcc); +gdb does not point to the actual line of the assert because the function gets +inlined. + +The fix is easy and obvious, don't set worker->monitors_config to NULL in +monitors_config_decref. I'm a bit baffled as to why that code is there in +the first place, the whole point of ref-counting is to not have one single +unique place to store the reference... + +This fix should not have any adverse side-effects as the 4 callers of +monitors_config_decref fall into 2 categories: +1) Code which immediately after the decref replaces worker->monitors_config + with a new monitors_config: + worker_update_monitors_config() + set_monitors_config_to_primary() +2) pipe-item freeing code, which should not touch the worker state at all + to being with + +Signed-off-by: Hans de Goede +--- + server/red_worker.c | 3 +-- + 1 file changed, 1 insertion(+), 2 deletions(-) + +diff --git a/server/red_worker.c b/server/red_worker.c +index 085e3e6..de070a6 100644 +--- a/server/red_worker.c ++++ b/server/red_worker.c +@@ -1239,8 +1239,7 @@ static void monitors_config_decref(MonitorsConfig *monitors_config) + return; + } + +- spice_debug("removing worker monitors config"); +- monitors_config->worker->monitors_config = NULL; ++ spice_debug("freeing monitors config"); + free(monitors_config); + } + +-- +1.8.1 + diff --git a/0006-worker_update_monitors_config-Drop-bogus-real_count-.patch b/0006-worker_update_monitors_config-Drop-bogus-real_count-.patch new file mode 100644 index 0000000..aac4b05 --- /dev/null +++ b/0006-worker_update_monitors_config-Drop-bogus-real_count-.patch @@ -0,0 +1,56 @@ +From 50efe1e48d2fcf6a2f12c933ce29e73b6985f599 Mon Sep 17 00:00:00 2001 +From: Hans de Goede +Date: Thu, 10 Jan 2013 23:30:34 +0100 +Subject: [PATCH spice 6/6] worker_update_monitors_config: Drop bogus + real_count accounting + +1) This does not buy us much, as red_marshall_monitors_config() also + removes 0x0 sized monitors and does a much better job at it + (also removing intermediate ones, not only tailing ones) +2) The code is wrong, as it allocs space for real_count heads, where + real_count always <= monitors_config->count and then stores + monitors_config->count in worker->monitors_config->count, causing + red_marshall_monitors_config to potentially walk + worker->monitors_config->heads past its boundaries. + +Signed-off-by: Hans de Goede +--- + server/red_worker.c | 15 +-------------- + 1 file changed, 1 insertion(+), 14 deletions(-) + +diff --git a/server/red_worker.c b/server/red_worker.c +index de070a6..11fa126 100644 +--- a/server/red_worker.c ++++ b/server/red_worker.c +@@ -10951,7 +10951,6 @@ static void worker_update_monitors_config(RedWorker *worker, + { + int heads_size; + MonitorsConfig *monitors_config; +- int real_count = 0; + int i; + + if (worker->monitors_config) { +@@ -10968,19 +10967,7 @@ static void worker_update_monitors_config(RedWorker *worker, + dev_monitors_config->heads[i].width, + dev_monitors_config->heads[i].height); + } +- +- // Ignore any empty sized monitors at the end of the config. +- // 4: {w1,h1},{w2,h2},{0,0},{0,0} -> 2: {w1,h1},{w2,h2} +- for (i = dev_monitors_config->count ; i > 0 ; --i) { +- if (dev_monitors_config->heads[i - 1].width > 0 && +- dev_monitors_config->heads[i - 1].height > 0) { +- real_count = i; +- break; +- } +- } +- heads_size = real_count * sizeof(QXLHead); +- spice_debug("new working monitor config (count: %d, real: %d)", +- dev_monitors_config->count, real_count); ++ heads_size = dev_monitors_config->count * sizeof(QXLHead); + worker->monitors_config = monitors_config = + spice_malloc(sizeof(*monitors_config) + heads_size); + monitors_config->refs = 1; +-- +1.8.1 + diff --git a/spice.spec b/spice.spec index cffd1cf..99e7edd 100644 --- a/spice.spec +++ b/spice.spec @@ -8,13 +8,19 @@ Name: spice Version: 0.12.2 -Release: 2%{?dist} +Release: 3%{?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 Source1: spice-xpi-client-spicec +Patch1: 0001-server-guest_set_client_capabilities-protect-against.patch +Patch2: 0002-red_worker.c-insert-a-drawable-to-its-position-in-th.patch +Patch3: 0003-red_worker.c-clearing-the-stream-vis_region-after-it.patch +Patch4: 0004-link-libspice-server-with-libm-libpthread.patch +Patch5: 0005-server-Fix-SpiceWorker-CRITICAL-red_worker.c-10968-r.patch +Patch6: 0006-worker_update_monitors_config-Drop-bogus-real_count-.patch # https://bugzilla.redhat.com/show_bug.cgi?id=613529 %if 0%{?rhel} @@ -89,6 +95,12 @@ 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 %build @@ -148,6 +160,9 @@ fi %changelog +* Mon Jan 21 2013 Hans de Goede - 0.12.2-3 +- Add a number of misc. bug-fixes from upstream + * Fri Dec 21 2012 Adam Tkac - 0.12.2-2 - rebuild against new libjpeg