packages/glib2
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

!155 [sync] PR-153: fix double unref and fix group comment management

Browse Source 
From: @openeuler-sync-bot 
Reviewed-by: @yanan-rock 
Signed-off-by: @yanan-rock
This commit is contained in:
openeuler-ci-bot 2023-08-21 02:41:58 +00:00 committed by Gitee
commit 4521ef2c0b
No known key found for this signature in database
GPG Key ID: 173E9B9CA92EEF8F
6 changed files with 1808 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

227
backport-add-g_free_sized-and-g_aligned_free_sized.patch Normal file
View File

@ -0,0 +1,227 @@
From 329843f682d1216d4f41aab7b5711f21ef280b71 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 25 Jan 2023 14:12:20 +0000
Subject: [PATCH] gmem: Add g_free_sized() and g_aligned_free_sized()
These wrap `free_sized()` and `free_aligned_sized()`, which are present
in C23[1]. This means that user code can start to use them without checking
for C23 support everywhere first.
It also means we can use them internally in GSlice to get a bit of
performance for the code which still uses it.
See https://en.cppreference.com/w/c/memory/free_aligned_sized and
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2699.htm.
[1]: Specifically, section 7.24.3.4 of the latest C23 draft at
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3088.pdf.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Conflict:NA
Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/329843f682d1216d4f41aab7b5711f21ef280b71
diff --git a/docs/reference/glib/glib-sections.txt b/docs/reference/glib/glib-sections.txt
index 3532d28..ac93ae6 100644
--- a/docs/reference/glib/glib-sections.txt
+++ b/docs/reference/glib/glib-sections.txt
@@ -1397,6 +1397,7 @@ g_try_realloc_n
<SUBSECTION>
g_free
+g_free_sized
g_clear_pointer
g_steal_pointer
g_mem_gc_friendly
@@ -1411,6 +1412,7 @@ g_newa0
g_aligned_alloc
g_aligned_alloc0
g_aligned_free
+g_aligned_free_sized
<SUBSECTION>
g_memmove
diff --git a/glib/gmem.c b/glib/gmem.c
index 060e91a..e94268a 100644
--- a/glib/gmem.c
+++ b/glib/gmem.c
@@ -209,6 +209,9 @@ g_realloc (gpointer mem,
*
* Frees the memory pointed to by @mem.
*
+ * If you know the allocated size of @mem, calling g_free_sized() may be faster,
+ * depending on the libc implementation in use.
+ *
* If @mem is %NULL it simply returns, so there is no need to check @mem
* against %NULL before calling this function.
*/
@@ -219,6 +222,33 @@ g_free (gpointer mem)
TRACE(GLIB_MEM_FREE((void*) mem));
}
+/**
+ * g_free_sized:
+ * @mem: (nullable): the memory to free
+ * @size: size of @mem, in bytes
+ *
+ * Frees the memory pointed to by @mem, assuming it is has the given @size.
+ *
+ * If @mem is %NULL this is a no-op (and @size is ignored).
+ *
+ * It is an error if @size doesn鈥檛 match the size passed when @mem was
+ * allocated. @size is passed to this function to allow optimizations in the
+ * allocator. If you don鈥檛 know the allocation size, use g_free() instead.
+ *
+ * Since: 2.72
+ */
+void
+g_free_sized (void *mem,
+ size_t size)
+{
+#ifdef HAVE_FREE_SIZED
+ free_sized (mem, size);
+#else
+ free (mem);
+#endif
+ TRACE (GLIB_MEM_FREE ((void*) mem));
+}
+
/**
* g_clear_pointer: (skip)
* @pp: (not nullable): a pointer to a variable, struct member etc. holding a
@@ -555,7 +585,7 @@ g_mem_profile (void)
* multiplication.
*
* Aligned memory allocations returned by this function can only be
- * freed using g_aligned_free().
+ * freed using g_aligned_free_sized() or g_aligned_free().
*
* Returns: (transfer full): the allocated memory
*
@@ -679,3 +709,33 @@ g_aligned_free (gpointer mem)
{
aligned_free (mem);
}
+
+/**
+ * g_aligned_free_sized:
+ * @mem: (nullable): the memory to free
+ * @alignment: alignment of @mem
+ * @size: size of @mem, in bytes
+ *
+ * Frees the memory pointed to by @mem, assuming it is has the given @size and
+ * @alignment.
+ *
+ * If @mem is %NULL this is a no-op (and @size is ignored).
+ *
+ * It is an error if @size doesn鈥檛 match the size, or @alignment doesn鈥檛 match
+ * the alignment, passed when @mem was allocated. @size and @alignment are
+ * passed to this function to allow optimizations in the allocator. If you
+ * don鈥檛 know either of them, use g_aligned_free() instead.
+ *
+ * Since: 2.72
+ */
+void
+g_aligned_free_sized (void *mem,
+ size_t alignment,
+ size_t size)
+{
+#ifdef HAVE_FREE_ALIGNED_SIZED
+ free_aligned_sized (mem, alignment, size);
+#else
+ aligned_free (mem);
+#endif
+}
diff --git a/glib/gmem.h b/glib/gmem.h
index d29907a..7b306b3 100644
--- a/glib/gmem.h
+++ b/glib/gmem.h
@@ -70,6 +70,9 @@ typedef struct _GMemVTable GMemVTable;
GLIB_AVAILABLE_IN_ALL
void g_free (gpointer mem);
+GLIB_AVAILABLE_IN_2_72
+void g_free_sized (gpointer mem,
+ size_t size);
GLIB_AVAILABLE_IN_2_34
void g_clear_pointer (gpointer *pp,
@@ -121,6 +124,10 @@ gpointer g_aligned_alloc0 (gsize n_blocks,
gsize alignment) G_GNUC_WARN_UNUSED_RESULT G_GNUC_ALLOC_SIZE2(1,2);
GLIB_AVAILABLE_IN_2_72
void g_aligned_free (gpointer mem);
+GLIB_AVAILABLE_IN_2_72
+void g_aligned_free_sized (gpointer mem,
+ size_t alignment,
+ size_t size);
#if defined(glib_typeof) && GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_58
#define g_clear_pointer(pp, destroy) \
diff --git a/glib/tests/utils.c b/glib/tests/utils.c
index dcdc5a6..602abe1 100644
--- a/glib/tests/utils.c
+++ b/glib/tests/utils.c
@@ -1000,6 +1000,39 @@ test_aligned_mem_zeroed (void)
g_aligned_free (p);
}
+static void
+test_aligned_mem_free_sized (void)
+{
+ gsize n_blocks = 10;
+ guint *p;
+
+ g_test_summary ("Check that g_aligned_free_sized() works");
+
+ p = g_aligned_alloc (n_blocks, sizeof (*p), 16);
+ g_assert_nonnull (p);
+
+ g_aligned_free_sized (p, sizeof (*p), n_blocks * 16);
+
+ /* NULL should be ignored */
+ g_aligned_free_sized (NULL, sizeof (*p), n_blocks * 16);
+}
+
+static void
+test_free_sized (void)
+{
+ gpointer p;
+
+ g_test_summary ("Check that g_free_sized() works");
+
+ p = g_malloc (123);
+ g_assert_nonnull (p);
+
+ g_free_sized (p, 123);
+
+ /* NULL should be ignored */
+ g_free_sized (NULL, 123);
+}
+
static void
test_nullify (void)
{
@@ -1174,6 +1207,8 @@ main (int argc,
g_test_add_func ("/utils/aligned-mem/subprocess/aligned_alloc_nmov", aligned_alloc_nmov);
g_test_add_func ("/utils/aligned-mem/alignment", test_aligned_mem_alignment);
g_test_add_func ("/utils/aligned-mem/zeroed", test_aligned_mem_zeroed);
+ g_test_add_func ("/utils/aligned-mem/free-sized", test_aligned_mem_free_sized);
+ g_test_add_func ("/utils/free-sized", test_free_sized);
g_test_add_func ("/utils/nullify", test_nullify);
g_test_add_func ("/utils/atexit", test_atexit);
g_test_add_func ("/utils/check-setuid", test_check_setuid);
diff --git a/meson.build b/meson.build
index 657e9f6..3f32ef7 100644
--- a/meson.build
+++ b/meson.build
@@ -535,6 +535,8 @@ functions = [
'fchmod',
'fchown',
'fdwalk',
+ 'free_aligned_sized',
+ 'free_sized',
'fsync',
'getauxval',
'getc_unlocked',
--
2.33.0

877
backport-gdbusconnection-Fix-double-unref-on-timeout-cancel-sending-a-message.patch Normal file
View File

@ -0,0 +1,877 @@
From 4900ea5215e329fbfe893be7975cf05ff153ef81 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 22 Feb 2023 02:40:35 +0000
Subject: [PATCH 1/9] gdbusconnection: Fix double unref on timeout/cancel
sending a message
This appears to fix an intermittent failure seen when sending a D-Bus
message with either of a cancellable or a timeout set.
In particular, I can reliably reproduce it with:
```
meson test gdbus-test-codegen-min-required-2-64 --repeat 10000
```
It can be caught easily with asan when reproduced. Tracking down the
location of the refcount mismatch was a little tricky, but was
simplified by replacing a load of `g_object_ref (message)` calls with
`g_dbus_message_copy (message, NULL)` to switch `GDBusMessage` handling
to using copy semantics. This allowed asan to home in on where the
refcount mismatch was happening.
The problem was that `send_message_data_deliver_error()` takes ownership
of the `GTask` passed to it, but the
`send_message_with_replace_cancelled_idle_cb()` and
`send_message_with_reply_timeout_cb()` functions which were calling it,
were not passing in a strong reference as they should have.
Another approach to fixing this would have been to change the transfer
semantics of `send_message_data_deliver_error()` so it was `(transfer
none)` on its `GTask`. That would probably have resulted in cleaner
code, but would have been a lot harder to verify/review the fix, and
easier to inadvertently introduce new bugs.
The fact that the bug was only triggered by the cancellation and timeout
callbacks explains why it was intermittent: these code paths are
typically never hit, but the timeout path may sometimes be hit on a very
slow test run.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #1264
Conflict:NA
Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/4900ea5215e329fbfe893be7975cf05ff153ef81
---
gio/gdbusconnection.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index d938f71b99..06c8a6141f 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -1822,7 +1822,7 @@ send_message_data_deliver_reply_unlocked (GTask *task,
;
}
-/* Called from a user thread, lock is not held */
+/* Called from a user thread, lock is not held; @task is (transfer full) */
static void
send_message_data_deliver_error (GTask *task,
GQuark domain,
@@ -1849,13 +1849,14 @@ send_message_data_deliver_error (GTask *task,
/* ---------------------------------------------------------------------------------------------------- */
-/* Called from a user thread, lock is not held; @task is (transfer full) */
+/* Called from a user thread, lock is not held; @task is (transfer none) */
static gboolean
send_message_with_reply_cancelled_idle_cb (gpointer user_data)
{
GTask *task = user_data;
- send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_CANCELLED,
+ g_object_ref (task);
+ send_message_data_deliver_error (g_steal_pointer (&task), G_IO_ERROR, G_IO_ERROR_CANCELLED,
_("Operation was cancelled"));
return FALSE;
}
@@ -1879,13 +1880,14 @@ send_message_with_reply_cancelled_cb (GCancellable *cancellable,
/* ---------------------------------------------------------------------------------------------------- */
-/* Called from a user thread, lock is not held; @task is (transfer full) */
+/* Called from a user thread, lock is not held; @task is (transfer none) */
static gboolean
send_message_with_reply_timeout_cb (gpointer user_data)
{
GTask *task = user_data;
- send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_TIMED_OUT,
+ g_object_ref (task);
+ send_message_data_deliver_error (g_steal_pointer (&task), G_IO_ERROR, G_IO_ERROR_TIMED_OUT,
_("Timeout was reached"));
return FALSE;
}
--
GitLab
From 127c899a2e727d10eb88b8fae196add11a6c053f Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 22 Feb 2023 02:45:15 +0000
Subject: [PATCH 2/9] gdbusconnection: Fix the type of a free function
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This didn’t actually cause any observable bugs, since the structures of
`PropertyData` and `PropertyGetAllData` were equivalent for the members
which the free function touches.
Definitely should be fixed though.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Conflict:NA
Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/127c899a2e727d10eb88b8fae196add11a6c053f
---
gio/gdbusconnection.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 06c8a6141f..6a0d67a8ee 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -4584,7 +4584,7 @@ typedef struct
} PropertyGetAllData;
static void
-property_get_all_data_free (PropertyData *data)
+property_get_all_data_free (PropertyGetAllData *data)
{
g_object_unref (data->connection);
g_object_unref (data->message);
--
GitLab
From 90af20d9505a11d02e81a4f8fa09ee15faba96b8 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 22 Feb 2023 02:46:55 +0000
Subject: [PATCH 3/9] gdbusconnection: Improve docs of message ownership in
closures
This introduces no functional changes, but makes it a little clearer how
the ownership of these `GDBusMessage` instances works. The free function
is changed to `g_clear_object()` to avoid the possibility of somehow
using the messages after freeing them.
Basically just some drive-by docs improvements while trying to debug
issue #1264.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1264
Conflict:NA
Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/90af20d9505a11d02e81a4f8fa09ee15faba96b8
---
gio/gdbusconnection.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 6a0d67a8ee..0cbfc66c13 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -3743,7 +3743,7 @@ g_dbus_connection_signal_unsubscribe (GDBusConnection *connection,
typedef struct
{
SignalSubscriber *subscriber; /* (owned) */
- GDBusMessage *message;
+ GDBusMessage *message; /* (owned) */
GDBusConnection *connection;
const gchar *sender; /* (nullable) for peer-to-peer connections */
const gchar *path;
@@ -3807,7 +3807,7 @@ emit_signal_instance_in_idle_cb (gpointer data)
static void
signal_instance_free (SignalInstance *signal_instance)
{
- g_object_unref (signal_instance->message);
+ g_clear_object (&signal_instance->message);
g_object_unref (signal_instance->connection);
signal_subscriber_unref (signal_instance->subscriber);
g_free (signal_instance);
@@ -4219,7 +4219,7 @@ has_object_been_unregistered (GDBusConnection *connection,
typedef struct
{
GDBusConnection *connection;
- GDBusMessage *message;
+ GDBusMessage *message; /* (owned) */
gpointer user_data;
const gchar *property_name;
const GDBusInterfaceVTable *vtable;
@@ -4233,7 +4233,7 @@ static void
property_data_free (PropertyData *data)
{
g_object_unref (data->connection);
- g_object_unref (data->message);
+ g_clear_object (&data->message);
g_free (data);
}
@@ -4575,7 +4575,7 @@ handle_getset_property (GDBusConnection *connection,
typedef struct
{
GDBusConnection *connection;
- GDBusMessage *message;
+ GDBusMessage *message; /* (owned) */
gpointer user_data;
const GDBusInterfaceVTable *vtable;
GDBusInterfaceInfo *interface_info;
@@ -4587,7 +4587,7 @@ static void
property_get_all_data_free (PropertyGetAllData *data)
{
g_object_unref (data->connection);
- g_object_unref (data->message);
+ g_clear_object (&data->message);
g_free (data);
}
@@ -6815,7 +6815,7 @@ typedef struct
static void
subtree_deferred_data_free (SubtreeDeferredData *data)
{
- g_object_unref (data->message);
+ g_clear_object (&data->message);
exported_subtree_unref (data->es);
g_free (data);
}
--
GitLab
From ed7044b5f383cf8b77df751578e184d4ad7a134f Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 22 Feb 2023 02:49:29 +0000
Subject: [PATCH 4/9] gdbusprivate: Improve docs on message ownership in
MessageToWriteData
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This doesn’t introduce any functional changes, but should make the code
a little clearer.
Drive-by improvements while trying to debug #1264.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1264
Conflict:NA
Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/ed7044b5f383cf8b77df751578e184d4ad7a134f
---
gio/gdbusprivate.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c
index 762afcee46..bd776a4214 100644
--- a/gio/gdbusprivate.c
+++ b/gio/gdbusprivate.c
@@ -889,7 +889,7 @@ _g_dbus_worker_do_initial_read (gpointer data)
struct _MessageToWriteData
{
GDBusWorker *worker;
- GDBusMessage *message;
+ GDBusMessage *message; /* (owned) */
gchar *blob;
gsize blob_size;
@@ -901,8 +901,7 @@ static void
message_to_write_data_free (MessageToWriteData *data)
{
_g_dbus_worker_unref (data->worker);
- if (data->message)
- g_object_unref (data->message);
+ g_clear_object (&data->message);
g_free (data->blob);
g_slice_free (MessageToWriteData, data);
}
--
GitLab
From 861741ef4bff1a3ee15175e189136563b74fe790 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 22 Feb 2023 02:50:47 +0000
Subject: [PATCH 5/9] gdbusprivate: Ensure data->task is cleared when it
returns
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The existing comment in the code was correct that `data` is freed when
the task callback is called, because `data` is also pointed to by the
`user_data` for the task, and that’s freed at the end of the callback.
So the existing code was correct to take a copy of `data->task` before
calling `g_task_return_*()`.
After calling `g_task_return_*()`, the existing code unreffed the task
(which is correct), but then didn’t clear the `data->task` pointer,
leaving `data->task` dangling. That could cause a use-after-free or a
double-unref.
Avoid that risk by explicitly clearing `data->task` before calling
`g_task_return_*()`.
After some testing, it turns out this doesn’t actually fix any bugs, but
it’s still a good robustness improvement.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1264
Conflict:NA
Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/861741ef4bff1a3ee15175e189136563b74fe790
---
gio/gdbusprivate.c | 54 ++++++++++++++++++++++++++++------------------
1 file changed, 33 insertions(+), 21 deletions(-)
diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c
index bd776a4214..0b4806f524 100644
--- a/gio/gdbusprivate.c
+++ b/gio/gdbusprivate.c
@@ -894,7 +894,7 @@ struct _MessageToWriteData
gsize blob_size;
gsize total_written;
- GTask *task;
+ GTask *task; /* (owned) and (nullable) before writing starts and after g_task_return_*() is called */
};
static void
@@ -903,6 +903,11 @@ message_to_write_data_free (MessageToWriteData *data)
_g_dbus_worker_unref (data->worker);
g_clear_object (&data->message);
g_free (data->blob);
+
+ /* The task must either not have been created, or have been created, returned
+ * and finalised by now. */
+ g_assert (data->task == NULL);
+
g_slice_free (MessageToWriteData, data);
}
@@ -921,14 +926,14 @@ write_message_async_cb (GObject *source_object,
gpointer user_data)
{
MessageToWriteData *data = user_data;
- GTask *task;
gssize bytes_written;
GError *error;
- /* Note: we can't access data->task after calling g_task_return_* () because the
- * callback can free @data and we're not completing in idle. So use a copy of the pointer.
- */
- task = data->task;
+ /* The ownership of @data is a bit odd in this function: it’s (transfer full)
+ * when the function is called, but the code paths which call g_task_return_*()
+ * on @data->task will indirectly cause it to be freed, because @data is
+ * always guaranteed to be the user_data in the #GTask. So that’s why it looks
+ * like @data is not always freed on every code path in this function. */
error = NULL;
bytes_written = g_output_stream_write_finish (G_OUTPUT_STREAM (source_object),
@@ -936,8 +941,9 @@ write_message_async_cb (GObject *source_object,
&error);
if (bytes_written == -1)
{
+ GTask *task = g_steal_pointer (&data->task);
g_task_return_error (task, error);
- g_object_unref (task);
+ g_clear_object (&task);
goto out;
}
g_assert (bytes_written > 0); /* zero is never returned */
@@ -948,8 +954,9 @@ write_message_async_cb (GObject *source_object,
g_assert (data->total_written <= data->blob_size);
if (data->total_written == data->blob_size)
{
+ GTask *task = g_steal_pointer (&data->task);
g_task_return_boolean (task, TRUE);
- g_object_unref (task);
+ g_clear_object (&task);
goto out;
}
@@ -986,16 +993,14 @@ write_message_continue_writing (MessageToWriteData *data)
{
GOutputStream *ostream;
#ifdef G_OS_UNIX
- GTask *task;
GUnixFDList *fd_list;
#endif
-#ifdef G_OS_UNIX
- /* Note: we can't access data->task after calling g_task_return_* () because the
- * callback can free @data and we're not completing in idle. So use a copy of the pointer.
- */
- task = data->task;
-#endif
+ /* The ownership of @data is a bit odd in this function: it’s (transfer full)
+ * when the function is called, but the code paths which call g_task_return_*()
+ * on @data->task will indirectly cause it to be freed, because @data is
+ * always guaranteed to be the user_data in the #GTask. So that’s why it looks
+ * like @data is not always freed on every code path in this function. */
ostream = g_io_stream_get_output_stream (data->worker->stream);
#ifdef G_OS_UNIX
@@ -1024,11 +1029,12 @@ write_message_continue_writing (MessageToWriteData *data)
{
if (!(data->worker->capabilities & G_DBUS_CAPABILITY_FLAGS_UNIX_FD_PASSING))
{
+ GTask *task = g_steal_pointer (&data->task);
g_task_return_new_error (task,
G_IO_ERROR,
G_IO_ERROR_FAILED,
"Tried sending a file descriptor but remote peer does not support this capability");
- g_object_unref (task);
+ g_clear_object (&task);
goto out;
}
control_message = g_unix_fd_message_new_with_fd_list (fd_list);
@@ -1065,9 +1071,13 @@ write_message_continue_writing (MessageToWriteData *data)
g_error_free (error);
goto out;
}
- g_task_return_error (task, error);
- g_object_unref (task);
- goto out;
+ else
+ {
+ GTask *task = g_steal_pointer (&data->task);
+ g_task_return_error (task, error);
+ g_clear_object (&task);
+ goto out;
+ }
}
g_assert (bytes_written > 0); /* zero is never returned */
@@ -1077,8 +1087,9 @@ write_message_continue_writing (MessageToWriteData *data)
g_assert (data->total_written <= data->blob_size);
if (data->total_written == data->blob_size)
{
+ GTask *task = g_steal_pointer (&data->task);
g_task_return_boolean (task, TRUE);
- g_object_unref (task);
+ g_clear_object (&task);
goto out;
}
@@ -1093,12 +1104,13 @@ write_message_continue_writing (MessageToWriteData *data)
/* We were trying to write byte 0 of the message, which needs
* the fd list to be attached to it, but this connection doesn't
* support doing that. */
+ GTask *task = g_steal_pointer (&data->task);
g_task_return_new_error (task,
G_IO_ERROR,
G_IO_ERROR_FAILED,
"Tried sending a file descriptor on unsupported stream of type %s",
g_type_name (G_TYPE_FROM_INSTANCE (ostream)));
- g_object_unref (task);
+ g_clear_object (&task);
goto out;
}
#endif
--
GitLab
From d7c813cf5b6148c18184e4f1af23d234e73aafb8 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 22 Feb 2023 02:56:56 +0000
Subject: [PATCH 6/9] gdbusprivate: Improve ownership docs for
write_message_async()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The ownership transfers in this code are a bit complex, so adding some
extra documentation and `g_steal_pointer()` calls should hopefully help
clarify things.
This doesn’t introduce any functional changes, just code documentation.
Another drive-by improvement in the quest for #1264.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1264
Conflict:NA
Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/d7c813cf5b6148c18184e4f1af23d234e73aafb8
---
gio/gdbusprivate.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c
index 0b4806f524..5aa141a60e 100644
--- a/gio/gdbusprivate.c
+++ b/gio/gdbusprivate.c
@@ -919,13 +919,14 @@ static void write_message_continue_writing (MessageToWriteData *data);
*
* write-lock is not held on entry
* output_pending is PENDING_WRITE on entry
+ * @user_data is (transfer full)
*/
static void
write_message_async_cb (GObject *source_object,
GAsyncResult *res,
gpointer user_data)
{
- MessageToWriteData *data = user_data;
+ MessageToWriteData *data = g_steal_pointer (&user_data);
gssize bytes_written;
GError *error;
@@ -960,7 +961,7 @@ write_message_async_cb (GObject *source_object,
goto out;
}
- write_message_continue_writing (data);
+ write_message_continue_writing (g_steal_pointer (&data));
out:
;
@@ -977,8 +978,8 @@ on_socket_ready (GSocket *socket,
GIOCondition condition,
gpointer user_data)
{
- MessageToWriteData *data = user_data;
- write_message_continue_writing (data);
+ MessageToWriteData *data = g_steal_pointer (&user_data);
+ write_message_continue_writing (g_steal_pointer (&data));
return FALSE; /* remove source */
}
#endif
@@ -987,6 +988,7 @@ on_socket_ready (GSocket *socket,
*
* write-lock is not held on entry
* output_pending is PENDING_WRITE on entry
+ * @data is (transfer full)
*/
static void
write_message_continue_writing (MessageToWriteData *data)
@@ -1064,7 +1066,7 @@ write_message_continue_writing (MessageToWriteData *data)
data->worker->cancellable);
g_source_set_callback (source,
(GSourceFunc) on_socket_ready,
- data,
+ g_steal_pointer (&data),
NULL); /* GDestroyNotify */
g_source_attach (source, g_main_context_get_thread_default ());
g_source_unref (source);
@@ -1093,7 +1095,7 @@ write_message_continue_writing (MessageToWriteData *data)
goto out;
}
- write_message_continue_writing (data);
+ write_message_continue_writing (g_steal_pointer (&data));
}
#endif
else
@@ -1121,7 +1123,7 @@ write_message_continue_writing (MessageToWriteData *data)
G_PRIORITY_DEFAULT,
data->worker->cancellable,
write_message_async_cb,
- data);
+ data); /* steal @data */
}
#ifdef G_OS_UNIX
out:
@@ -1144,7 +1146,7 @@ write_message_async (GDBusWorker *worker,
g_task_set_source_tag (data->task, write_message_async);
g_task_set_name (data->task, "[gio] D-Bus write message");
data->total_written = 0;
- write_message_continue_writing (data);
+ write_message_continue_writing (g_steal_pointer (&data));
}
/* called in private thread shared by all GDBusConnection instances (with write-lock held) */
@@ -1333,6 +1335,7 @@ prepare_flush_unlocked (GDBusWorker *worker)
*
* write-lock is not held on entry
* output_pending is PENDING_WRITE on entry
+ * @user_data is (transfer full)
*/
static void
write_message_cb (GObject *source_object,
@@ -1551,7 +1554,7 @@ continue_writing (GDBusWorker *worker)
write_message_async (worker,
data,
write_message_cb,
- data);
+ data); /* takes ownership of @data as user_data */
}
}
--
GitLab
From 08a4387678346caaa42b69e5e6e5995d48cd61c4 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 22 Feb 2023 02:58:05 +0000
Subject: [PATCH 7/9] gdbusprivate: Use G_SOURCE_REMOVE in a source callback
This is equivalent to the current behaviour, but a little clearer in its
meaning.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1264
Conflict:NA
Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/08a4387678346caaa42b69e5e6e5995d48cd61c4
---
gio/gdbusprivate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c
index 5aa141a60e..2c9238c638 100644
--- a/gio/gdbusprivate.c
+++ b/gio/gdbusprivate.c
@@ -980,7 +980,7 @@ on_socket_ready (GSocket *socket,
{
MessageToWriteData *data = g_steal_pointer (&user_data);
write_message_continue_writing (g_steal_pointer (&data));
- return FALSE; /* remove source */
+ return G_SOURCE_REMOVE;
}
#endif
--
GitLab
From b84ec21f9c4c57990309e691206582c589f59770 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 22 Feb 2023 12:19:16 +0000
Subject: [PATCH 8/9] gdbusconnection: Rearrange refcount handling of
map_method_serial_to_task
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
It already implicitly held a strong ref on its `GTask` values, but
didn’t have a free function set so that they would be automatically
unreffed on removal from the map.
This meant that the functions handling removals from the map,
`on_worker_closed()` (via `cancel_method_on_close()`) and
`send_message_with_reply_cleanup()` had to call unref once more than
they would otherwise.
In `send_message_with_reply_cleanup()`, this behaviour depended on
whether it was called with `remove == TRUE`. If not, it was `(transfer
none)` not `(transfer full)`. This led to bugs in its callers.
For example, this led to a direct leak in `cancel_method_on_close()`, as
it needed to remove tasks from `map_method_serial_to_task`, but called
`send_message_with_reply_cleanup(remove = FALSE)` and erroneously didn’t
call unref an additional time.
Try and simplify it all by setting a `GDestroyNotify` on
`map_method_serial_to_task`’s values, and making the refcount handling
of `send_message_with_reply_cleanup()` not be conditional on its
arguments.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1264
Conflict:NA
Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/b84ec21f9c4c57990309e691206582c589f59770
---
gio/gdbusconnection.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 0cbfc66c13..f4bc21bb37 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -409,7 +409,7 @@ struct _GDBusConnection
GDBusConnectionFlags flags;
/* Map used for managing method replies, protected by @lock */
- GHashTable *map_method_serial_to_task; /* guint32 -> GTask* */
+ GHashTable *map_method_serial_to_task; /* guint32 -> owned GTask* */
/* Maps used for managing signal subscription, protected by @lock */
GHashTable *map_rule_to_signal_data; /* match rule (gchar*) -> SignalData */
@@ -1061,7 +1061,7 @@ g_dbus_connection_init (GDBusConnection *connection)
g_mutex_init (&connection->lock);
g_mutex_init (&connection->init_lock);
- connection->map_method_serial_to_task = g_hash_table_new (g_direct_hash, g_direct_equal);
+ connection->map_method_serial_to_task = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_object_unref);
connection->map_rule_to_signal_data = g_hash_table_new (g_str_hash,
g_str_equal);
@@ -1768,7 +1768,7 @@ send_message_data_free (SendMessageData *data)
/* ---------------------------------------------------------------------------------------------------- */
-/* can be called from any thread with lock held; @task is (transfer full) */
+/* can be called from any thread with lock held; @task is (transfer none) */
static void
send_message_with_reply_cleanup (GTask *task, gboolean remove)
{
@@ -1798,13 +1798,11 @@ send_message_with_reply_cleanup (GTask *task, gboolean remove)
GUINT_TO_POINTER (data->serial));
g_warn_if_fail (removed);
}
-
- g_object_unref (task);
}
/* ---------------------------------------------------------------------------------------------------- */
-/* Called from GDBus worker thread with lock held; @task is (transfer full). */
+/* Called from GDBus worker thread with lock held; @task is (transfer none). */
static void
send_message_data_deliver_reply_unlocked (GTask *task,
GDBusMessage *reply)
@@ -1822,7 +1820,7 @@ send_message_data_deliver_reply_unlocked (GTask *task,
;
}
-/* Called from a user thread, lock is not held; @task is (transfer full) */
+/* Called from a user thread, lock is not held; @task is (transfer none) */
static void
send_message_data_deliver_error (GTask *task,
GQuark domain,
@@ -1839,7 +1837,10 @@ send_message_data_deliver_error (GTask *task,
return;
}
+ /* Hold a ref on @task as send_message_with_reply_cleanup() will remove it
+ * from the task map and could end up dropping the last reference */
g_object_ref (task);
+
send_message_with_reply_cleanup (task, TRUE);
CONNECTION_UNLOCK (connection);
@@ -1855,8 +1856,7 @@ send_message_with_reply_cancelled_idle_cb (gpointer user_data)
{
GTask *task = user_data;
- g_object_ref (task);
- send_message_data_deliver_error (g_steal_pointer (&task), G_IO_ERROR, G_IO_ERROR_CANCELLED,
+ send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_CANCELLED,
_("Operation was cancelled"));
return FALSE;
}
@@ -1886,8 +1886,7 @@ send_message_with_reply_timeout_cb (gpointer user_data)
{
GTask *task = user_data;
- g_object_ref (task);
- send_message_data_deliver_error (g_steal_pointer (&task), G_IO_ERROR, G_IO_ERROR_TIMED_OUT,
+ send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_TIMED_OUT,
_("Timeout was reached"));
return FALSE;
}
@@ -2391,7 +2390,8 @@ on_worker_message_about_to_be_sent (GDBusWorker *worker,
return message;
}
-/* called with connection lock held, in GDBusWorker thread */
+/* called with connection lock held, in GDBusWorker thread
+ * @key, @value and @user_data are (transfer none) */
static gboolean
cancel_method_on_close (gpointer key, gpointer value, gpointer user_data)
{
--
GitLab
From 0a84c182e28f50be2263e42e0bc21074dd523701 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 22 Feb 2023 14:55:40 +0000
Subject: [PATCH 9/9] gdbusconnection: Improve refcount handling of timeout
source
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The ref on the timeout source owned by `SendMessageData` was being
dropped just after attaching the source to the main context, leaving it
unowned in that struct. That meant the only ref on the source was held
by the `GMainContext` it was attached to.
This ref was dropped when returning `G_SOURCE_REMOVE` from
`send_message_with_reply_timeout_cb()`. Before that happens,
`send_message_data_deliver_error()` is called, which normally calls
`send_message_with_reply_cleanup()` and destroys the source.
However, if `send_message_data_deliver_error()` is called when the
message has already been delivered, calling
`send_message_with_reply_cleanup()` will be skipped. This leaves the
source pointer in `SendMessageData` dangling, which will cause problems
when `g_source_destroy()` is subsequently called on it.
I’m not sure if it’s possible in practice for this situation to occur,
but the code certainly does nothing to prevent it, and it’s easy enough
to avoid by keeping a strong ref on the source in `SendMessageData`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1264
Conflict:NA
Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/0a84c182e28f50be2263e42e0bc21074dd523701
---
gio/gdbusconnection.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index f4bc21bb37..bed1ff2841 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -1751,7 +1751,7 @@ typedef struct
gulong cancellable_handler_id;
- GSource *timeout_source;
+ GSource *timeout_source; /* (owned) (nullable) */
gboolean delivered;
} SendMessageData;
@@ -1760,6 +1760,7 @@ typedef struct
static void
send_message_data_free (SendMessageData *data)
{
+ /* These should already have been cleared by send_message_with_reply_cleanup(). */
g_assert (data->timeout_source == NULL);
g_assert (data->cancellable_handler_id == 0);
@@ -1784,7 +1785,7 @@ send_message_with_reply_cleanup (GTask *task, gboolean remove)
if (data->timeout_source != NULL)
{
g_source_destroy (data->timeout_source);
- data->timeout_source = NULL;
+ g_clear_pointer (&data->timeout_source, g_source_unref);
}
if (data->cancellable_handler_id > 0)
{
@@ -1888,7 +1889,7 @@ send_message_with_reply_timeout_cb (gpointer user_data)
send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_TIMED_OUT,
_("Timeout was reached"));
- return FALSE;
+ return G_SOURCE_REMOVE;
}
/* ---------------------------------------------------------------------------------------------------- */
@@ -1949,7 +1950,6 @@ g_dbus_connection_send_message_with_reply_unlocked (GDBusConnection *connect
data->timeout_source = g_timeout_source_new (timeout_msec);
g_task_attach_source (task, data->timeout_source,
(GSourceFunc) send_message_with_reply_timeout_cb);
- g_source_unref (data->timeout_source);
}
g_hash_table_insert (connection->map_method_serial_to_task,
--
GitLab

59
backport-gkeyfile-Ensure-we-don-t-add-extra-blank-line-above-new-group.patch Normal file
View File

@ -0,0 +1,59 @@
From c49502582faedecc7020155d95b16c7a1d78d432 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ga=C3=ABl=20Bonithon?= <gael@xfce.org>
Date: Thu, 13 Jul 2023 10:06:21 +0200
Subject: [PATCH] gkeyfile: Ensure we don't add extra blank line above new
group
A forgotten edge case in 86b4b045: when the last value of the last group
has been added via g_key_file_set_value() and it contains line breaks.
The best we can do in this case is probably to do nothing.
Closes: #3047
Fixes: 86b4b0453ea3a814167d4a5f7a4031d467543716
---
glib/gkeyfile.c | 6 +++++-
glib/tests/keyfile.c | 10 ++++++++++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/glib/gkeyfile.c b/glib/gkeyfile.c
index 145136706f..0e21ab4f14 100644
--- a/glib/gkeyfile.c
+++ b/glib/gkeyfile.c
@@ -3858,8 +3858,12 @@ g_key_file_add_group (GKeyFile *key_file,
{
/* separate groups by a blank line if we don't keep comments or group is created */
GKeyFileGroup *next_group = key_file->groups->next->data;
+ GKeyFileKeyValuePair *pair;
+ if (next_group->key_value_pairs != NULL)
+ pair = next_group->key_value_pairs->data;
+
if (next_group->key_value_pairs == NULL ||
- ((GKeyFileKeyValuePair *) next_group->key_value_pairs->data)->key != NULL)
+ (pair->key != NULL && !g_strstr_len (pair->value, -1, "\n")))
{
GKeyFileKeyValuePair *pair = g_new (GKeyFileKeyValuePair, 1);
pair->key = NULL;
diff --git a/glib/tests/keyfile.c b/glib/tests/keyfile.c
index d3eed29841..80cdc93d8f 100644
--- a/glib/tests/keyfile.c
+++ b/glib/tests/keyfile.c
@@ -480,6 +480,16 @@ test_comments (void)
G_KEY_FILE_ERROR_GROUP_NOT_FOUND);
g_assert_null (comment);
+ g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/3047");
+
+ /* check if we don't add a blank line above new group if last value of preceding
+ * group was added via g_key_file_set_value() and contains line breaks */
+ g_key_file_set_value (keyfile, "group4", "key1", "value1\n\n# group comment");
+ g_key_file_set_string (keyfile, "group5", "key1", "value1");
+ comment = g_key_file_get_comment (keyfile, "group5", NULL, &error);
+ check_no_error (&error);
+ g_assert_null (comment);
+
g_key_file_free (keyfile);
}
--
GitLab

536
backport-gkeyfile-Fix-group-comment-management.patch Normal file
View File

@ -0,0 +1,536 @@
From f74589f53041abff29d538a5d9884c3202f2c00a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ga=C3=ABl=20Bonithon?= <gael@xfce.org>
Date: Thu, 20 Apr 2023 16:52:19 +0200
Subject: [PATCH 1/2] gkeyfile: Replace g_slice_*() with
g_new*()/g_free_sized()
Conflict:NA
Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/f74589f53041abff29d538a5d9884c3202f2c00a
---
glib/gkeyfile.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/glib/gkeyfile.c b/glib/gkeyfile.c
index 9a4821bc5a..d76335653f 100644
--- a/glib/gkeyfile.c
+++ b/glib/gkeyfile.c
@@ -638,7 +638,7 @@ G_DEFINE_QUARK (g-key-file-error-quark, g_key_file_error)
static void
g_key_file_init (GKeyFile *key_file)
{
- key_file->current_group = g_slice_new0 (GKeyFileGroup);
+ key_file->current_group = g_new0 (GKeyFileGroup, 1);
key_file->groups = g_list_prepend (NULL, key_file->current_group);
key_file->group_hash = NULL;
key_file->start_group = NULL;
@@ -700,7 +700,7 @@ g_key_file_new (void)
{
GKeyFile *key_file;
- key_file = g_slice_new0 (GKeyFile);
+ key_file = g_new0 (GKeyFile, 1);
key_file->ref_count = 1;
g_key_file_init (key_file);
@@ -1205,7 +1205,7 @@ g_key_file_free (GKeyFile *key_file)
g_key_file_clear (key_file);
if (g_atomic_int_dec_and_test (&key_file->ref_count))
- g_slice_free (GKeyFile, key_file);
+ g_free_sized (key_file, sizeof (GKeyFile));
else
g_key_file_init (key_file);
}
@@ -1227,7 +1227,7 @@ g_key_file_unref (GKeyFile *key_file)
if (g_atomic_int_dec_and_test (&key_file->ref_count))
{
g_key_file_clear (key_file);
- g_slice_free (GKeyFile, key_file);
+ g_free_sized (key_file, sizeof (GKeyFile));
}
}
@@ -1317,7 +1317,7 @@ g_key_file_parse_comment (GKeyFile *key_file,
g_warn_if_fail (key_file->current_group != NULL);
- pair = g_slice_new (GKeyFileKeyValuePair);
+ pair = g_new (GKeyFileKeyValuePair, 1);
pair->key = NULL;
pair->value = g_strndup (line, length);
@@ -1442,7 +1442,7 @@ g_key_file_parse_key_value_pair (GKeyFile *key_file,
{
GKeyFileKeyValuePair *pair;
- pair = g_slice_new (GKeyFileKeyValuePair);
+ pair = g_new (GKeyFileKeyValuePair, 1);
pair->key = g_steal_pointer (&key);
pair->value = g_strndup (value_start, value_len);
@@ -3339,7 +3339,7 @@ g_key_file_set_key_comment (GKeyFile *key_file,
/* Now we can add our new comment
*/
- pair = g_slice_new (GKeyFileKeyValuePair);
+ pair = g_new (GKeyFileKeyValuePair, 1);
pair->key = NULL;
pair->value = g_key_file_parse_comment_as_value (key_file, comment);
@@ -3383,7 +3383,7 @@ g_key_file_set_group_comment (GKeyFile *key_file,
/* Now we can add our new comment
*/
- group->comment = g_slice_new (GKeyFileKeyValuePair);
+ group->comment = g_new (GKeyFileKeyValuePair, 1);
group->comment->key = NULL;
group->comment->value = g_key_file_parse_comment_as_value (key_file, comment);
@@ -3416,7 +3416,7 @@ g_key_file_set_top_comment (GKeyFile *key_file,
if (comment == NULL)
return TRUE;
- pair = g_slice_new (GKeyFileKeyValuePair);
+ pair = g_new (GKeyFileKeyValuePair, 1);
pair->key = NULL;
pair->value = g_key_file_parse_comment_as_value (key_file, comment);
@@ -3840,7 +3840,7 @@ g_key_file_add_group (GKeyFile *key_file,
return;
}
- group = g_slice_new0 (GKeyFileGroup);
+ group = g_new0 (GKeyFileGroup, 1);
group->name = g_strdup (group_name);
group->lookup_map = g_hash_table_new (g_str_hash, g_str_equal);
key_file->groups = g_list_prepend (key_file->groups, group);
@@ -3862,7 +3862,7 @@ g_key_file_key_value_pair_free (GKeyFileKeyValuePair *pair)
{
g_free (pair->key);
g_free (pair->value);
- g_slice_free (GKeyFileKeyValuePair, pair);
+ g_free_sized (pair, sizeof (GKeyFileKeyValuePair));
}
}
@@ -3971,7 +3971,7 @@ g_key_file_remove_group_node (GKeyFile *key_file,
}
g_free ((gchar *) group->name);
- g_slice_free (GKeyFileGroup, group);
+ g_free_sized (group, sizeof (GKeyFileGroup));
g_list_free_1 (group_node);
}
@@ -4031,7 +4031,7 @@ g_key_file_add_key (GKeyFile *key_file,
{
GKeyFileKeyValuePair *pair;
- pair = g_slice_new (GKeyFileKeyValuePair);
+ pair = g_new (GKeyFileKeyValuePair, 1);
pair->key = g_strdup (key);
pair->value = g_strdup (value);
--
GitLab
From 86b4b0453ea3a814167d4a5f7a4031d467543716 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ga=C3=ABl=20Bonithon?= <gael@xfce.org>
Date: Fri, 14 Apr 2023 19:40:30 +0200
Subject: [PATCH 2/2] gkeyfile: Fix group comment management
This removes the `comment` member of the GKeyFileGroup structure, which
seemed intended to distinguish comments just above a group from comments
above them, separated by one or more blank lines. Indeed:
* This does not seem to match any specification in the documentation,
where blank lines and lines starting with `#` are indiscriminately
considered comments. In particular, no distinction is made between the
comment above the first group and the comment at the beginning of the
file.
* This distinction was only half implemented, resulting in confusion
between comment above a group and comment at the end of the preceding
group.
Instead, the same logic is used for groups as for keys: the comment
above a group is simply the sequence of key-value pairs of the preceding
group where the key is null, starting from the bottom.
The addition of a blank line above groups when writing, involved in
bugs #104 and #2927, is kept, but:
* It is now added as a comment as soon as the group is added (since a
blank line is considered a comment), so that
`g_key_file_get_comment()` returns the correct result right away.
* It is always added if comments are not kept.
* Otherwise it is only added if the group is newly created (not present
in the original data), in order to really keep comments (existing and
not existing).
Closes: #104, #2927
Conflict:NA
Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/86b4b0453ea3a814167d4a5f7a4031d467543716
---
glib/gkeyfile.c | 137 +++++++++++++++++++++++--------------------
glib/tests/keyfile.c | 75 ++++++++++++++++++++++-
2 files changed, 147 insertions(+), 65 deletions(-)
diff --git a/glib/gkeyfile.c b/glib/gkeyfile.c
index d76335653f..1fcef9fc91 100644
--- a/glib/gkeyfile.c
+++ b/glib/gkeyfile.c
@@ -529,8 +529,6 @@ struct _GKeyFileGroup
{
const gchar *name; /* NULL for above first group (which will be comments) */
- GKeyFileKeyValuePair *comment; /* Special comment that is stuck to the top of a group */
-
GList *key_value_pairs;
/* Used in parallel with key_value_pairs for
@@ -579,7 +577,8 @@ static void g_key_file_add_key (GKeyFile
const gchar *key,
const gchar *value);
static void g_key_file_add_group (GKeyFile *key_file,
- const gchar *group_name);
+ const gchar *group_name,
+ gboolean created);
static gboolean g_key_file_is_group_name (const gchar *name);
static gboolean g_key_file_is_key_name (const gchar *name,
gsize len);
@@ -1354,7 +1353,7 @@ g_key_file_parse_group (GKeyFile *key_file,
return;
}
- g_key_file_add_group (key_file, group_name);
+ g_key_file_add_group (key_file, group_name, FALSE);
g_free (group_name);
}
@@ -1610,14 +1609,6 @@ g_key_file_to_data (GKeyFile *key_file,
group = (GKeyFileGroup *) group_node->data;
- /* separate groups by at least an empty line */
- if (data_string->len >= 2 &&
- data_string->str[data_string->len - 2] != '\n')
- g_string_append_c (data_string, '\n');
-
- if (group->comment != NULL)
- g_string_append_printf (data_string, "%s\n", group->comment->value);
-
if (group->name != NULL)
g_string_append_printf (data_string, "[%s]\n", group->name);
@@ -1902,7 +1893,7 @@ g_key_file_set_value (GKeyFile *key_file,
if (!group)
{
- g_key_file_add_group (key_file, group_name);
+ g_key_file_add_group (key_file, group_name, TRUE);
group = (GKeyFileGroup *) key_file->groups->data;
g_key_file_add_key (key_file, group, key, value);
@@ -3349,6 +3340,42 @@ g_key_file_set_key_comment (GKeyFile *key_file,
return TRUE;
}
+static gboolean
+g_key_file_set_top_comment (GKeyFile *key_file,
+ const gchar *comment,
+ GError **error)
+{
+ GList *group_node;
+ GKeyFileGroup *group;
+ GKeyFileKeyValuePair *pair;
+
+ /* The last group in the list should be the top (comments only)
+ * group in the file
+ */
+ g_warn_if_fail (key_file->groups != NULL);
+ group_node = g_list_last (key_file->groups);
+ group = (GKeyFileGroup *) group_node->data;
+ g_warn_if_fail (group->name == NULL);
+
+ /* Note all keys must be comments at the top of
+ * the file, so we can just free it all.
+ */
+ g_list_free_full (group->key_value_pairs, (GDestroyNotify) g_key_file_key_value_pair_free);
+ group->key_value_pairs = NULL;
+
+ if (comment == NULL)
+ return TRUE;
+
+ pair = g_new (GKeyFileKeyValuePair, 1);
+ pair->key = NULL;
+ pair->value = g_key_file_parse_comment_as_value (key_file, comment);
+
+ group->key_value_pairs =
+ g_list_prepend (group->key_value_pairs, pair);
+
+ return TRUE;
+}
+
static gboolean
g_key_file_set_group_comment (GKeyFile *key_file,
const gchar *group_name,
@@ -3356,6 +3383,8 @@ g_key_file_set_group_comment (GKeyFile *key_file,
GError **error)
{
GKeyFileGroup *group;
+ GList *group_node;
+ GKeyFileKeyValuePair *pair;
g_return_val_if_fail (group_name != NULL && g_key_file_is_group_name (group_name), FALSE);
@@ -3370,12 +3399,22 @@ g_key_file_set_group_comment (GKeyFile *key_file,
return FALSE;
}
+ if (group == key_file->start_group)
+ return g_key_file_set_top_comment (key_file, comment, error);
+
/* First remove any existing comment
*/
- if (group->comment)
+ group_node = g_key_file_lookup_group_node (key_file, group_name);
+ group = group_node->next->data;
+ for (GList *lp = group->key_value_pairs; lp != NULL; )
{
- g_key_file_key_value_pair_free (group->comment);
- group->comment = NULL;
+ GList *lnext = lp->next;
+ pair = lp->data;
+ if (pair->key != NULL)
+ break;
+
+ g_key_file_remove_key_value_pair_node (key_file, group, lp);
+ lp = lnext;
}
if (comment == NULL)
@@ -3383,45 +3422,10 @@ g_key_file_set_group_comment (GKeyFile *key_file,
/* Now we can add our new comment
*/
- group->comment = g_new (GKeyFileKeyValuePair, 1);
- group->comment->key = NULL;
- group->comment->value = g_key_file_parse_comment_as_value (key_file, comment);
-
- return TRUE;
-}
-
-static gboolean
-g_key_file_set_top_comment (GKeyFile *key_file,
- const gchar *comment,
- GError **error)
-{
- GList *group_node;
- GKeyFileGroup *group;
- GKeyFileKeyValuePair *pair;
-
- /* The last group in the list should be the top (comments only)
- * group in the file
- */
- g_warn_if_fail (key_file->groups != NULL);
- group_node = g_list_last (key_file->groups);
- group = (GKeyFileGroup *) group_node->data;
- g_warn_if_fail (group->name == NULL);
-
- /* Note all keys must be comments at the top of
- * the file, so we can just free it all.
- */
- g_list_free_full (group->key_value_pairs, (GDestroyNotify) g_key_file_key_value_pair_free);
- group->key_value_pairs = NULL;
-
- if (comment == NULL)
- return TRUE;
-
pair = g_new (GKeyFileKeyValuePair, 1);
pair->key = NULL;
pair->value = g_key_file_parse_comment_as_value (key_file, comment);
-
- group->key_value_pairs =
- g_list_prepend (group->key_value_pairs, pair);
+ group->key_value_pairs = g_list_prepend (group->key_value_pairs, pair);
return TRUE;
}
@@ -3629,9 +3633,6 @@ g_key_file_get_group_comment (GKeyFile *key_file,
return NULL;
}
- if (group->comment)
- return g_strdup (group->comment->value);
-
group_node = g_key_file_lookup_group_node (key_file, group_name);
group_node = group_node->next;
group = (GKeyFileGroup *)group_node->data;
@@ -3826,7 +3827,8 @@ g_key_file_has_key (GKeyFile *key_file,
static void
g_key_file_add_group (GKeyFile *key_file,
- const gchar *group_name)
+ const gchar *group_name,
+ gboolean created)
{
GKeyFileGroup *group;
@@ -3847,7 +3849,22 @@ g_key_file_add_group (GKeyFile *key_file,
key_file->current_group = group;
if (key_file->start_group == NULL)
- key_file->start_group = group;
+ {
+ key_file->start_group = group;
+ }
+ else if (!(key_file->flags & G_KEY_FILE_KEEP_COMMENTS) || created)
+ {
+ /* separate groups by a blank line if we don't keep comments or group is created */
+ GKeyFileGroup *next_group = key_file->groups->next->data;
+ if (next_group->key_value_pairs == NULL ||
+ ((GKeyFileKeyValuePair *) next_group->key_value_pairs->data)->key != NULL)
+ {
+ GKeyFileKeyValuePair *pair = g_new (GKeyFileKeyValuePair, 1);
+ pair->key = NULL;
+ pair->value = g_strdup ("");
+ next_group->key_value_pairs = g_list_prepend (next_group->key_value_pairs, pair);
+ }
+ }
if (!key_file->group_hash)
key_file->group_hash = g_hash_table_new (g_str_hash, g_str_equal);
@@ -3958,12 +3975,6 @@ g_key_file_remove_group_node (GKeyFile *key_file,
g_warn_if_fail (group->key_value_pairs == NULL);
- if (group->comment)
- {
- g_key_file_key_value_pair_free (group->comment);
- group->comment = NULL;
- }
-
if (group->lookup_map)
{
g_hash_table_destroy (group->lookup_map);
diff --git a/glib/tests/keyfile.c b/glib/tests/keyfile.c
index 3d72d9670e..d3eed29841 100644
--- a/glib/tests/keyfile.c
+++ b/glib/tests/keyfile.c
@@ -382,7 +382,9 @@ test_comments (void)
"key4 = value4\n"
"# group comment\n"
"# group comment, continued\n"
- "[group2]\n";
+ "[group2]\n\n"
+ "[group3]\n"
+ "[group4]\n";
const gchar *top_comment = " top comment\n top comment, continued";
const gchar *group_comment = " group comment\n group comment, continued";
@@ -427,6 +429,12 @@ test_comments (void)
check_name ("top comment", comment, top_comment, 0);
g_free (comment);
+ g_key_file_remove_comment (keyfile, NULL, NULL, &error);
+ check_no_error (&error);
+ comment = g_key_file_get_comment (keyfile, NULL, NULL, &error);
+ check_no_error (&error);
+ g_assert_null (comment);
+
comment = g_key_file_get_comment (keyfile, "group1", "key2", &error);
check_no_error (&error);
check_name ("key comment", comment, key_comment, 0);
@@ -448,7 +456,25 @@ test_comments (void)
check_name ("group comment", comment, group_comment, 0);
g_free (comment);
+ g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/104");
+
+ /* check if comments above another group than the first one are properly removed */
+ g_key_file_remove_comment (keyfile, "group2", NULL, &error);
+ check_no_error (&error);
+ comment = g_key_file_get_comment (keyfile, "group2", NULL, &error);
+ check_no_error (&error);
+ g_assert_null (comment);
+
comment = g_key_file_get_comment (keyfile, "group3", NULL, &error);
+ check_no_error (&error);
+ check_name ("group comment", comment, "", 0);
+ g_free (comment);
+
+ comment = g_key_file_get_comment (keyfile, "group4", NULL, &error);
+ check_no_error (&error);
+ g_assert_null (comment);
+
+ comment = g_key_file_get_comment (keyfile, "group5", NULL, &error);
check_error (&error,
G_KEY_FILE_ERROR,
G_KEY_FILE_ERROR_GROUP_NOT_FOUND);
@@ -1321,9 +1347,16 @@ test_reload_idempotency (void)
"[fifth]\n";
GKeyFile *keyfile;
GError *error = NULL;
- gchar *data1, *data2;
+ gchar *data1, *data2, *comment;
gsize len1, len2;
+ const gchar *key_comment = " A random comment in the first group";
+ const gchar *top_comment = " Top comment\n\n First comment";
+ const gchar *group_comment_1 = top_comment;
+ const gchar *group_comment_2 = " Second comment - one line";
+ const gchar *group_comment_3 = " Third comment - two lines\n Third comment - two lines";
+ const gchar *group_comment_4 = "\n";
+
g_test_bug ("https://bugzilla.gnome.org/show_bug.cgi?id=420686");
/* check that we only insert a single new line between groups */
@@ -1347,6 +1380,44 @@ test_reload_idempotency (void)
data2 = g_key_file_to_data (keyfile, &len2, &error);
g_assert_nonnull (data2);
+
+ g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2927");
+
+ /* check if comments are preserved on reload */
+ comment = g_key_file_get_comment (keyfile, "first", "anotherkey", &error);
+ check_no_error (&error);
+ g_assert_cmpstr (comment, ==, key_comment);
+ g_free (comment);
+
+ comment = g_key_file_get_comment (keyfile, NULL, NULL, &error);
+ check_no_error (&error);
+ g_assert_cmpstr (comment, ==, top_comment);
+ g_free (comment);
+
+ comment = g_key_file_get_comment (keyfile, "first", NULL, &error);
+ check_no_error (&error);
+ g_assert_cmpstr (comment, ==, group_comment_1);
+ g_free (comment);
+
+ comment = g_key_file_get_comment (keyfile, "second", NULL, &error);
+ check_no_error (&error);
+ g_assert_cmpstr (comment, ==, group_comment_2);
+ g_free (comment);
+
+ comment = g_key_file_get_comment (keyfile, "third", NULL, &error);
+ check_no_error (&error);
+ g_assert_cmpstr (comment, ==, group_comment_3);
+ g_free (comment);
+
+ comment = g_key_file_get_comment (keyfile, "fourth", NULL, &error);
+ check_no_error (&error);
+ g_assert_cmpstr (comment, ==, group_comment_4);
+ g_free (comment);
+
+ comment = g_key_file_get_comment (keyfile, "fifth", NULL, &error);
+ check_no_error (&error);
+ g_assert_null (comment);
+
g_key_file_free (keyfile);
g_assert_cmpstr (data1, ==, data2);
--
GitLab

97
backport-gkeyfile-Skip-group-comment-when-adding-a-new-key-to-a-group.patch Normal file
View File

@ -0,0 +1,97 @@
From 51dfb3c229c0478b3615f486fbbc36de2586bd52 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ga=C3=ABl=20Bonithon?= <gael@xfce.org>
Date: Thu, 13 Jul 2023 10:19:04 +0200
Subject: [PATCH] gkeyfile: Skip group comment when adding a new key to a group
An oversight in 86b4b045: since the comment of group N now consists of
the last null-key values of group N-1, these keys must obviously be
skipped when adding a new non-null key to group N-1.
Closes: #3047
Fixes: 86b4b0453ea3a814167d4a5f7a4031d467543716
---
glib/gkeyfile.c | 19 ++++++++++++++-----
glib/tests/keyfile.c | 9 +++++++++
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/glib/gkeyfile.c b/glib/gkeyfile.c
index 0e21ab4f14..4759051977 100644
--- a/glib/gkeyfile.c
+++ b/glib/gkeyfile.c
@@ -573,7 +573,8 @@ static void g_key_file_remove_key_value_pair_node (GKeyFile
static void g_key_file_add_key_value_pair (GKeyFile *key_file,
GKeyFileGroup *group,
- GKeyFileKeyValuePair *pair);
+ GKeyFileKeyValuePair *pair,
+ GList *sibling);
static void g_key_file_add_key (GKeyFile *key_file,
GKeyFileGroup *group,
const gchar *key,
@@ -1447,7 +1448,8 @@ g_key_file_parse_key_value_pair (GKeyFile *key_file,
pair->key = g_steal_pointer (&key);
pair->value = g_strndup (value_start, value_len);
- g_key_file_add_key_value_pair (key_file, key_file->current_group, pair);
+ g_key_file_add_key_value_pair (key_file, key_file->current_group, pair,
+ key_file->current_group->key_value_pairs);
}
g_free (key);
@@ -4034,10 +4036,11 @@ g_key_file_remove_group (GKeyFile *key_file,
static void
g_key_file_add_key_value_pair (GKeyFile *key_file,
GKeyFileGroup *group,
- GKeyFileKeyValuePair *pair)
+ GKeyFileKeyValuePair *pair,
+ GList *sibling)
{
g_hash_table_replace (group->lookup_map, pair->key, pair);
- group->key_value_pairs = g_list_prepend (group->key_value_pairs, pair);
+ group->key_value_pairs = g_list_insert_before (group->key_value_pairs, sibling, pair);
}
static void
@@ -4047,12 +4050,18 @@ g_key_file_add_key (GKeyFile *key_file,
const gchar *value)
{
GKeyFileKeyValuePair *pair;
+ GList *lp;
pair = g_new (GKeyFileKeyValuePair, 1);
pair->key = g_strdup (key);
pair->value = g_strdup (value);
- g_key_file_add_key_value_pair (key_file, group, pair);
+ /* skip group comment */
+ lp = group->key_value_pairs;
+ while (lp != NULL && ((GKeyFileKeyValuePair *) lp->data)->key == NULL)
+ lp = lp->next;
+
+ g_key_file_add_key_value_pair (key_file, group, pair, lp);
}
/**
diff --git a/glib/tests/keyfile.c b/glib/tests/keyfile.c
index 80cdc93d8f..2c8eca4ebc 100644
--- a/glib/tests/keyfile.c
+++ b/glib/tests/keyfile.c
@@ -456,6 +456,15 @@ test_comments (void)
check_name ("group comment", comment, group_comment, 0);
g_free (comment);
+ g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/3047");
+
+ /* check if adding a key to group N preserve group comment of group N+1 */
+ g_key_file_set_string (keyfile, "group1", "key5", "value5");
+ comment = g_key_file_get_comment (keyfile, "group2", NULL, &error);
+ check_no_error (&error);
+ check_name ("group comment", comment, group_comment, 0);
+ g_free (comment);
+
g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/104");
/* check if comments above another group than the first one are properly removed */
--
GitLab

16
glib2.spec
View File

@ -1,6 +1,6 @@
Name: glib2
Version: 2.72.2
Release: 10
Release: 11
Summary: The core library that forms the basis for projects such as GTK+ and GNOME
License: LGPLv2+
URL: http://www.gtk.org
@ -59,9 +59,14 @@ Patch6049: backport-gregex-Drop-explanation-G_REGEX_JAVASCRIPT_COMPAT.patch
Patch6050: backport-gregex-Remove-an-unreachable-return-statement.patch
Patch6051: backport-gmessages-Add-missing-trailing-newline-in-fallback-log-hander.patch
Patch6052: backport-Revert-Handling-collision-between-standard-i-o-filedescriptors-and-newly-created-ones.patch
patch6053: backport-gdbusinterfaceskeleton-Fix-a-use-after-free-of-a-GDBusMethodInvocation.patch
patch6054: backport-CVE-2023-24593_CVE-2023-25180-1.patch
patch6055: backport-CVE-2023-24593_CVE-2023-25180-2.patch
patch6053: backport-gdbusinterfaceskeleton-Fix-a-use-after-free-of-a-GDBusMethodInvocation.patch
patch6054: backport-CVE-2023-24593_CVE-2023-25180-1.patch
patch6055: backport-CVE-2023-24593_CVE-2023-25180-2.patch
patch6056: backport-gdbusconnection-Fix-double-unref-on-timeout-cancel-sending-a-message.patch
patch6057: backport-add-g_free_sized-and-g_aligned_free_sized.patch
patch6058: backport-gkeyfile-Fix-group-comment-management.patch
patch6059: backport-gkeyfile-Ensure-we-don-t-add-extra-blank-line-above-new-group.patch
patch6060: backport-gkeyfile-Skip-group-comment-when-adding-a-new-key-to-a-group.patch
BuildRequires: chrpath gcc gcc-c++ gettext perl-interpreter
BUildRequires: glibc-devel libattr-devel libselinux-devel meson
@ -257,6 +262,9 @@ glib-compile-schemas %{_datadir}/glib-2.0/schemas &> /dev/null || :
%endif
%changelog
* Sat Aug 19 2023 hanhuihui <hanhuihui5@huawei.com> - 2.72.2-11
- fix double unref and fix group comment management
* Sat Apr 1 2023 hanhuihui <hanhuihui5@huawei.com> - 2.72.2-10
- fix CVE-2023-24593 and CVE-2023-25180