Skip to content

Commit

Permalink
glib: add patches fixing crashes in the registry backend
Browse files Browse the repository at this point in the history
  • Loading branch information
nacho committed Feb 5, 2025
1 parent 1c8e2f0 commit c47b995
Show file tree
Hide file tree
Showing 5 changed files with 434 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
From 431b9374d8bc85d532c66e08b3505dc155a36fc3 Mon Sep 17 00:00:00 2001
From: Silvio Lazzeretti <[email protected]>
Date: Thu, 30 Jan 2025 17:34:30 +0100
Subject: [PATCH 1/2] registrybackend: make subscribe and unsubscribe
thread-safe

The registry backend uses a thread to monitor
registry changes and send notifications.
The state of this thread and structures used
for communicating with it are kept in the watch
variable.
The subscribe and unsubscribe functions might be
concurrently called from multiple threads and
need to communicate with the monitoring thread.
For this reason we need to synchronize the access
to the watch variable.
---
gio/gregistrysettingsbackend.c | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/gio/gregistrysettingsbackend.c b/gio/gregistrysettingsbackend.c
index 3ed384fbc..8c99da8e1 100644
--- a/gio/gregistrysettingsbackend.c
+++ b/gio/gregistrysettingsbackend.c
@@ -179,6 +179,10 @@ typedef struct {
CRITICAL_SECTION *cache_lock;
GNode *cache_root;

+ /* A lock to protect access to the watch variable */
+ CRITICAL_SECTION watch_lock;
+ /* Contains the state of the watching thread. Any access to this variable
+ * must be done while holding the watch_lock critical section. */
WatchThreadState *watch;
} GRegistrySettingsBackend;

@@ -1901,6 +1905,7 @@ watch_thread_function (LPVOID parameter)
return -1;
}

+/* This function assumes you hold the watch lock! */
static gboolean
watch_start (GRegistrySettingsBackend *self)
{
@@ -1947,6 +1952,7 @@ fail:
return FALSE;
}

+/* This function assumes you hold the watch lock! */
/* This function assumes you hold the message lock! */
static void
watch_stop_unlocked (GRegistrySettingsBackend *self)
@@ -1982,6 +1988,7 @@ watch_stop_unlocked (GRegistrySettingsBackend *self)
self->watch = NULL;
}

