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
@@ -478,3 +478,2 @@ typedef struct _DRI2FrameEvent {
 	XID drawable_id;
-	XID client_id;	/* fake client ID to track client destruction */
 	ClientPtr client;
@@ -483,2 +482,4 @@ typedef struct _DRI2FrameEvent {
 
+	struct list drawable_resource, client_resource;
+
 	/* for swaps & flips only */
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
@@ -73,2 +73,4 @@ typedef struct {
 
+static DevPrivateKeyRec i830_client_key;
+
 static uint32_t pixmap_flink(PixmapPtr pixmap)
@@ -610,2 +612,34 @@ 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
@@ -613,6 +647,15 @@ 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;
@@ -623,5 +666,15 @@ 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;
@@ -643,2 +696,11 @@ i830_dri2_register_frame_event_resource_types(void)
 
+static XID
+get_client_id(ClientPtr client)
+{
+	XID *ptr = dixGetPrivateAddr(&client->devPrivates, &i830_client_key);
+	if (*ptr == 0)
+		*ptr = FakeClientID(client->index);
+	return *ptr;
+}
+
 /*
@@ -649,11 +711,16 @@ i830_dri2_register_frame_event_resource_types(void)
 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;
@@ -661,2 +728,4 @@ i830_dri2_add_frame_event(DRI2FrameEventPtr frame_event)
 
+	list_add(&info->drawable_resource, &resource->list);
+
 	return TRUE;
@@ -665,8 +734,6 @@ i830_dri2_add_frame_event(DRI2FrameEventPtr frame_event)
 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);
 }
@@ -1352,2 +1419,6 @@ Bool I830DRI2ScreenInit(ScreenPtr screen)
 
+	if (!dixRegisterPrivateKey(&i830_client_key, PRIVATE_CLIENT, sizeof(XID)))
+		return FALSE;
+
+
 #if DRI2INFOREC_VERSION >= 4
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
@@ -95,2 +95,8 @@ struct sna_dri_private {
 
+struct sna_dri_resource {
+	XID id;
+	RESTYPE type;
+	struct list list;
+};
+
 struct sna_dri_frame_event {
@@ -98,3 +104,2 @@ struct sna_dri_frame_event {
 	XID drawable_id;
-	XID client_id;	/* fake client ID to track client destruction */
 	ClientPtr client;
@@ -105,2 +110,5 @@ struct sna_dri_frame_event {
 
+	struct list drawable_resource;
+	struct list client_resource;
+
 	/* for swaps & flips only */
@@ -122,2 +130,4 @@ struct sna_dri_frame_event {
 
+static DevPrivateKeyRec sna_client_key;
+
 static inline struct sna_dri_frame_event *
@@ -533,2 +543,28 @@ 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
@@ -536,6 +572,15 @@ 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;
@@ -546,5 +591,15 @@ 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;
@@ -570,2 +625,11 @@ sna_dri_register_frame_event_resource_types(void)
 
+static XID
+get_client_id(ClientPtr client)
+{
+	XID *ptr = dixGetPrivateAddr(&client->devPrivates, &sna_client_key);
+	if (*ptr == 0)
+		*ptr = FakeClientID(client->index);
+	return *ptr;
+}
+
 /*
@@ -576,17 +640,16 @@ sna_dri_register_frame_event_resource_types(void)
 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;
@@ -594,2 +657,4 @@ sna_dri_add_frame_event(struct sna_dri_frame_event *frame_event)
 
+	list_add(&info->drawable_resource, &resource->list);
+
 	return TRUE;
@@ -600,11 +665,4 @@ 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);
 
@@ -1608,2 +1666,6 @@ Bool sna_dri_open(struct sna *sna, ScreenPtr screen)
 	}
+
+	if (!dixRegisterPrivateKey(&sna_client_key, PRIVATE_CLIENT, sizeof(XID)))
+		return FALSE;
+
 	sna->deviceName = drmGetDeviceNameFromFd(sna->kgem.fd);