Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

glib Registry backend patches #1527

Merged
merged 1 commit into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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