+/* This function assumes you hold the watch lock! */
static gboolean
watch_add_notify (GRegistrySettingsBackend *self,
HANDLE event,
@@ -2055,6 +2062,7 @@ watch_add_notify (GRegistrySettingsBackend *self,
return TRUE;
}

+/* This function assumes you hold the watch lock! */
static void
watch_remove_notify (GRegistrySettingsBackend *self,
const gchar *key_name)
@@ -2105,12 +2113,17 @@ g_registry_settings_backend_subscribe (GSettingsBackend *backend,
HANDLE event;
LONG result;

+ EnterCriticalSection (&self->watch_lock);
if (self->watch == NULL && !watch_start (self))
- return;
+ {
+ LeaveCriticalSection (&self->watch_lock);
+ return;
+ }

if (g_atomic_int_dec_and_test (&self->watch->watches_remaining))
{
g_atomic_int_inc (&self->watch->watches_remaining);
+ LeaveCriticalSection (&self->watch_lock);
g_warning ("subscribe() failed: only %i different paths may be watched.", MAX_WATCHES);
return;
}
@@ -2139,6 +2152,7 @@ g_registry_settings_backend_subscribe (GSettingsBackend *backend,
{
g_message_win32_error (result, "gregistrysettingsbackend: Unable to subscribe to key %s.", key_name);
g_atomic_int_inc (&self->watch->watches_remaining);
+ LeaveCriticalSection (&self->watch_lock);
return;
}

@@ -2147,6 +2161,7 @@ g_registry_settings_backend_subscribe (GSettingsBackend *backend,
{
g_message_win32_error (result, "gregistrysettingsbackend: CreateEvent failed.");
g_atomic_int_inc (&self->watch->watches_remaining);
+ LeaveCriticalSection (&self->watch_lock);
RegCloseKey (hpath);
return;
}
@@ -2159,15 +2174,21 @@ g_registry_settings_backend_subscribe (GSettingsBackend *backend,
RegCloseKey (hpath);
CloseHandle (event);
}
+
+ LeaveCriticalSection (&self->watch_lock);
}

static void
g_registry_settings_backend_unsubscribe (GSettingsBackend *backend,
const char *key_name)
{
+ GRegistrySettingsBackend *self = G_REGISTRY_SETTINGS_BACKEND (backend);
+
trace ("unsubscribe: %s.\n", key_name);

- watch_remove_notify (G_REGISTRY_SETTINGS_BACKEND (backend), key_name);
+ EnterCriticalSection (&self->watch_lock);
+ watch_remove_notify (self, key_name);
+ LeaveCriticalSection (&self->watch_lock);
}

/********************************************************************************
@@ -2191,6 +2212,7 @@ g_registry_settings_backend_finalize (GObject *object)
EnterCriticalSection (self->watch->message_lock);
watch_stop_unlocked (self);
}
+ DeleteCriticalSection (&self->watch_lock);

DeleteCriticalSection (self->cache_lock);
g_slice_free (CRITICAL_SECTION, self->cache_lock);
@@ -2341,6 +2363,7 @@ g_registry_settings_backend_init (GRegistrySettingsBackend *self)
InitializeCriticalSection (self->cache_lock);

self->watch = NULL;
+ InitializeCriticalSection (&self->watch_lock);
}

/**
--
2.47.1.windows.2

Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
From 4c836565a883ebf0f58986b3f4f5e26f1f118463 Mon Sep 17 00:00:00 2001
From: Silvio Lazzeretti <[email protected]>
Date: Thu, 30 Jan 2025 17:34:39 +0100
Subject: [PATCH 2/2] registrybackend: fix some memory leaks

---
gio/gregistrysettingsbackend.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/gio/gregistrysettingsbackend.c b/gio/gregistrysettingsbackend.c
index 8c99da8e1..aa461e1e8 100644
--- a/gio/gregistrysettingsbackend.c
+++ b/gio/gregistrysettingsbackend.c
@@ -1738,6 +1738,7 @@ watch_thread_handle_message (WatchThreadState *self)

trace ("watch thread: unsubscribe: freeing node %p, prefix %s, index %i\n",
cache_node, self->message.watch.prefix, i);
+ g_free (self->message.watch.prefix);

if (cache_node != NULL)
{
@@ -1752,7 +1753,6 @@ watch_thread_handle_message (WatchThreadState *self)
}

_free_watch (self, i, cache_node);
- g_free (self->message.watch.prefix);

g_atomic_int_inc (&self->watches_remaining);
break;
@@ -1766,6 +1766,11 @@ watch_thread_handle_message (WatchThreadState *self)
for (i = 1; i < self->events->len; i++)
_free_watch (self, i, g_ptr_array_index (self->cache_nodes, i));

+ g_ptr_array_unref (self->events);
+ g_ptr_array_unref (self->handles);
+ g_ptr_array_unref (self->prefixes);
+ g_ptr_array_unref (self->cache_nodes);
+
SetEvent (self->message_received_event);
ExitThread (0);
}
@@ -1993,7 +1998,7 @@ static gboolean
watch_add_notify (GRegistrySettingsBackend *self,
HANDLE event,
HKEY hpath,
- gchar *gsettings_prefix)
+ const gchar *gsettings_prefix)
{
WatchThreadState *watch = self->watch;
GNode *cache_node;
@@ -2039,7 +2044,7 @@ watch_add_notify (GRegistrySettingsBackend *self,
watch->message.type = WATCH_THREAD_ADD_WATCH;
watch->message.watch.event = event;
watch->message.watch.hpath = hpath;
- watch->message.watch.prefix = gsettings_prefix;
+ watch->message.watch.prefix = g_strdup (gsettings_prefix);
watch->message.watch.cache_node = cache_node;

SetEvent (watch->message_sent_event);
@@ -2168,7 +2173,7 @@ g_registry_settings_backend_subscribe (GSettingsBackend *backend,

/* The actual watch is added by the thread, which has to re-subscribe each time it
* receives a change. */
- if (!watch_add_notify (self, event, hpath, g_strdup (key_name)))
+ if (!watch_add_notify (self, event, hpath, key_name))
{
g_atomic_int_inc (&self->watch->watches_remaining);
RegCloseKey (hpath);
--
2.47.1.windows.2

Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
From 431b9374d8bc85d532c66e08b3505dc155a36fc3 Mon Sep 17 00:00:00 2001
From: Silvio Lazzeretti <[email protected]>
Date: Thu, 30 Jan 2025 17:34:30 +0100
Subject: [PATCH 1/2] registrybackend: make subscribe and unsubscribe
thread-safe

The registry backend uses a thread to monitor
registry changes and send notifications.
The state of this thread and structures used
for communicating with it are kept in the watch
variable.
The subscribe and unsubscribe functions might be
concurrently called from multiple threads and
need to communicate with the monitoring thread.
For this reason we need to synchronize the access
to the watch variable.
---
gio/gregistrysettingsbackend.c | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/gio/gregistrysettingsbackend.c b/gio/gregistrysettingsbackend.c
index 3ed384fbc..8c99da8e1 100644
--- a/gio/gregistrysettingsbackend.c
+++ b/gio/gregistrysettingsbackend.c
@@ -179,6 +179,10 @@ typedef struct {
CRITICAL_SECTION *cache_lock;
GNode *cache_root;

+ /* A lock to protect access to the watch variable */
+ CRITICAL_SECTION watch_lock;
+ /* Contains the state of the watching thread. Any access to this variable
+ * must be done while holding the watch_lock critical section. */
WatchThreadState *watch;
} GRegistrySettingsBackend;

@@ -1901,6 +1905,7 @@ watch_thread_function (LPVOID parameter)
return -1;
}

+/* This function assumes you hold the watch lock! */
static gboolean
watch_start (GRegistrySettingsBackend *self)
{
@@ -1947,6 +1952,7 @@ fail:
return FALSE;
}

+/* This function assumes you hold the watch lock! */
/* This function assumes you hold the message lock! */
static void
watch_stop_unlocked (GRegistrySettingsBackend *self)
@@ -1982,6 +1988,7 @@ watch_stop_unlocked (GRegistrySettingsBackend *self)
self->watch = NULL;
}

+/* This function assumes you hold the watch lock! */
static gboolean
watch_add_notify (GRegistrySettingsBackend *self,
HANDLE event,
@@ -2055,6 +2062,7 @@ watch_add_notify (GRegistrySettingsBackend *self,
return TRUE;
}

+/* This function assumes you hold the watch lock! */
static void
watch_remove_notify (GRegistrySettingsBackend *self,
const gchar *key_name)
@@ -2105,12 +2113,17 @@ g_registry_settings_backend_subscribe (GSettingsBackend *backend,
HANDLE event;
LONG result;

+ EnterCriticalSection (&self->watch_lock);
if (self->watch == NULL && !watch_start (self))
- return;
+ {
+ LeaveCriticalSection (&self->watch_lock);
+ return;
+ }

if (g_atomic_int_dec_and_test (&self->watch->watches_remaining))
{
g_atomic_int_inc (&self->watch->watches_remaining);
+ LeaveCriticalSection (&self->watch_lock);
g_warning ("subscribe() failed: only %i different paths may be watched.", MAX_WATCHES);
return;
}
@@ -2139,6 +2152,7 @@ g_registry_settings_backend_subscribe (GSettingsBackend *backend,
{
g_message_win32_error (result, "gregistrysettingsbackend: Unable to subscribe to key %s.", key_name);
g_atomic_int_inc (&self->watch->watches_remaining);
+ LeaveCriticalSection (&self->watch_lock);
return;
}

@@ -2147,6 +2161,7 @@ g_registry_settings_backend_subscribe (GSettingsBackend *backend,
{
g_message_win32_error (result, "gregistrysettingsbackend: CreateEvent failed.");
g_atomic_int_inc (&self->watch->watches_remaining);
+ LeaveCriticalSection (&self->watch_lock);
RegCloseKey (hpath);
return;
}
@@ -2159,15 +2174,21 @@ g_registry_settings_backend_subscribe (GSettingsBackend *backend,
RegCloseKey (hpath);
CloseHandle (event);
}
+
+ LeaveCriticalSection (&self->watch_lock);
}

static void
g_registry_settings_backend_unsubscribe (GSettingsBackend *backend,
const char *key_name)
{
+ GRegistrySettingsBackend *self = G_REGISTRY_SETTINGS_BACKEND (backend);
+
trace ("unsubscribe: %s.\n", key_name);

- watch_remove_notify (G_REGISTRY_SETTINGS_BACKEND (backend), key_name);
+ EnterCriticalSection (&self->watch_lock);
+ watch_remove_notify (self, key_name);
+ LeaveCriticalSection (&self->watch_lock);
}

/********************************************************************************
@@ -2191,6 +2212,7 @@ g_registry_settings_backend_finalize (GObject *object)
EnterCriticalSection (self->watch->message_lock);
watch_stop_unlocked (self);
}
+ DeleteCriticalSection (&self->watch_lock);

DeleteCriticalSection (self->cache_lock);
g_slice_free (CRITICAL_SECTION, self->cache_lock);
@@ -2341,6 +2363,7 @@ g_registry_settings_backend_init (GRegistrySettingsBackend *self)
InitializeCriticalSection (self->cache_lock);

self->watch = NULL;
+ InitializeCriticalSection (&self->watch_lock);
}

/**
--
2.47.1.windows.2

Loading

0 comments on commit c47b995

Please sign in to comment.