Attachment #188365 for bug #199872

Lines 3-9 Link Here

(-)Makefile (-1 / +1 lines)
3		3
4	PORTNAME= glib	4	PORTNAME= glib
5	PORTVERSION= 2.50.2	5	PORTVERSION= 2.50.2
6	PORTREVISION= 7	6	PORTREVISION= 8
7	PORTEPOCH= 1	7	PORTEPOCH= 1
8	CATEGORIES= devel	8	CATEGORIES= devel
9	MASTER_SITES= GNOME	9	MASTER_SITES= GNOME




From 165c867f83b7c479a0352b8adde900f36739dcc6 Mon Sep 17 00:00:00 2001
From: Martin Pieuchot <mpi@openbsd.org>
Date: Fri, 28 Apr 2017 15:06:52 +0200
Subject: [PATCH] kqueue: fix use-after-free of ``kqueue_sub''.

To prevent such crash, make sure the threads are holding ``hash_lock''
when manipulating such items.
---
 gio/kqueue/gkqueuefilemonitor.c | 14 ++++++++++++++
 gio/kqueue/kqueue-helper.c      | 10 +++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/gio/kqueue/gkqueuefilemonitor.c b/gio/kqueue/gkqueuefilemonitor.c
index 78b749637..bd55e2e70 100644
--- gio/kqueue/gkqueuefilemonitor.c
+++ gio/kqueue/gkqueuefilemonitor.c
@@ -30,6 +30,16 @@
 #include <gio/giomodule.h>
 
 
