Prevent access to freed memory when:
1. qxl_leave_vt/qxl_surface_cache_evacuate_all freed cache->all_surfaces
2. ProcRenderDispatch/damageDestroyPixmap/qxl_destroy_pixmap/qxl_surface_kill
access a surface that pointed inside the all_surfaces array
Solution in this patch:
1. never free all_surfaces
2. add an 'evacuated' field per surface, initialied to NULL, set during
evacuation.
3. on qxl_surface_kill, if surface->evacuated is set, don't destroy the
surface (it is already destroyed by this point via a reset in
qxl_surface_cache_evacuate_all's caller, qxl_leave_vt), just unref the
host pixmap, free the evacuated_surface_t and unlink it from the
evacuated linked list, so it isn't recreated later on
qxl_surface_cache_replace_all.
---
src/qxl_surface.c | 44 +++++++++++++++++++++++++++++++++++++-------
1 file changed, 37 insertions(+), 7 deletions(-)
diff --git a/src/qxl_surface.c b/src/qxl_surface.c
index cb0de1d..d999b4d 100644
--- a/src/qxl_surface.c
+++ b/src/qxl_surface.c
@@ -76,7 +76,9 @@ struct qxl_surface_t
int ref_count;
PixmapPtr pixmap;
-
+
+ struct evacuated_surface_t *evacuated;
+
union
{
qxl_surface_t *copy_src;
@@ -90,6 +92,7 @@ struct evacuated_surface_t
PixmapPtr pixmap;
int bpp;
+ evacuated_surface_t *prev;
evacuated_surface_t *next;
};
@@ -125,9 +128,14 @@ surface_cache_init (surface_cache_t *cache, qxl_screen_t *qxl)
int n_surfaces = qxl->rom->n_surfaces;
int i;
- cache->all_surfaces = calloc (n_surfaces, sizeof (qxl_surface_t));
- if (!cache->all_surfaces)
- return FALSE;
+ if (!cache->all_surfaces) {
+ /* all_surfaces is not freed when evacuating, since surfaces are still
+ * tied to pixmaps that may be destroyed after evacuation before
+ * recreation */
+ cache->all_surfaces = calloc (n_surfaces, sizeof (qxl_surface_t));
+ if (!cache->all_surfaces)
+ return FALSE;
+ }
memset (cache->all_surfaces, 0, n_surfaces * sizeof (qxl_surface_t));
memset (cache->cached_surfaces, 0, N_CACHED_SURFACES * sizeof (qxl_surface_t *));
@@ -141,6 +149,7 @@ surface_cache_init (surface_cache_t *cache, qxl_screen_t *qxl)
cache->all_surfaces[i].cache = cache;
cache->all_surfaces[i].dev_image = NULL;
cache->all_surfaces[i].host_image = NULL;
+ cache->all_surfaces[i].evacuated = NULL;
REGION_INIT (
NULL, &(cache->all_surfaces[i].access_region), (BoxPtr)NULL, 0);
@@ -165,6 +174,7 @@ qxl_surface_cache_create (qxl_screen_t *qxl)
if (!cache)
return NULL;
+ memset(cache, 0, sizeof(*cache));
cache->qxl = qxl;
if (!surface_cache_init (cache, qxl))
{
@@ -344,6 +354,7 @@ qxl_surface_cache_create_primary (surface_cache_t *cache,
surface->bpp = mode->bits;
surface->next = NULL;
surface->prev = NULL;
+ surface->evacuated = NULL;
REGION_INIT (NULL, &(surface->access_region), (BoxPtr)NULL, 0);
surface->access_type = UXA_ACCESS_RO;
@@ -790,6 +801,23 @@ qxl_surface_unref (surface_cache_t *cache, uint32_t id)
void
qxl_surface_kill (qxl_surface_t *surface)
{
+ struct evacuated_surface_t *ev = surface->evacuated;
+
+ if (ev) {
+ /* server side surface is already destroyed (via reset), don't
+ * resend a destroy. Just mark surface as not to be recreated */
+ ev->pixmap = NULL;
+ if (ev->image)
+ pixman_image_unref (ev->image);
+ if (ev->next)
+ ev->next->prev = ev->prev;
+ if (ev->prev)
+ ev->prev->next = ev->next;
+ free(ev);
+ surface->evacuated = NULL;
+ return;
+ }
+
unlink_surface (surface);
if (surface->id != 0 &&
@@ -1041,16 +1069,18 @@ qxl_surface_cache_evacuate_all (surface_cache_t *cache)
unlink_surface (s);
evacuated->next = evacuated_surfaces;
+ if (evacuated_surfaces) {
+ evacuated_surfaces->prev = evacuated;
+ }
evacuated_surfaces = evacuated;
+ s->evacuated = evacuated;
s = next;
}
- free (cache->all_surfaces);
- cache->all_surfaces = NULL;
cache->live_surfaces = NULL;
cache->free_surfaces = NULL;
-
+
return evacuated_surfaces;
}
--
1.7.10.1
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel