From e2c3581e370907a48249d8fe646e0c30d3eda0ec Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 9 Apr 2024 16:11:09 +0100 Subject: [PATCH 1/8] gdbusconnection: Ensure out_serial return value is always set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There were some error paths where it wasn’t set, returning an uninitialised value to the caller. Spotted by scan-build. Signed-off-by: Philip Withnall Helps: #1767 --- gio/gdbusconnection.c | 1 + 1 file changed, 1 insertion(+) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 42134a601..1e13606d1 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -1897,6 +1897,7 @@ g_dbus_connection_send_message_with_reply_unlocked (GDBusConnection *connect if (out_serial == NULL) out_serial = &serial; + *out_serial = 0; if (timeout_msec == -1) timeout_msec = 25 * 1000; From 4141eea25739bed16c553a4304d41c43f9165939 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 10 Apr 2024 00:20:24 +0100 Subject: [PATCH 2/8] girffi: Add hints to indicate ownership transfer into ffi_cif MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scan-build thinks that the `atypes` array is leaked, but it’s not. Ownership is transferred into the `ffi_cif` structure, and it’s eventually freed in `gi_callable_info_destroy_closure()`. Try and help the static analysis by adding an explicit ownership transfer annotation. It probably won’t help. Signed-off-by: Philip Withnall Helps: #1767 --- girepository/girffi.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/girepository/girffi.c b/girepository/girffi.c index 053fd3439..1f3b4b282 100644 --- a/girepository/girffi.c +++ b/girepository/girffi.c @@ -339,7 +339,7 @@ gi_function_invoker_new_for_address (void *addr, return ffi_prep_cif (&(invoker->cif), FFI_DEFAULT_ABI, n_args, gi_callable_info_get_ffi_return_type (info), - atypes) == FFI_OK; + g_steal_pointer (&atypes)) == FFI_OK; } /** @@ -409,6 +409,11 @@ gi_callable_info_create_closure (GICallableInfo *callable_info, status = ffi_prep_cif (cif, FFI_DEFAULT_ABI, n_args, gi_callable_info_get_ffi_return_type (callable_info), atypes); + + /* Explicitly store atypes to satisfy static analysers, which can’t see inside + * ffi_prep_cif(), and hence assume that it’s leaked. */ + cif->arg_types = g_steal_pointer (&atypes); + if (status != FFI_OK) { g_warning ("ffi_prep_cif failed: %d", status); From 39f0e6d435f9110c6bbd9783603edc6fb5ae9828 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 15:51:39 +0100 Subject: [PATCH 3/8] girffi: Fix ffi_cif leaks on error return paths Spotted by scan-build. Signed-off-by: Philip Withnall Helps: #1767 --- girepository/girffi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/girepository/girffi.c b/girepository/girffi.c index 1f3b4b282..d8102a25c 100644 --- a/girepository/girffi.c +++ b/girepository/girffi.c @@ -417,7 +417,7 @@ gi_callable_info_create_closure (GICallableInfo *callable_info, if (status != FFI_OK) { g_warning ("ffi_prep_cif failed: %d", status); - ffi_closure_free (closure); + gi_callable_info_destroy_closure (callable_info, &closure->ffi_closure); return NULL; } @@ -425,7 +425,7 @@ gi_callable_info_create_closure (GICallableInfo *callable_info, if (status != FFI_OK) { g_warning ("ffi_prep_closure failed: %d", status); - ffi_closure_free (closure); + gi_callable_info_destroy_closure (callable_info, &closure->ffi_closure); return NULL; } From 747e3af9987b37847d7d5acbf882d1ee4a6bd91b Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 10 Apr 2024 00:44:40 +0100 Subject: [PATCH 4/8] gdbusconnection: Fix user_data leaks on error There were a couple of functions in `GDBusConnection` which take a `user_data` argument, but which then leak it if they error out early. A true positive spotted by scan-build! Signed-off-by: Philip Withnall Helps: #1767 --- gio/gdbusconnection.c | 6 ++++++ gio/tests/gdbus-export.c | 19 ++++++++++++------- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 1e13606d1..1b31f4b67 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -5364,6 +5364,9 @@ g_dbus_connection_register_object (GDBusConnection *connection, out: CONNECTION_UNLOCK (connection); + if (ret == 0 && user_data_free_func != NULL) + user_data_free_func (user_data); + return ret; } @@ -7020,6 +7023,9 @@ g_dbus_connection_register_subtree (GDBusConnection *connection, out: CONNECTION_UNLOCK (connection); + if (ret == 0 && user_data_free_func != NULL) + user_data_free_func (user_data); + return ret; } diff --git a/gio/tests/gdbus-export.c b/gio/tests/gdbus-export.c index 2080ebe12..f7efe8c03 100644 --- a/gio/tests/gdbus-export.c +++ b/gio/tests/gdbus-export.c @@ -1003,12 +1003,14 @@ test_object_registration (void) guint non_subtree_object_path_bar_reg_id; guint dyna_subtree_registration_id; guint num_successful_registrations; + guint num_failed_registrations; data.num_unregistered_calls = 0; data.num_unregistered_subtree_calls = 0; data.num_subtree_nodes = 0; num_successful_registrations = 0; + num_failed_registrations = 0; error = NULL; c = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error); @@ -1100,7 +1102,8 @@ test_object_registration (void) intern2_bar_reg_id = registration_id; num_successful_registrations++; - /* register at the same path/interface - this should fail */ + /* register at the same path/interface - this should fail and result in an + * immediate unregistration (so the user_data isn’t leaked) */ registration_id = g_dbus_connection_register_object (c, "/foo/boss/interns/intern2", (GDBusInterfaceInfo *) &bar_interface_info, @@ -1113,6 +1116,8 @@ test_object_registration (void) g_error_free (error); error = NULL; g_assert (registration_id == 0); + g_assert_cmpint (data.num_unregistered_calls, ==, 1); + num_failed_registrations++; /* register at different interface - shouldn't fail */ registration_id = g_dbus_connection_register_object (c, @@ -1130,7 +1135,7 @@ test_object_registration (void) /* unregister it via the id */ g_assert (g_dbus_connection_unregister_object (c, registration_id)); g_main_context_iteration (NULL, FALSE); - g_assert_cmpint (data.num_unregistered_calls, ==, 1); + g_assert_cmpint (data.num_unregistered_calls, ==, 2); intern2_foo_reg_id = 0; /* register it back */ @@ -1181,12 +1186,12 @@ test_object_registration (void) g_error_free (error); error = NULL; g_assert (registration_id == 0); + g_assert_cmpint (data.num_unregistered_subtree_calls, ==, 1); /* unregister it, then register it again */ - g_assert_cmpint (data.num_unregistered_subtree_calls, ==, 0); g_assert (g_dbus_connection_unregister_subtree (c, subtree_registration_id)); g_main_context_iteration (NULL, FALSE); - g_assert_cmpint (data.num_unregistered_subtree_calls, ==, 1); + g_assert_cmpint (data.num_unregistered_subtree_calls, ==, 2); subtree_registration_id = g_dbus_connection_register_subtree (c, "/foo/boss/executives", &subtree_vtable, @@ -1382,10 +1387,10 @@ test_object_registration (void) #endif /* check that unregistering the subtree handler works */ - g_assert_cmpint (data.num_unregistered_subtree_calls, ==, 1); + g_assert_cmpint (data.num_unregistered_subtree_calls, ==, 2); g_assert (g_dbus_connection_unregister_subtree (c, subtree_registration_id)); g_main_context_iteration (NULL, FALSE); - g_assert_cmpint (data.num_unregistered_subtree_calls, ==, 2); + g_assert_cmpint (data.num_unregistered_subtree_calls, ==, 3); nodes = get_nodes_at (c, "/foo/boss/executives"); g_assert (nodes != NULL); g_assert_cmpint (g_strv_length (nodes), ==, 1); @@ -1405,7 +1410,7 @@ test_object_registration (void) g_assert (g_dbus_connection_unregister_object (c, non_subtree_object_path_foo_reg_id)); g_main_context_iteration (NULL, FALSE); - g_assert_cmpint (data.num_unregistered_calls, ==, num_successful_registrations); + g_assert_cmpint (data.num_unregistered_calls, ==, num_successful_registrations + num_failed_registrations); /* check that we no longer export any objects - TODO: it looks like there's a bug in * libdbus-1 here: libdbus still reports the '/foo' object; so disable the test for now From 21f5e175d4a7954a9f77cc2c98cbd075e8efa72f Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 15:39:41 +0100 Subject: [PATCH 5/8] xdgmimeglob: Fix a memory leak on a duplicate-entry path Rather than `strdup()`ing strings when passing them into `_xdg_glob_list_append()`, `strdup()` them *inside* the function instead. This avoids a leak in the case that the list entry (tuple of `data` and `mime_type`) already exists in the list. This has been upstreamed as https://gitlab.freedesktop.org/xdg/xdgmime/-/merge_requests/36. Signed-off-by: Philip Withnall --- gio/xdgmime/xdgmimeglob.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/gio/xdgmime/xdgmimeglob.c b/gio/xdgmime/xdgmimeglob.c index e84580873..f40f448ae 100644 --- a/gio/xdgmime/xdgmimeglob.c +++ b/gio/xdgmime/xdgmimeglob.c @@ -98,7 +98,7 @@ _xdg_glob_list_free (XdgGlobList *glob_list) static XdgGlobList * _xdg_glob_list_append (XdgGlobList *glob_list, - void *data, + const char *data, const char *mime_type, int weight, int case_sensitive) @@ -117,8 +117,8 @@ _xdg_glob_list_append (XdgGlobList *glob_list, } new_element = _xdg_glob_list_new (); - new_element->data = data; - new_element->mime_type = mime_type; + new_element->data = strdup (data); + new_element->mime_type = strdup (mime_type); new_element->weight = weight; new_element->case_sensitive = case_sensitive; if (glob_list == NULL) @@ -576,13 +576,13 @@ _xdg_glob_hash_append_glob (XdgGlobHash *glob_hash, switch (type) { case XDG_GLOB_LITERAL: - glob_hash->literal_list = _xdg_glob_list_append (glob_hash->literal_list, strdup (glob), strdup (mime_type), weight, case_sensitive); + glob_hash->literal_list = _xdg_glob_list_append (glob_hash->literal_list, glob, mime_type, weight, case_sensitive); break; case XDG_GLOB_SIMPLE: glob_hash->simple_node = _xdg_glob_hash_insert_text (glob_hash->simple_node, glob + 1, mime_type, weight, case_sensitive); break; case XDG_GLOB_FULL: - glob_hash->full_list = _xdg_glob_list_append (glob_hash->full_list, strdup (glob), strdup (mime_type), weight, case_sensitive); + glob_hash->full_list = _xdg_glob_list_append (glob_hash->full_list, glob, mime_type, weight, case_sensitive); break; } } From 7c7c00635ef1e1fc0d52c1a1af681e8ec79202c5 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 15:58:20 +0100 Subject: [PATCH 6/8] gspawn: Fix use of uninitialised FDs on error path Spotted by scan-build, an actual true positive result from it, and a fiendish one too. If any of the calls to `dupfd_cloexec()` (except the final one) fail, the remainder of the `duped_source_fds` array would have been left uninitialised. The code in `out_close_fds` would have then called `g_clear_fd()` on an uninitialised FD, with unpredictable results. Signed-off-by: Philip Withnall Helps: #1767 --- glib/gspawn.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/glib/gspawn.c b/glib/gspawn.c index 0ddd53249..d2ef460ab 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -1811,6 +1811,8 @@ do_posix_spawn (const gchar * const *argv, goto out_close_fds; duped_source_fds = g_new (gint, n_fds); + for (i = 0; i < n_fds; i++) + duped_source_fds[i] = -1; /* initialise in case dupfd_cloexec() fails below */ for (i = 0; i < n_fds; i++) { duped_source_fds[i] = dupfd_cloexec (source_fds[i], max_target_fd + 1); From dc2027e72891aa716033e5df68a2c3f82b458b39 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 16:01:20 +0100 Subject: [PATCH 7/8] gparamspecs: Fix NULL pointer dereference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I’m not sure exactly how this code is supposed to work, so this might not be the right fix. But there’s definitely a problem here, and it was spotted by scan-build. If `param_value_array_validate()` is entered with `value->data[0].v_pointer == NULL && aspec->fixed_n_elements`, that `NULL` will be stored in `value_array` too. `value->data[0].v_pointer` will then be set to a new non-`NULL` array. A few lines down, `value_array_ensure_size()` is called on `value_array` – which is still `NULL` – and this results in a `NULL` pointer dereference. It looks like `value->data[0].v_pointer` and `value_array` are used interchangeably throughout the whole of the function, so assign the new value of `value->data[0].v_pointer` to `value_array` too. My guess is that `value_array` is just a convenience alias for `value->data[0].v_pointer`, because the latter is a real mouthful to type or read. Signed-off-by: Philip Withnall Helps: #1767 --- gobject/gparamspecs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gobject/gparamspecs.c b/gobject/gparamspecs.c index cf50df74f..12a81245a 100644 --- a/gobject/gparamspecs.c +++ b/gobject/gparamspecs.c @@ -1018,7 +1018,7 @@ param_value_array_validate (GParamSpec *pspec, guint changed = 0; if (!value->data[0].v_pointer && aspec->fixed_n_elements) - value->data[0].v_pointer = g_value_array_new (aspec->fixed_n_elements); + value_array = value->data[0].v_pointer = g_value_array_new (aspec->fixed_n_elements); if (value->data[0].v_pointer) { From 705ce39b9a530e054e3801a9d90d31b2e741102a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 19:27:34 +0100 Subject: [PATCH 8/8] gvariant-serialiser: Check offsets array is initialised before using it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When piecewise validating the offset table for a variable sized array, it’s possible that the offset table (`offsets.array`) won’t actually have been set by `gvs_variable_sized_array_get_frame_offsets()` iff the serialised `GVariant` is not in normal form. Add an additional check to guard against this. This will result in an empty child variant being returned, as with other error handling paths in `gvs_variable_sized_array_get_child()`. This is a true positive spotted by scan-build. Thanks, scan-build. Signed-off-by: Philip Withnall Helps: #1767 --- glib/gvariant-serialiser.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c index 4e4a73ad1..9a2975b33 100644 --- a/glib/gvariant-serialiser.c +++ b/glib/gvariant-serialiser.c @@ -762,7 +762,8 @@ gvs_variable_sized_array_get_child (GVariantSerialised value, * Don’t bother checking if the highest known-good offset is lower than the * highest checked offset, as that means there’s an invalid element at that * index, so there’s no need to check further. */ - if (index_ > value.checked_offsets_up_to && + if (offsets.array != NULL && + index_ > value.checked_offsets_up_to && value.ordered_offsets_up_to == value.checked_offsets_up_to) { switch (offsets.offset_size)