+/*
+ * Because ``kqueue_sub'' are not refcounted, we need
+ * ensure no other thread is getting a reference to
+ * the element we want to free.
+ *
+ * That's why _kh_cancel_sub() must be called with
+ * this lock held to prevent a race.
+ */
+G_LOCK_EXTERN (hash_lock);
+
 struct _GKqueueFileMonitor
 {
   GLocalFileMonitor parent_instance;
@@ -80,9 +90,11 @@ g_kqueue_file_monitor_finalize (GObject *object)
 
   if (kqueue_monitor->sub)
     {
+      G_LOCK (hash_lock);
       _kh_cancel_sub (kqueue_monitor->sub);
       _kh_sub_free (kqueue_monitor->sub);
       kqueue_monitor->sub = NULL;
+      G_UNLOCK (hash_lock);
     }
 
   if (kqueue_monitor->fallback)
@@ -181,9 +193,11 @@ g_kqueue_file_monitor_cancel (GFileMonitor *monitor)
 
   if (kqueue_monitor->sub)
     {
+      G_LOCK (hash_lock);
       _kh_cancel_sub (kqueue_monitor->sub);
       _kh_sub_free (kqueue_monitor->sub);
       kqueue_monitor->sub = NULL;
+      G_UNLOCK (hash_lock);
     }
   else if (kqueue_monitor->fallback)
     {
diff --git a/gio/kqueue/kqueue-helper.c b/gio/kqueue/kqueue-helper.c
index d4e66cd4d..f8267464b 100644
--- gio/kqueue/kqueue-helper.c
+++ gio/kqueue/kqueue-helper.c
@@ -43,7 +43,7 @@ static gboolean kh_debug_enabled = FALSE;
 #define KH_W if (kh_debug_enabled) g_warning
 
 static GHashTable *subs_hash_table = NULL;
-G_LOCK_DEFINE_STATIC (hash_lock);
+G_LOCK_DEFINE (hash_lock);
 
 static int kqueue_descriptor = -1;
 static int kqueue_socket_pair[] = {-1, -1};
@@ -291,10 +291,10 @@ process_kqueue_notifications (GIOChannel   *gioc,
 
   G_LOCK (hash_lock);

   return TRUE;
 }
 
@@ -504,9 +506,7 @@ _kh_cancel_sub (kqueue_sub *sub)
 
   _km_remove (sub);
 
-  G_LOCK (hash_lock);
   removed = g_hash_table_remove (subs_hash_table, GINT_TO_POINTER (sub->fd));
-  G_UNLOCK (hash_lock);
 
   if (removed)
     {
-- 
2.12.2





From 22656f16c29591207c667362e2a42fd348fe8494 Mon Sep 17 00:00:00 2001
From: Martin Pieuchot <mpi@openbsd.org>
Date: Fri, 28 Apr 2017 15:06:52 +0200
Subject: [PATCH] kqueue: fix use-after-free of ``kqueue_sub''.

Since ``kqueue_sub'' are not refcounted it is common to see a thread
freeing one of them while another thread is manipulating them.  This
leads to crashs reported in:
	https://bugzilla.gnome.org/show_bug.cgi?id=739424

To prevent such crash, make sure the threads are holding ``hash_lock''
when manipulating such items.
---
 gio/kqueue/kqueue-helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gio/kqueue/kqueue-helper.c b/gio/kqueue/kqueue-helper.c
index d4e66cd4d..84b9ef164 100644
--- gio/kqueue/kqueue-helper.c
+++ gio/kqueue/kqueue-helper.c
@@ -291,10 +291,10 @@ process_kqueue_notifications (GIOChannel   *gioc,
 
   G_LOCK (hash_lock);
   sub = (kqueue_sub *) g_hash_table_lookup (subs_hash_table, GINT_TO_POINTER (n.fd));
-  G_UNLOCK (hash_lock);
 
   if (sub == NULL)
     {
+      G_UNLOCK (hash_lock);
       KH_W ("Got a notification for a deleted or non-existing subscription %d",
              n.fd);
       return TRUE;
@@ -336,6 +336,7 @@ process_kqueue_notifications (GIOChannel   *gioc,
         g_file_monitor_source_handle_event (source, mask, NULL, NULL, NULL, g_get_monotonic_time ());
     }
 
+  G_UNLOCK (hash_lock);
   return TRUE;
 }
 
@@ -451,13 +452,14 @@ _kh_start_watching (kqueue_sub *sub)
 
   G_LOCK (hash_lock);
   g_hash_table_insert (subs_hash_table, GINT_TO_POINTER (sub->fd), sub);
-  G_UNLOCK (hash_lock);
 
   _kqueue_thread_push_fd (sub->fd);
   
   /* Bump the kqueue thread. It will pick up a new sub entry to monitor */
   if (!_ku_write (kqueue_socket_pair[0], "A", 1))
     KH_W ("Failed to bump the kqueue thread (add fd, error %d)", errno);
+  G_UNLOCK (hash_lock);
+
   return TRUE;
 }
 
-- 
2.12.2





From e305fe971e4647d971428a772b7290b9c308a96f Mon Sep 17 00:00:00 2001
From: Steven McDonald <steven@steven-mcdonald.id.au>
Date: Sun, 12 Feb 2017 11:02:55 +1100
Subject: gio: Always purge kqueue subs from missing list

Previously, _kh_cancel_sub assumed that it only needed to call
_km_remove if sub did not exist in subs_hash_table. This is erroneous
because the complementary operation, _km_add_missing, can be called
from process_kqueue_notifications, in which context sub can *only* have
come from subs_hash_table.

Since _km_remove is implemented using g_slist_remove, which is
documented to be a noop if the list does not contain the element to be
removed, it is safe to call _km_remove unconditionally here.

https://bugzilla.gnome.org/show_bug.cgi?id=778515
---
 gio/kqueue/kqueue-helper.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/gio/kqueue/kqueue-helper.c b/gio/kqueue/kqueue-helper.c
index 4671396..d4e66cd 100644
--- gio/kqueue/kqueue-helper.c
+++ gio/kqueue/kqueue-helper.c
@@ -498,22 +498,17 @@ _kh_add_sub (kqueue_sub *sub)
 gboolean
 _kh_cancel_sub (kqueue_sub *sub)
 {
-  gboolean missing = FALSE;
+  gboolean removed = FALSE;
   g_assert (kqueue_socket_pair[0] != -1);
   g_assert (sub != NULL);
 
+  _km_remove (sub);
+
   G_LOCK (hash_lock);
-  missing = !g_hash_table_remove (subs_hash_table, GINT_TO_POINTER (sub->fd));
+  removed = g_hash_table_remove (subs_hash_table, GINT_TO_POINTER (sub->fd));
   G_UNLOCK (hash_lock);
 
-  if (missing)
-    {
-      /* If there were no fd for this subscription, file is still
-       * missing. */
-      KH_W ("Removing subscription from missing");
-      _km_remove (sub);
-    }
-  else
+  if (removed)
     {
       /* fd will be closed in the kqueue thread */
       _kqueue_thread_remove_fd (sub->fd);
-- 
cgit v0.12





diff --git a/gio/kqueue/kqueue-helper.c b/gio/kqueue/kqueue-helper.c
index d4e66cd4d..e7d583c8b 100644
--- gio/kqueue/kqueue-helper.c
+++ gio/kqueue/kqueue-helper.c
@@ -97,8 +97,10 @@ convert_kqueue_events_to_gio (uint32_t flags, gboolean *done)
     }
   if (flags & NOTE_RENAME)
     {
+      /* Since there’s apparently no way to get the new name of the file out of
+       * kqueue(), all we can do is say that this one has been deleted. */
       *done = TRUE;
-      return G_FILE_MONITOR_EVENT_MOVED;
+      return G_FILE_MONITOR_EVENT_DELETED;
     }
   if (flags & NOTE_REVOKE)
     {

Return to bug 199872

Lines 1-4 Link Here

(-)files/patch-bug-b-739424 (-4 / +69 lines)
1	From 22656f16c29591207c667362e2a42fd348fe8494 Mon Sep 17 00:00:00 2001	1	From 165c867f83b7c479a0352b8adde900f36739dcc6 Mon Sep 17 00:00:00 2001
2	From: Martin Pieuchot <mpi@openbsd.org>	2	From: Martin Pieuchot <mpi@openbsd.org>
3	Date: Fri, 28 Apr 2017 15:06:52 +0200	3	Date: Fri, 28 Apr 2017 15:06:52 +0200
4	Subject: [PATCH] kqueue: fix use-after-free of ``kqueue_sub''.	4	Subject: [PATCH] kqueue: fix use-after-free of ``kqueue_sub''.
Lines 11-23 Link Here
11	To prevent such crash, make sure the threads are holding ``hash_lock''	11	To prevent such crash, make sure the threads are holding ``hash_lock''
12	when manipulating such items.	12	when manipulating such items.
13	---	13	---
14	gio/kqueue/kqueue-helper.c \| 6 ++++--	14	gio/kqueue/gkqueuefilemonitor.c \| 14 ++++++++++++++
15	1 file changed, 4 insertions(+), 2 deletions(-)	15	gio/kqueue/kqueue-helper.c \| 10 +++++-----
		16	2 files changed, 19 insertions(+), 5 deletions(-)
16		17
		18	diff --git a/gio/kqueue/gkqueuefilemonitor.c b/gio/kqueue/gkqueuefilemonitor.c
		19	index 78b749637..bd55e2e70 100644
		20	--- gio/kqueue/gkqueuefilemonitor.c
		21	+++ gio/kqueue/gkqueuefilemonitor.c
		22	@@ -30,6 +30,16 @@
		23	#include <gio/giomodule.h>
		24
		25
		26	+/*
		27	+ * Because ``kqueue_sub'' are not refcounted, we need
		28	+ * ensure no other thread is getting a reference to
		29	+ * the element we want to free.
		30	+ *
		31	+ * That's why _kh_cancel_sub() must be called with
		32	+ * this lock held to prevent a race.
		33	+ */
		34	+G_LOCK_EXTERN (hash_lock);
		35	+
		36	struct _GKqueueFileMonitor
		37	{
		38	GLocalFileMonitor parent_instance;
		39	@@ -80,9 +90,11 @@ g_kqueue_file_monitor_finalize (GObject *object)
		40
		41	if (kqueue_monitor->sub)
		42	{
		43	+ G_LOCK (hash_lock);
		44	_kh_cancel_sub (kqueue_monitor->sub);
		45	_kh_sub_free (kqueue_monitor->sub);
		46	kqueue_monitor->sub = NULL;
		47	+ G_UNLOCK (hash_lock);
		48	}
		49
		50	if (kqueue_monitor->fallback)
		51	@@ -181,9 +193,11 @@ g_kqueue_file_monitor_cancel (GFileMonitor *monitor)
		52
		53	if (kqueue_monitor->sub)
		54	{
		55	+ G_LOCK (hash_lock);
		56	_kh_cancel_sub (kqueue_monitor->sub);
		57	_kh_sub_free (kqueue_monitor->sub);
		58	kqueue_monitor->sub = NULL;
		59	+ G_UNLOCK (hash_lock);
		60	}
		61	else if (kqueue_monitor->fallback)
		62	{
17	diff --git a/gio/kqueue/kqueue-helper.c b/gio/kqueue/kqueue-helper.c	63	diff --git a/gio/kqueue/kqueue-helper.c b/gio/kqueue/kqueue-helper.c
18	index d4e66cd4d..84b9ef164 100644	64	index d4e66cd4d..f8267464b 100644
19	--- gio/kqueue/kqueue-helper.c	65	--- gio/kqueue/kqueue-helper.c
20	+++ gio/kqueue/kqueue-helper.c	66	+++ gio/kqueue/kqueue-helper.c
		67	@@ -43,7 +43,7 @@ static gboolean kh_debug_enabled = FALSE;
		68	#define KH_W if (kh_debug_enabled) g_warning
		69
		70	static GHashTable *subs_hash_table = NULL;
		71	-G_LOCK_DEFINE_STATIC (hash_lock);
		72	+G_LOCK_DEFINE (hash_lock);
		73
		74	static int kqueue_descriptor = -1;
		75	static int kqueue_socket_pair[] = {-1, -1};
21	@@ -291,10 +291,10 @@ process_kqueue_notifications (GIOChannel *gioc,	76	@@ -291,10 +291,10 @@ process_kqueue_notifications (GIOChannel *gioc,
22		77
23	G_LOCK (hash_lock);	78	G_LOCK (hash_lock);
Lines 54-59 Link Here
54	return TRUE;	109	return TRUE;
55	}	110	}
56		111
		112	@@ -504,9 +506,7 @@ _kh_cancel_sub (kqueue_sub *sub)
		113
		114	_km_remove (sub);
		115
		116	- G_LOCK (hash_lock);
		117	removed = g_hash_table_remove (subs_hash_table, GINT_TO_POINTER (sub->fd));
		118	- G_UNLOCK (hash_lock);
		119
		120	if (removed)
		121	{
57	--	122	--
58	2.12.2	123	2.12.2
59		124

Lines 1-59 Link Here

(-)files/patch-bug739424 (-59 lines)
1	From 22656f16c29591207c667362e2a42fd348fe8494 Mon Sep 17 00:00:00 2001
2	From: Martin Pieuchot <mpi@openbsd.org>
3	Date: Fri, 28 Apr 2017 15:06:52 +0200
4	Subject: [PATCH] kqueue: fix use-after-free of ``kqueue_sub''.
5
6	Since ``kqueue_sub'' are not refcounted it is common to see a thread
7	freeing one of them while another thread is manipulating them. This
8	leads to crashs reported in:
9	https://bugzilla.gnome.org/show_bug.cgi?id=739424
10
11	To prevent such crash, make sure the threads are holding ``hash_lock''
12	when manipulating such items.
13	---
14	gio/kqueue/kqueue-helper.c \| 6 ++++--
15	1 file changed, 4 insertions(+), 2 deletions(-)
16
17	diff --git a/gio/kqueue/kqueue-helper.c b/gio/kqueue/kqueue-helper.c
18	index d4e66cd4d..84b9ef164 100644
19	--- gio/kqueue/kqueue-helper.c
20	+++ gio/kqueue/kqueue-helper.c
21	@@ -291,10 +291,10 @@ process_kqueue_notifications (GIOChannel *gioc,
22
23	G_LOCK (hash_lock);
24	sub = (kqueue_sub *) g_hash_table_lookup (subs_hash_table, GINT_TO_POINTER (n.fd));
25	- G_UNLOCK (hash_lock);
26
27	if (sub == NULL)
28	{
29	+ G_UNLOCK (hash_lock);
30	KH_W ("Got a notification for a deleted or non-existing subscription %d",
31	n.fd);
32	return TRUE;
33	@@ -336,6 +336,7 @@ process_kqueue_notifications (GIOChannel *gioc,
34	g_file_monitor_source_handle_event (source, mask, NULL, NULL, NULL, g_get_monotonic_time ());
35	}
36
37	+ G_UNLOCK (hash_lock);
38	return TRUE;
39	}
40
41	@@ -451,13 +452,14 @@ _kh_start_watching (kqueue_sub *sub)
42
43	G_LOCK (hash_lock);
44	g_hash_table_insert (subs_hash_table, GINT_TO_POINTER (sub->fd), sub);
45	- G_UNLOCK (hash_lock);
46
47	_kqueue_thread_push_fd (sub->fd);
48
49	/* Bump the kqueue thread. It will pick up a new sub entry to monitor */
50	if (!_ku_write (kqueue_socket_pair[0], "A", 1))
51	KH_W ("Failed to bump the kqueue thread (add fd, error %d)", errno);
52	+ G_UNLOCK (hash_lock);
53	+
54	return TRUE;
55	}
56
57	--
58	2.12.2
59

Lines 1-55 Link Here

(-)files/patch-bug778515 (-55 lines)
1	From e305fe971e4647d971428a772b7290b9c308a96f Mon Sep 17 00:00:00 2001
2	From: Steven McDonald <steven@steven-mcdonald.id.au>
3	Date: Sun, 12 Feb 2017 11:02:55 +1100
4	Subject: gio: Always purge kqueue subs from missing list
5
6	Previously, _kh_cancel_sub assumed that it only needed to call
7	_km_remove if sub did not exist in subs_hash_table. This is erroneous
8	because the complementary operation, _km_add_missing, can be called
9	from process_kqueue_notifications, in which context sub can only have
10	come from subs_hash_table.
11
12	Since _km_remove is implemented using g_slist_remove, which is
13	documented to be a noop if the list does not contain the element to be
14	removed, it is safe to call _km_remove unconditionally here.
15
16	https://bugzilla.gnome.org/show_bug.cgi?id=778515
17	---
18	gio/kqueue/kqueue-helper.c \| 15 +++++----------
19	1 file changed, 5 insertions(+), 10 deletions(-)
20
21	diff --git a/gio/kqueue/kqueue-helper.c b/gio/kqueue/kqueue-helper.c
22	index 4671396..d4e66cd 100644
23	--- gio/kqueue/kqueue-helper.c
24	+++ gio/kqueue/kqueue-helper.c
25	@@ -498,22 +498,17 @@ _kh_add_sub (kqueue_sub *sub)
26	gboolean
27	_kh_cancel_sub (kqueue_sub *sub)
28	{
29	- gboolean missing = FALSE;
30	+ gboolean removed = FALSE;
31	g_assert (kqueue_socket_pair[0] != -1);
32	g_assert (sub != NULL);
33
34	+ _km_remove (sub);
35	+
36	G_LOCK (hash_lock);
37	- missing = !g_hash_table_remove (subs_hash_table, GINT_TO_POINTER (sub->fd));
38	+ removed = g_hash_table_remove (subs_hash_table, GINT_TO_POINTER (sub->fd));
39	G_UNLOCK (hash_lock);
40
41	- if (missing)
42	- {
43	- /* If there were no fd for this subscription, file is still
44	- * missing. */
45	- KH_W ("Removing subscription from missing");
46	- _km_remove (sub);
47	- }
48	- else
49	+ if (removed)
50	{
51	/* fd will be closed in the kqueue thread */
52	_kqueue_thread_remove_fd (sub->fd);
53	--
54	cgit v0.12
55

Line 0 Link Here

(-)files/patch-gio_kqueue_kqueue-helper.c (+16 lines)
	1	diff --git a/gio/kqueue/kqueue-helper.c b/gio/kqueue/kqueue-helper.c
	2	index d4e66cd4d..e7d583c8b 100644
	3	--- gio/kqueue/kqueue-helper.c
	4	+++ gio/kqueue/kqueue-helper.c
	5	@@ -97,8 +97,10 @@ convert_kqueue_events_to_gio (uint32_t flags, gboolean *done)
	6	}
	7	if (flags & NOTE_RENAME)
	8	{
	9	+ /* Since there’s apparently no way to get the new name of the file out of
	10	+ * kqueue(), all we can do is say that this one has been deleted. */
	11	*done = TRUE;
	12	- return G_FILE_MONITOR_EVENT_MOVED;
	13	+ return G_FILE_MONITOR_EVENT_DELETED;
	14	}
	15	if (flags & NOTE_REVOKE)
	16	{