[spice] Add a number of misc. bug-fixes from upstream
Hans de Goede
jwrdegoede at fedoraproject.org
Mon Jan 21 15:07:28 UTC 2013
commit 708002ccbc39dd6fa9b1f99da654ecc474b2967a
Author: Hans de Goede <hdegoede at redhat.com>
Date: Mon Jan 21 16:09:59 2013 +0100
Add a number of misc. bug-fixes from upstream
- Add a number of misc. bug-fixes from upstream
...t_set_client_capabilities-protect-against.patch | 36 +++++
...c-insert-a-drawable-to-its-position-in-th.patch | 166 ++++++++++++++++++++
...c-clearing-the-stream-vis_region-after-it.patch | 39 +++++
...link-libspice-server-with-libm-libpthread.patch | 32 ++++
...SpiceWorker-CRITICAL-red_worker.c-10968-r.patch | 77 +++++++++
...te_monitors_config-Drop-bogus-real_count-.patch | 56 +++++++
spice.spec | 17 ++-
7 files changed, 422 insertions(+), 1 deletions(-)
---
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 <uril at redhat.com>
+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 <mluscon at redhat.com>
+
+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 <yhalperi at redhat.com>
+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 <yhalperi at redhat.com>
+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 <mjt at tls.msk.ru>
+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 <mjt at tls.msk.ru>
+---
+ 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 <hdegoede at redhat.com>
+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=<optimized out>, client=<optimized out>, worker=<optimized out>)
+ 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 <hdegoede at redhat.com>
+---
+ 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 <hdegoede at redhat.com>
+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 <hdegoede at redhat.com>
+---
+ 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 <hdegoede at redhat.com> - 0.12.2-3
+- Add a number of misc. bug-fixes from upstream
+
* Fri Dec 21 2012 Adam Tkac <atkac redhat com> - 0.12.2-2
- rebuild against new libjpeg
More information about the scm-commits
mailing list