dri: Prevent abuse of the Resource database

The Resource database is only designed to store a single value for a
particular type associated with an XID. Due to the asynchronous nature
of the vblank/flip requests, we would often associate multiple frame
events with a particular drawable/client. Upon freeing the resource, we
would not necessarily decouple the right value, leaving a stale pointer
behind. Later when the client disappeared, we would write through that
stale pointer upsetting valgrind and causing memory corruption. MDK.

Instead, we need to implement an extra layer for tracking multiple
frames within a single Resource.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=37700
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

3 files changed, 176 insertions, 42 deletions

diff --git a/src/intel.h b/src/intel.h
index 2b114c37..9d64d30f 100644
--- a/src/intel.h
+++ b/src/intel.h
@@ -476,11 +476,12 @@ typedef void (*DRI2SwapEventPtr)(ClientPtr client, void *data, int type,
 
 typedef struct _DRI2FrameEvent {
 	XID drawable_id;
-	XID client_id;	/* fake client ID to track client destruction */
 	ClientPtr client;
 	enum DRI2FrameEventType type;
 	int frame;
 
+	struct list drawable_resource, client_resource;
+
 	/* for swaps & flips only */
 	DRI2SwapEventPtr event_complete;
 	void *event_data;
diff --git a/src/intel_dri.c b/src/intel_dri.c
index a6be07f6..38d7a6bc 100644
--- a/src/intel_dri.c
+++ b/src/intel_dri.c
@@ -71,6 +71,8 @@ typedef struct {
 	PixmapPtr pixmap;
 } I830DRI2BufferPrivateRec, *I830DRI2BufferPrivatePtr;
 
+static DevPrivateKeyRec i830_client_key;
+
 static uint32_t pixmap_flink(PixmapPtr pixmap)
 {
 	struct intel_pixmap *priv = intel_get_pixmap_private(pixmap);
@@ -608,22 +610,73 @@ I830DRI2DrawablePipe(DrawablePtr pDraw)
 
 static RESTYPE	frame_event_client_type, frame_event_drawable_type;
 
+struct i830_dri2_resource {
+	XID id;
+	RESTYPE type;
+	struct list list;
+};
+
+static struct i830_dri2_resource *
+get_resource(XID id, RESTYPE type)
+{
+	struct i830_dri2_resource *resource;
+	void *ptr;
+
+	ptr = NULL;
+	dixLookupResourceByType(&ptr, id, type, NULL, DixWriteAccess);
+	if (ptr)
+		return ptr;
+
+	resource = malloc(sizeof(*resource));
+	if (resource == NULL)
+		return NULL;
+
+	if (!AddResource(id, type, resource)) {
+		free(resource);
+		return NULL;
+	}
+
+	resource->id = id;
+	resource->type = type;
+	list_init(&resource->list);
+	return resource;
+}
+
 static int
 i830_dri2_frame_event_client_gone(void *data, XID id)
 {
-	DRI2FrameEventPtr	frame_event = data;
+	struct i830_dri2_resource *resource = data;
+
+	while (!list_is_empty(&resource->list)) {
+		DRI2FrameEventPtr info =
+			list_first_entry(&resource->list,
+					 DRI2FrameEventRec,
+					 client_resource);
+
+		list_del(&info->client_resource);
+		info->client = NULL;
+	}
+	free(resource);
 
-	frame_event->client = NULL;
-	frame_event->client_id = None;
 	return Success;
 }
 
 static int
 i830_dri2_frame_event_drawable_gone(void *data, XID id)
 {
-	DRI2FrameEventPtr	frame_event = data;
+	struct i830_dri2_resource *resource = data;
+
+	while (!list_is_empty(&resource->list)) {
+		DRI2FrameEventPtr info =
+			list_first_entry(&resource->list,
+					 DRI2FrameEventRec,
+					 drawable_resource);
+
+		list_del(&info->drawable_resource);
+		info->drawable_id = None;
+	}
+	free(resource);
 
-	frame_event->drawable_id = None;
 	return Success;
 }
 
@@ -641,34 +694,48 @@ i830_dri2_register_frame_event_resource_types(void)
 	return TRUE;
 }
 
+static XID
+get_client_id(ClientPtr client)
+{
+	XID *ptr = dixGetPrivateAddr(&client->devPrivates, &i830_client_key);
+	if (*ptr == 0)
+		*ptr = FakeClientID(client->index);
+	return *ptr;
+}
+
 /*
  * Hook this frame event into the server resource
  * database so we can clean it up if the drawable or
  * client exits while the swap is pending
  */
 static Bool
-i830_dri2_add_frame_event(DRI2FrameEventPtr frame_event)
+i830_dri2_add_frame_event(DRI2FrameEventPtr info)
 {
-	frame_event->client_id = FakeClientID(frame_event->client->index);
+	struct i830_dri2_resource *resource;
 
-	if (!AddResource(frame_event->client_id, frame_event_client_type, frame_event))
+	resource = get_resource(get_client_id(info->client),
+				frame_event_client_type);
+	if (resource == NULL)
 		return FALSE;
 
-	if (!AddResource(frame_event->drawable_id, frame_event_drawable_type, frame_event)) {
-		FreeResourceByType(frame_event->client_id, frame_event_client_type, TRUE);
+	list_add(&info->client_resource, &resource->list);
+
+	resource = get_resource(info->drawable_id, frame_event_drawable_type);
+	if (resource == NULL) {
+		list_del(&info->client_resource);
 		return FALSE;
 	}
 
+	list_add(&info->drawable_resource, &resource->list);
+
 	return TRUE;
 }
 
 static void
-i830_dri2_del_frame_event(DRI2FrameEventPtr frame_event)
+i830_dri2_del_frame_event(DRI2FrameEventPtr info)
 {
-	if (frame_event->client_id)
-		FreeResourceByType(frame_event->client_id, frame_event_client_type, TRUE);
-	if (frame_event->drawable_id)
-		FreeResourceByType(frame_event->drawable_id, frame_event_drawable_type, TRUE);
+	list_del(&info->client_resource);
+	list_del(&info->drawable_resource);
 }
 
 static void
@@ -1350,6 +1417,10 @@ Bool I830DRI2ScreenInit(ScreenPtr screen)
 		return FALSE;
 	}
 
+	if (!dixRegisterPrivateKey(&i830_client_key, PRIVATE_CLIENT, sizeof(XID)))
+		return FALSE;
+
+
 #if DRI2INFOREC_VERSION >= 4
 	if (serverGeneration != dri2_server_generation) {
 	    dri2_server_generation = serverGeneration;
diff --git a/src/sna/sna_dri.c b/src/sna/sna_dri.c
index a6cc9ceb..f5d81b3e 100644
--- a/src/sna/sna_dri.c
+++ b/src/sna/sna_dri.c
@@ -93,16 +93,24 @@ struct sna_dri_private {
 	struct kgem_bo *bo;
 };
 
+struct sna_dri_resource {
+	XID id;
+	RESTYPE type;
+	struct list list;
+};
+
 struct sna_dri_frame_event {
 	struct sna *sna;
 	XID drawable_id;
-	XID client_id;	/* fake client ID to track client destruction */
 	ClientPtr client;
 	enum frame_event_type type;
 	int frame;
 	int pipe;
 	int count;
 
+	struct list drawable_resource;
+	struct list client_resource;
+
 	/* for swaps & flips only */
 	DRI2SwapEventPtr event_complete;
 	void *event_data;
@@ -120,6 +128,8 @@ struct sna_dri_frame_event {
 	struct sna_dri_frame_event *chain;
 };
 
+static DevPrivateKeyRec sna_client_key;
+
 static inline struct sna_dri_frame_event *
 to_frame_event(void *data)
 {
@@ -531,22 +541,67 @@ sna_dri_get_pipe(DrawablePtr pDraw)
 
 static RESTYPE frame_event_client_type, frame_event_drawable_type;
 
+static struct sna_dri_resource *
+get_resource(XID id, RESTYPE type)
+{
+	struct sna_dri_resource *resource;
+	void *ptr;
+
+	ptr = NULL;
+	dixLookupResourceByType(&ptr, id, type, NULL, DixWriteAccess);
+	if (ptr)
+		return ptr;
+
+	resource = malloc(sizeof(*resource));
+	if (resource == NULL)
+		return NULL;
+
+	if (!AddResource(id, type, resource)) {
+		free(resource);
+		return NULL;
+	}
+
+	resource->id = id;
+	resource->type = type;
+	list_init(&resource->list);
+	return resource;
+}
+
 static int
 sna_dri_frame_event_client_gone(void *data, XID id)
 {
-	struct sna_dri_frame_event *frame_event = data;
+	struct sna_dri_resource *resource = data;
+
+	while (!list_is_empty(&resource->list)) {
+		struct sna_dri_frame_event *info =
+			list_first_entry(&resource->list,
+					 struct sna_dri_frame_event,
+					 client_resource);
+
+		list_del(&info->client_resource);
+		info->client = NULL;
+	}
+	free(resource);
 
-	frame_event->client = NULL;
-	frame_event->client_id = None;
 	return Success;
 }
 
 static int
 sna_dri_frame_event_drawable_gone(void *data, XID id)
 {
-	struct sna_dri_frame_event *frame_event = data;
+	struct sna_dri_resource *resource = data;
+
+	while (!list_is_empty(&resource->list)) {
+		struct sna_dri_frame_event *info =
+			list_first_entry(&resource->list,
+					 struct sna_dri_frame_event,
+					 drawable_resource);
+
+		list_del(&info->drawable_resource);
+		info->drawable_id = None;
+	}
+	free(resource);
 
-	frame_event->drawable_id = None;
 	return Success;
 }
 
@@ -568,45 +623,48 @@ sna_dri_register_frame_event_resource_types(void)
 	return TRUE;
 }
 
+static XID
+get_client_id(ClientPtr client)
+{
+	XID *ptr = dixGetPrivateAddr(&client->devPrivates, &sna_client_key);
+	if (*ptr == 0)
+		*ptr = FakeClientID(client->index);
+	return *ptr;
+}
+
 /*
  * Hook this frame event into the server resource
  * database so we can clean it up if the drawable or
  * client exits while the swap is pending
  */
 static Bool
-sna_dri_add_frame_event(struct sna_dri_frame_event *frame_event)
+sna_dri_add_frame_event(struct sna_dri_frame_event *info)
 {
-	frame_event->client_id = FakeClientID(frame_event->client->index);
+	struct sna_dri_resource *resource;
 
-	if (!AddResource(frame_event->client_id,
-			 frame_event_client_type,
-			 frame_event))
+	resource = get_resource(get_client_id(info->client),
+				frame_event_client_type);
+	if (resource == NULL)
 		return FALSE;
 
-	if (!AddResource(frame_event->drawable_id,
-			 frame_event_drawable_type,
-			 frame_event)) {
-		FreeResourceByType(frame_event->client_id,
-				   frame_event_client_type,
-				   TRUE);
+	list_add(&info->client_resource, &resource->list);
+
+	resource = get_resource(info->drawable_id, frame_event_drawable_type);
+	if (resource == NULL) {
+		list_del(&info->client_resource);
 		return FALSE;
 	}
 
+	list_add(&info->drawable_resource, &resource->list);
+
 	return TRUE;
 }
 
 static void
 sna_dri_frame_event_info_free(struct sna_dri_frame_event *info)
 {
-	if (info->client_id)
-		FreeResourceByType(info->client_id,
-				   frame_event_client_type,
-				   TRUE);
-
-	if (info->drawable_id)
-		FreeResourceByType(info->drawable_id,
-				   frame_event_drawable_type,
-				   TRUE);
+	list_del(&info->client_resource);
+	list_del(&info->drawable_resource);
 
 	if (info->front)
 		_sna_dri_destroy_buffer(info->sna, info->front);
@@ -1606,6 +1664,10 @@ Bool sna_dri_open(struct sna *sna, ScreenPtr screen)
 		return FALSE;
 	    }
 	}
+
+	if (!dixRegisterPrivateKey(&sna_client_key, PRIVATE_CLIENT, sizeof(XID)))
+		return FALSE;
+
 	sna->deviceName = drmGetDeviceNameFromFd(sna->kgem.fd);
 	memset(&info, '\0', sizeof(info));
 	info.fd = sna->kgem.fd;