From 2dfd9518e1184d32a240ac351a7fbc76fdecfda7 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 3 Apr 2024 23:21:31 +0100 Subject: [PATCH 01/16] ci: Exclude tests from scan-build analysis runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They cause too much noise at the moment. I want to make scan-build messages fatal, and with 66 of 238 reports coming from the tests, that’s not currently feasible. Signed-off-by: Philip Withnall Helps: #1767 --- .gitlab-ci.yml | 12 +++++++++++- .gitlab-ci/scan-build.sh | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) create mode 100755 .gitlab-ci/scan-build.sh diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index f8dc428c3..767c156e0 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -725,6 +725,16 @@ scan-build: image: $FEDORA_IMAGE stage: analysis needs: [] + variables: + # FIXME: Eventually we want static analysis on the tests, for code quality, + # but for the moment it’s just busywork. + SCAN_BUILD_FLAGS: >- + --exclude gio/tests/ + --exclude girepository/tests/ + --exclude glib/tests/ + --exclude gmodule/tests/ + --exclude gobject/tests/ + --exclude gthread/tests/ script: - meson setup ${MESON_COMMON_OPTIONS} --werror @@ -737,7 +747,7 @@ scan-build: -Dinstalled_tests=true -Dintrospection=enabled _scan_build - - ninja -C _scan_build scan-build + - SCANBUILD="$(pwd)/.gitlab-ci/scan-build.sh" ninja -C _scan_build scan-build artifacts: name: "glib-${CI_JOB_NAME}-${CI_COMMIT_REF_NAME}" when: always diff --git a/.gitlab-ci/scan-build.sh b/.gitlab-ci/scan-build.sh new file mode 100755 index 000000000..23fc3b979 --- /dev/null +++ b/.gitlab-ci/scan-build.sh @@ -0,0 +1,15 @@ +#!/usr/bin/env bash +# +# Copyright 2024 GNOME Foundation, Inc. +# +# SPDX-License-Identifier: LGPL-2.1-or-later +# +# Original author: Philip Withnall + +set -eu + +# This script just exists so that we can set the scan-build flags in +# .gitlab-ci.yml and pass them into Meson’s `scan-build` target. + +# shellcheck disable=SC2086 +exec scan-build ${SCAN_BUILD_FLAGS:-} "$@" \ No newline at end of file From 165ae8c8a755a353319f62baa58129376255651b Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 4 Apr 2024 00:28:06 +0100 Subject: [PATCH 02/16] =?UTF-8?q?ci:=20Disable=20scan-build=E2=80=99s=20de?= =?UTF-8?q?ad=20code=20checker?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s not highlighting severe bugs for us, and currently generates 132 out of 172 of the scan-build reports, so let’s disable it for now. Signed-off-by: Philip Withnall Helps: #1767 --- .gitlab-ci.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 767c156e0..dd68a207d 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -728,6 +728,8 @@ scan-build: variables: # FIXME: Eventually we want static analysis on the tests, for code quality, # but for the moment it’s just busywork. + # FIXME: Disable the dead code checkers for now because they create a lot of + # noise and don’t indicate high severity problems. SCAN_BUILD_FLAGS: >- --exclude gio/tests/ --exclude girepository/tests/ @@ -735,6 +737,7 @@ scan-build: --exclude gmodule/tests/ --exclude gobject/tests/ --exclude gthread/tests/ + -disable-checker deadcode.DeadStores script: - meson setup ${MESON_COMMON_OPTIONS} --werror From 0f869f3d73d7df1c344dc84268b430e63aa4ccad Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 15:09:20 +0100 Subject: [PATCH 03/16] ci: Disable scan-build for copylibs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eventually, we do want to include them in static analysis (their code is run in the same process as GLib, after all). But for now, that’s too much work to get started. Signed-off-by: Philip Withnall Helps: #1767 --- .gitlab-ci.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index dd68a207d..96db39184 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -726,8 +726,8 @@ scan-build: stage: analysis needs: [] variables: - # FIXME: Eventually we want static analysis on the tests, for code quality, - # but for the moment it’s just busywork. + # FIXME: Eventually we want static analysis on the tests and copylibs, for + # code quality, but for the moment it’s just busywork. # FIXME: Disable the dead code checkers for now because they create a lot of # noise and don’t indicate high severity problems. SCAN_BUILD_FLAGS: >- @@ -737,6 +737,9 @@ scan-build: --exclude gmodule/tests/ --exclude gobject/tests/ --exclude gthread/tests/ + --exclude girepository/cmph/ + --exclude glib/libcharset/ + --exclude gio/xdgmime/ -disable-checker deadcode.DeadStores script: - meson setup ${MESON_COMMON_OPTIONS} From b2f27beb34123e1c85a466a7163d8f5efec71d8e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 16:05:37 +0100 Subject: [PATCH 04/16] build: Enable -Wnull-dereference warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This enables `NULL` pointer dereference checking in the compiler. This isn’t as good as static analysis, but it should hopefully catch some simple errors without too high a false positive rate. If the false positive rate is too high to be useful, we can always disable it again. Signed-off-by: Philip Withnall Helps: #1767 --- meson.build | 1 + 1 file changed, 1 insertion(+) diff --git a/meson.build b/meson.build index f0c5e070b..46c5aa200 100644 --- a/meson.build +++ b/meson.build @@ -549,6 +549,7 @@ if cc.get_id() == 'gcc' or cc.get_id() == 'clang' '-Wmisleading-indentation', '-Wmissing-field-initializers', '-Wnonnull', + '-Wnull-dereference', '-Wunused', # Due to maintained deprecated code, we do not want to see unused parameters '-Wno-unused-parameter', From b8225c905bfff5543d91f57b46aaf5cf17ea60e2 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 9 Apr 2024 16:11:09 +0100 Subject: [PATCH 05/16] 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 e45c93da79e166309e6763410978610ac75e463b Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 10 Apr 2024 00:20:24 +0100 Subject: [PATCH 06/16] 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 6162bccf1f89f3d68e50ba0ddc6567bdfda2be6d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 15:51:39 +0100 Subject: [PATCH 07/16] 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 3f30ec86cd11af9cf12f271f118460c9c13978a0 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 10 Apr 2024 00:44:40 +0100 Subject: [PATCH 08/16] 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 1ed199a881be99b3ce281418eee6d14c8c4b3ac7 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 18:44:04 +0100 Subject: [PATCH 09/16] tests: Use g_assert_*() rather than g_assert() in gdbus-export tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It won’t get compiled out with `G_DISABLE_ASSERT`. Signed-off-by: Philip Withnall --- gio/tests/gdbus-export.c | 254 +++++++++++++++++++-------------------- 1 file changed, 127 insertions(+), 127 deletions(-) diff --git a/gio/tests/gdbus-export.c b/gio/tests/gdbus-export.c index f7efe8c03..af88dc442 100644 --- a/gio/tests/gdbus-export.c +++ b/gio/tests/gdbus-export.c @@ -336,7 +336,7 @@ introspect_callback (GDBusProxy *proxy, res, &error); g_assert_no_error (error); - g_assert (result != NULL); + g_assert_nonnull (result); g_variant_get (result, "(s)", xml_data); g_variant_unref (result); @@ -381,7 +381,7 @@ get_nodes_at (GDBusConnection *c, NULL, &error); g_assert_no_error (error); - g_assert (proxy != NULL); + g_assert_nonnull (proxy); /* do this async to avoid libdbus-1 deadlocks */ xml_data = NULL; @@ -394,11 +394,11 @@ get_nodes_at (GDBusConnection *c, (GAsyncReadyCallback) introspect_callback, &xml_data); g_main_loop_run (loop); - g_assert (xml_data != NULL); + g_assert_nonnull (xml_data); node_info = g_dbus_node_info_new_for_xml (xml_data, &error); g_assert_no_error (error); - g_assert (node_info != NULL); + g_assert_nonnull (node_info); p = g_ptr_array_new (); for (n = 0; node_info->nodes != NULL && node_info->nodes[n] != NULL; n++) @@ -440,7 +440,7 @@ has_interface (GDBusConnection *c, NULL, &error); g_assert_no_error (error); - g_assert (proxy != NULL); + g_assert_nonnull (proxy); /* do this async to avoid libdbus-1 deadlocks */ xml_data = NULL; @@ -453,11 +453,11 @@ has_interface (GDBusConnection *c, (GAsyncReadyCallback) introspect_callback, &xml_data); g_main_loop_run (loop); - g_assert (xml_data != NULL); + g_assert_nonnull (xml_data); node_info = g_dbus_node_info_new_for_xml (xml_data, &error); g_assert_no_error (error); - g_assert (node_info != NULL); + g_assert_nonnull (node_info); ret = (g_dbus_node_info_lookup_interface (node_info, interface_name) != NULL); @@ -489,7 +489,7 @@ count_interfaces (GDBusConnection *c, NULL, &error); g_assert_no_error (error); - g_assert (proxy != NULL); + g_assert_nonnull (proxy); /* do this async to avoid libdbus-1 deadlocks */ xml_data = NULL; @@ -502,11 +502,11 @@ count_interfaces (GDBusConnection *c, (GAsyncReadyCallback) introspect_callback, &xml_data); g_main_loop_run (loop); - g_assert (xml_data != NULL); + g_assert_nonnull (xml_data); node_info = g_dbus_node_info_new_for_xml (xml_data, &error); g_assert_no_error (error); - g_assert (node_info != NULL); + g_assert_nonnull (node_info); ret = 0; while (node_info->interfaces != NULL && node_info->interfaces[ret] != NULL) @@ -532,7 +532,7 @@ dyna_create_callback (GDBusProxy *proxy, res, &error); g_assert_no_error (error); - g_assert (result != NULL); + g_assert_nonnull (result); g_variant_unref (result); g_main_loop_quit (loop); @@ -560,7 +560,7 @@ dyna_create (GDBusConnection *c, NULL, &error); g_assert_no_error (error); - g_assert (proxy != NULL); + g_assert_nonnull (proxy); /* do this async to avoid libdbus-1 deadlocks */ g_dbus_proxy_call (proxy, @@ -773,7 +773,7 @@ test_dispatch_thread_func (gpointer user_data) "org.example.Foo", NULL, &error); - g_assert (foo_proxy != NULL); + g_assert_nonnull (foo_proxy); /* generic interfaces */ error = NULL; @@ -785,7 +785,7 @@ test_dispatch_thread_func (gpointer user_data) NULL, &error); g_assert_no_error (error); - g_assert (value != NULL); + g_assert_nonnull (value); g_variant_unref (value); /* user methods */ @@ -798,8 +798,8 @@ test_dispatch_thread_func (gpointer user_data) NULL, &error); g_assert_no_error (error); - g_assert (value != NULL); - g_assert (g_variant_is_of_type (value, G_VARIANT_TYPE ("(s)"))); + g_assert_nonnull (value); + g_assert_true (g_variant_is_of_type (value, G_VARIANT_TYPE ("(s)"))); g_variant_get (value, "(&s)", &value_str); g_assert_cmpstr (value_str, ==, "You passed the string 'winwinwin'. Jolly good!"); g_variant_unref (value); @@ -815,7 +815,7 @@ test_dispatch_thread_func (gpointer user_data) g_assert_error (error, G_IO_ERROR, G_IO_ERROR_DBUS_ERROR); g_assert_cmpstr (error->message, ==, "GDBus.Error:org.example.SomeError: How do you like them apples, buddy!"); g_error_free (error); - g_assert (value == NULL); + g_assert_null (value); error = NULL; value = g_dbus_proxy_call_sync (foo_proxy, @@ -828,7 +828,7 @@ test_dispatch_thread_func (gpointer user_data) g_assert_error (error, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS); g_assert_cmpstr (error->message, ==, "GDBus.Error:org.freedesktop.DBus.Error.InvalidArgs: Type of message, “(s)”, does not match expected type “()”"); g_error_free (error); - g_assert (value == NULL); + g_assert_null (value); error = NULL; value = g_dbus_proxy_call_sync (foo_proxy, @@ -841,7 +841,7 @@ test_dispatch_thread_func (gpointer user_data) g_assert_error (error, G_DBUS_ERROR, G_DBUS_ERROR_UNKNOWN_METHOD); g_assert_cmpstr (error->message, ==, "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such method “NonExistantMethod”"); g_error_free (error); - g_assert (value == NULL); + g_assert_null (value); error = NULL; value = g_dbus_proxy_call_sync (foo_proxy, @@ -853,7 +853,7 @@ test_dispatch_thread_func (gpointer user_data) &error); g_assert_error (error, G_DBUS_ERROR, G_DBUS_ERROR_UNKNOWN_METHOD); g_error_free (error); - g_assert (value == NULL); + g_assert_null (value); /* user properties */ error = NULL; @@ -867,10 +867,10 @@ test_dispatch_thread_func (gpointer user_data) NULL, &error); g_assert_no_error (error); - g_assert (value != NULL); - g_assert (g_variant_is_of_type (value, G_VARIANT_TYPE ("(v)"))); + g_assert_nonnull (value); + g_assert_true (g_variant_is_of_type (value, G_VARIANT_TYPE ("(v)"))); g_variant_get (value, "(v)", &inner); - g_assert (g_variant_is_of_type (inner, G_VARIANT_TYPE_STRING)); + g_assert_true (g_variant_is_of_type (inner, G_VARIANT_TYPE_STRING)); g_assert_cmpstr (g_variant_get_string (inner, NULL), ==, "Property 'PropertyUno' Is What It Is!"); g_variant_unref (value); g_variant_unref (inner); @@ -885,7 +885,7 @@ test_dispatch_thread_func (gpointer user_data) -1, NULL, &error); - g_assert (value == NULL); + g_assert_null (value); g_assert_error (error, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS); g_assert_cmpstr (error->message, ==, "GDBus.Error:org.freedesktop.DBus.Error.InvalidArgs: No such property “ThisDoesntExist”"); g_error_free (error); @@ -900,7 +900,7 @@ test_dispatch_thread_func (gpointer user_data) -1, NULL, &error); - g_assert (value == NULL); + g_assert_null (value); g_assert_error (error, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS); g_assert_cmpstr (error->message, ==, "GDBus.Error:org.freedesktop.DBus.Error.InvalidArgs: Property “NotReadable” is not readable"); g_error_free (error); @@ -916,14 +916,14 @@ test_dispatch_thread_func (gpointer user_data) -1, NULL, &error); - g_assert (value == NULL); + g_assert_null (value); if (args->check_remote_errors) { /* _with_closures variant doesn't support customizing error data. */ g_assert_error (error, G_DBUS_ERROR, G_DBUS_ERROR_SPAWN_FILE_INVALID); g_assert_cmpstr (error->message, ==, "GDBus.Error:org.freedesktop.DBus.Error.Spawn.FileInvalid: Returning some error instead of writing the value ''But Writable you are!'' to the property 'NotReadable'"); } - g_assert (error != NULL && error->domain == G_DBUS_ERROR); + g_assert_true (error != NULL && error->domain == G_DBUS_ERROR); g_error_free (error); error = NULL; @@ -937,7 +937,7 @@ test_dispatch_thread_func (gpointer user_data) -1, NULL, &error); - g_assert (value == NULL); + g_assert_null (value); g_assert_error (error, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS); g_assert_cmpstr (error->message, ==, "GDBus.Error:org.freedesktop.DBus.Error.InvalidArgs: Property “NotWritable” is not writable"); g_error_free (error); @@ -952,8 +952,8 @@ test_dispatch_thread_func (gpointer user_data) NULL, &error); g_assert_no_error (error); - g_assert (value != NULL); - g_assert (g_variant_is_of_type (value, G_VARIANT_TYPE ("(a{sv})"))); + g_assert_nonnull (value); + g_assert_true (g_variant_is_of_type (value, G_VARIANT_TYPE ("(a{sv})"))); s = g_variant_print (value, TRUE); g_assert_cmpstr (s, ==, "({'PropertyUno': <\"Property 'PropertyUno' Is What It Is!\">, 'NotWritable': <\"Property 'NotWritable' Is What It Is!\">},)"); g_free (s); @@ -1015,7 +1015,7 @@ test_object_registration (void) error = NULL; c = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error); g_assert_no_error (error); - g_assert (c != NULL); + g_assert_nonnull (c); registration_id = g_dbus_connection_register_object (c, "/foo/boss", @@ -1025,7 +1025,7 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); boss_foo_reg_id = registration_id; num_successful_registrations++; @@ -1037,7 +1037,7 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); boss_bar_reg_id = registration_id; num_successful_registrations++; @@ -1049,7 +1049,7 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); worker1_foo_reg_id = registration_id; num_successful_registrations++; @@ -1061,7 +1061,7 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); worker1p1_foo_reg_id = registration_id; num_successful_registrations++; @@ -1073,7 +1073,7 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); worker2_bar_reg_id = registration_id; num_successful_registrations++; @@ -1085,7 +1085,7 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); intern1_foo_reg_id = registration_id; num_successful_registrations++; @@ -1098,7 +1098,7 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); intern2_bar_reg_id = registration_id; num_successful_registrations++; @@ -1112,10 +1112,10 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_error (error, G_IO_ERROR, G_IO_ERROR_EXISTS); - g_assert (!g_dbus_error_is_remote_error (error)); + g_assert_false (g_dbus_error_is_remote_error (error)); g_error_free (error); error = NULL; - g_assert (registration_id == 0); + g_assert_cmpuint (registration_id, ==, 0); g_assert_cmpint (data.num_unregistered_calls, ==, 1); num_failed_registrations++; @@ -1128,12 +1128,12 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); intern2_foo_reg_id = registration_id; num_successful_registrations++; /* unregister it via the id */ - g_assert (g_dbus_connection_unregister_object (c, registration_id)); + g_assert_true (g_dbus_connection_unregister_object (c, registration_id)); g_main_context_iteration (NULL, FALSE); g_assert_cmpint (data.num_unregistered_calls, ==, 2); intern2_foo_reg_id = 0; @@ -1147,7 +1147,7 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); intern2_foo_reg_id = registration_id; num_successful_registrations++; @@ -1159,7 +1159,7 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); intern3_bar_reg_id = registration_id; num_successful_registrations++; @@ -1172,7 +1172,7 @@ test_object_registration (void) on_subtree_unregistered, &error); g_assert_no_error (error); - g_assert (subtree_registration_id > 0); + g_assert_cmpuint (subtree_registration_id, >, 0); /* try registering it again.. this should fail */ registration_id = g_dbus_connection_register_subtree (c, "/foo/boss/executives", @@ -1182,14 +1182,14 @@ test_object_registration (void) on_subtree_unregistered, &error); g_assert_error (error, G_IO_ERROR, G_IO_ERROR_EXISTS); - g_assert (!g_dbus_error_is_remote_error (error)); + g_assert_false (g_dbus_error_is_remote_error (error)); g_error_free (error); error = NULL; - g_assert (registration_id == 0); + g_assert_cmpuint (registration_id, ==, 0); g_assert_cmpint (data.num_unregistered_subtree_calls, ==, 1); /* unregister it, then register it again */ - g_assert (g_dbus_connection_unregister_subtree (c, subtree_registration_id)); + g_assert_true (g_dbus_connection_unregister_subtree (c, subtree_registration_id)); g_main_context_iteration (NULL, FALSE); g_assert_cmpint (data.num_unregistered_subtree_calls, ==, 2); subtree_registration_id = g_dbus_connection_register_subtree (c, @@ -1200,7 +1200,7 @@ test_object_registration (void) on_subtree_unregistered, &error); g_assert_no_error (error); - g_assert (subtree_registration_id > 0); + g_assert_cmpuint (subtree_registration_id, >, 0); /* try to register something under /foo/boss/executives - this should work * because registered subtrees and registered objects can coexist. @@ -1216,7 +1216,7 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); non_subtree_object_path_bar_reg_id = registration_id; num_successful_registrations++; registration_id = g_dbus_connection_register_object (c, @@ -1227,7 +1227,7 @@ test_object_registration (void) on_object_unregistered, &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); non_subtree_object_path_foo_reg_id = registration_id; num_successful_registrations++; @@ -1241,11 +1241,11 @@ test_object_registration (void) (GDestroyNotify)g_ptr_array_unref, &error); g_assert_no_error (error); - g_assert (dyna_subtree_registration_id > 0); + g_assert_cmpuint (dyna_subtree_registration_id, >, 0); /* First assert that we have no nodes in the dynamic subtree */ nodes = get_nodes_at (c, "/foo/dyna"); - g_assert (nodes != NULL); + g_assert_nonnull (nodes); g_assert_cmpint (g_strv_length (nodes), ==, 0); g_strfreev (nodes); g_assert_cmpint (count_interfaces (c, "/foo/dyna"), ==, 4); @@ -1256,7 +1256,7 @@ test_object_registration (void) g_ptr_array_add (dyna_data, g_strdup ("cat")); g_ptr_array_add (dyna_data, g_strdup ("cheezburger")); nodes = get_nodes_at (c, "/foo/dyna"); - g_assert (nodes != NULL); + g_assert_nonnull (nodes); g_assert_cmpint (g_strv_length (nodes), ==, 3); g_assert_cmpstr (nodes[0], ==, "cat"); g_assert_cmpstr (nodes[1], ==, "cheezburger"); @@ -1269,7 +1269,7 @@ test_object_registration (void) /* Call a non-existing object path and assert that it has been created */ dyna_create (c, "dynamicallycreated"); nodes = get_nodes_at (c, "/foo/dyna"); - g_assert (nodes != NULL); + g_assert_nonnull (nodes); g_assert_cmpint (g_strv_length (nodes), ==, 4); g_assert_cmpstr (nodes[0], ==, "cat"); g_assert_cmpstr (nodes[1], ==, "cheezburger"); @@ -1282,14 +1282,14 @@ test_object_registration (void) * perverse that we round-trip to the bus to introspect ourselves ;-) */ nodes = get_nodes_at (c, "/"); - g_assert (nodes != NULL); + g_assert_nonnull (nodes); g_assert_cmpint (g_strv_length (nodes), ==, 1); g_assert_cmpstr (nodes[0], ==, "foo"); g_strfreev (nodes); g_assert_cmpint (count_interfaces (c, "/"), ==, 0); nodes = get_nodes_at (c, "/foo"); - g_assert (nodes != NULL); + g_assert_nonnull (nodes); g_assert_cmpint (g_strv_length (nodes), ==, 2); g_assert_cmpstr (nodes[0], ==, "boss"); g_assert_cmpstr (nodes[1], ==, "dyna"); @@ -1297,25 +1297,25 @@ test_object_registration (void) g_assert_cmpint (count_interfaces (c, "/foo"), ==, 0); nodes = get_nodes_at (c, "/foo/boss"); - g_assert (nodes != NULL); + g_assert_nonnull (nodes); g_assert_cmpint (g_strv_length (nodes), ==, 5); - g_assert (g_strv_contains ((const gchar* const *) nodes, "worker1")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "worker1p1")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "worker2")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "interns")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "executives")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "worker1")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "worker1p1")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "worker2")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "interns")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "executives")); g_strfreev (nodes); /* any registered object always implement org.freedesktop.DBus.[Peer,Introspectable,Properties] */ g_assert_cmpint (count_interfaces (c, "/foo/boss"), ==, 5); - g_assert (has_interface (c, "/foo/boss", foo_interface_info.name)); - g_assert (has_interface (c, "/foo/boss", bar_interface_info.name)); + g_assert_true (has_interface (c, "/foo/boss", foo_interface_info.name)); + g_assert_true (has_interface (c, "/foo/boss", bar_interface_info.name)); /* check subtree nodes - we should have only non_subtree_object in /foo/boss/executives * because data.num_subtree_nodes is 0 */ nodes = get_nodes_at (c, "/foo/boss/executives"); - g_assert (nodes != NULL); - g_assert (g_strv_contains ((const gchar* const *) nodes, "non_subtree_object")); + g_assert_nonnull (nodes); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "non_subtree_object")); g_assert_cmpint (g_strv_length (nodes), ==, 1); g_strfreev (nodes); g_assert_cmpint (count_interfaces (c, "/foo/boss/executives"), ==, 0); @@ -1323,41 +1323,41 @@ test_object_registration (void) /* now change data.num_subtree_nodes and check */ data.num_subtree_nodes = 2; nodes = get_nodes_at (c, "/foo/boss/executives"); - g_assert (nodes != NULL); + g_assert_nonnull (nodes); g_assert_cmpint (g_strv_length (nodes), ==, 5); - g_assert (g_strv_contains ((const gchar* const *) nodes, "non_subtree_object")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "vp0")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "vp1")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "evp0")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "evp1")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "non_subtree_object")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "vp0")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "vp1")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "evp0")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "evp1")); /* check that /foo/boss/executives/non_subtree_object is not handled by the * subtree handlers - we can do this because objects from subtree handlers * has exactly one interface and non_subtree_object has two */ g_assert_cmpint (count_interfaces (c, "/foo/boss/executives/non_subtree_object"), ==, 5); - g_assert (has_interface (c, "/foo/boss/executives/non_subtree_object", foo_interface_info.name)); - g_assert (has_interface (c, "/foo/boss/executives/non_subtree_object", bar_interface_info.name)); + g_assert_true (has_interface (c, "/foo/boss/executives/non_subtree_object", foo_interface_info.name)); + g_assert_true (has_interface (c, "/foo/boss/executives/non_subtree_object", bar_interface_info.name)); /* check that the vp and evp objects are handled by the subtree handlers */ g_assert_cmpint (count_interfaces (c, "/foo/boss/executives/vp0"), ==, 4); g_assert_cmpint (count_interfaces (c, "/foo/boss/executives/vp1"), ==, 4); g_assert_cmpint (count_interfaces (c, "/foo/boss/executives/evp0"), ==, 4); g_assert_cmpint (count_interfaces (c, "/foo/boss/executives/evp1"), ==, 4); - g_assert (has_interface (c, "/foo/boss/executives/vp0", foo_interface_info.name)); - g_assert (has_interface (c, "/foo/boss/executives/vp1", foo_interface_info.name)); - g_assert (has_interface (c, "/foo/boss/executives/evp0", bar_interface_info.name)); - g_assert (has_interface (c, "/foo/boss/executives/evp1", bar_interface_info.name)); + g_assert_true (has_interface (c, "/foo/boss/executives/vp0", foo_interface_info.name)); + g_assert_true (has_interface (c, "/foo/boss/executives/vp1", foo_interface_info.name)); + g_assert_true (has_interface (c, "/foo/boss/executives/evp0", bar_interface_info.name)); + g_assert_true (has_interface (c, "/foo/boss/executives/evp1", bar_interface_info.name)); g_strfreev (nodes); data.num_subtree_nodes = 3; nodes = get_nodes_at (c, "/foo/boss/executives"); - g_assert (nodes != NULL); + g_assert_nonnull (nodes); g_assert_cmpint (g_strv_length (nodes), ==, 7); - g_assert (g_strv_contains ((const gchar* const *) nodes, "non_subtree_object")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "vp0")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "vp1")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "vp2")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "evp0")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "evp1")); - g_assert (g_strv_contains ((const gchar* const *) nodes, "evp2")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "non_subtree_object")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "vp0")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "vp1")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "vp2")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "evp0")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "evp1")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "evp2")); g_strfreev (nodes); /* This is to check that a bug (rather, class of bugs) in gdbusconnection.c's @@ -1367,7 +1367,7 @@ test_object_registration (void) * where /foo/boss/worker1 reported a child '1', is now fixed. */ nodes = get_nodes_at (c, "/foo/boss/worker1"); - g_assert (nodes != NULL); + g_assert_nonnull (nodes); g_assert_cmpint (g_strv_length (nodes), ==, 0); g_strfreev (nodes); @@ -1388,26 +1388,26 @@ test_object_registration (void) /* check that unregistering the subtree handler works */ g_assert_cmpint (data.num_unregistered_subtree_calls, ==, 2); - g_assert (g_dbus_connection_unregister_subtree (c, subtree_registration_id)); + g_assert_true (g_dbus_connection_unregister_subtree (c, subtree_registration_id)); g_main_context_iteration (NULL, FALSE); g_assert_cmpint (data.num_unregistered_subtree_calls, ==, 3); nodes = get_nodes_at (c, "/foo/boss/executives"); - g_assert (nodes != NULL); + g_assert_nonnull (nodes); g_assert_cmpint (g_strv_length (nodes), ==, 1); - g_assert (g_strv_contains ((const gchar* const *) nodes, "non_subtree_object")); + g_assert_true (g_strv_contains ((const gchar* const *) nodes, "non_subtree_object")); g_strfreev (nodes); - g_assert (g_dbus_connection_unregister_object (c, boss_foo_reg_id)); - g_assert (g_dbus_connection_unregister_object (c, boss_bar_reg_id)); - g_assert (g_dbus_connection_unregister_object (c, worker1_foo_reg_id)); - g_assert (g_dbus_connection_unregister_object (c, worker1p1_foo_reg_id)); - g_assert (g_dbus_connection_unregister_object (c, worker2_bar_reg_id)); - g_assert (g_dbus_connection_unregister_object (c, intern1_foo_reg_id)); - g_assert (g_dbus_connection_unregister_object (c, intern2_bar_reg_id)); - g_assert (g_dbus_connection_unregister_object (c, intern2_foo_reg_id)); - g_assert (g_dbus_connection_unregister_object (c, intern3_bar_reg_id)); - g_assert (g_dbus_connection_unregister_object (c, non_subtree_object_path_bar_reg_id)); - g_assert (g_dbus_connection_unregister_object (c, non_subtree_object_path_foo_reg_id)); + g_assert_true (g_dbus_connection_unregister_object (c, boss_foo_reg_id)); + g_assert_true (g_dbus_connection_unregister_object (c, boss_bar_reg_id)); + g_assert_true (g_dbus_connection_unregister_object (c, worker1_foo_reg_id)); + g_assert_true (g_dbus_connection_unregister_object (c, worker1p1_foo_reg_id)); + g_assert_true (g_dbus_connection_unregister_object (c, worker2_bar_reg_id)); + g_assert_true (g_dbus_connection_unregister_object (c, intern1_foo_reg_id)); + g_assert_true (g_dbus_connection_unregister_object (c, intern2_bar_reg_id)); + g_assert_true (g_dbus_connection_unregister_object (c, intern2_foo_reg_id)); + g_assert_true (g_dbus_connection_unregister_object (c, intern3_bar_reg_id)); + g_assert_true (g_dbus_connection_unregister_object (c, non_subtree_object_path_bar_reg_id)); + g_assert_true (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 + num_failed_registrations); @@ -1417,7 +1417,7 @@ test_object_registration (void) */ #if 0 nodes = get_nodes_at (c, "/"); - g_assert (nodes != NULL); + g_assert_nonnull (nodes); g_assert_cmpint (g_strv_length (nodes), ==, 0); g_strfreev (nodes); #endif @@ -1434,7 +1434,7 @@ test_object_registration_with_closures (void) error = NULL; c = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error); g_assert_no_error (error); - g_assert (c != NULL); + g_assert_nonnull (c); registration_id = g_dbus_connection_register_object_with_closures (c, "/foo/boss", @@ -1444,11 +1444,11 @@ test_object_registration_with_closures (void) g_cclosure_new (G_CALLBACK (foo_set_property), NULL, NULL), &error); g_assert_no_error (error); - g_assert (registration_id > 0); + g_assert_cmpuint (registration_id, >, 0); test_dispatch ("/foo/boss", FALSE); - g_assert (g_dbus_connection_unregister_object (c, registration_id)); + g_assert_true (g_dbus_connection_unregister_object (c, registration_id)); g_object_unref (c); } @@ -1495,7 +1495,7 @@ check_interfaces (GDBusConnection *c, NULL, &error); g_assert_no_error (error); - g_assert (proxy != NULL); + g_assert_nonnull (proxy); /* do this async to avoid libdbus-1 deadlocks */ xml_data = NULL; @@ -1508,13 +1508,13 @@ check_interfaces (GDBusConnection *c, (GAsyncReadyCallback) introspect_callback, &xml_data); g_main_loop_run (loop); - g_assert (xml_data != NULL); + g_assert_nonnull (xml_data); node_info = g_dbus_node_info_new_for_xml (xml_data, &error); g_assert_no_error (error); - g_assert (node_info != NULL); + g_assert_nonnull (node_info); - g_assert (node_info->interfaces != NULL); + g_assert_nonnull (node_info->interfaces); for (i = 0; node_info->interfaces[i]; i++) ; #if 0 if (g_strv_length ((gchar**)interfaces) != i - 1) @@ -1563,7 +1563,7 @@ test_registered_interfaces (void) error = NULL; c = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error); g_assert_no_error (error); - g_assert (c != NULL); + g_assert_nonnull (c); id1 = g_dbus_connection_register_object (c, "/test", @@ -1573,7 +1573,7 @@ test_registered_interfaces (void) NULL, &error); g_assert_no_error (error); - g_assert (id1 > 0); + g_assert_cmpuint (id1, >, 0); id2 = g_dbus_connection_register_object (c, "/test", (GDBusInterfaceInfo *) &test_interface_info2, @@ -1582,12 +1582,12 @@ test_registered_interfaces (void) NULL, &error); g_assert_no_error (error); - g_assert (id2 > 0); + g_assert_cmpuint (id2, >, 0); check_interfaces (c, "/test", interfaces); - g_assert (g_dbus_connection_unregister_object (c, id1)); - g_assert (g_dbus_connection_unregister_object (c, id2)); + g_assert_true (g_dbus_connection_unregister_object (c, id1)); + g_assert_true (g_dbus_connection_unregister_object (c, id2)); g_object_unref (c); } @@ -1611,7 +1611,7 @@ test_async_method_call (GDBusConnection *connection, * but we don't do any during this testcase, so assert that. */ g_assert_cmpstr (interface_name, ==, "org.freedesktop.DBus.Properties"); - g_assert (g_dbus_method_invocation_get_method_info (invocation) == NULL); + g_assert_null (g_dbus_method_invocation_get_method_info (invocation)); property = g_dbus_method_invocation_get_property_info (invocation); @@ -1631,9 +1631,9 @@ test_async_method_call (GDBusConnection *connection, g_variant_get (parameters, "(&s&s)", &iface_name, &prop_name); g_assert_cmpstr (iface_name, ==, "org.example.Foo"); - g_assert (property != NULL); + g_assert_nonnull (property); g_assert_cmpstr (prop_name, ==, property->name); - g_assert (property->flags & G_DBUS_PROPERTY_INFO_FLAGS_READABLE); + g_assert_true (property->flags & G_DBUS_PROPERTY_INFO_FLAGS_READABLE); g_dbus_method_invocation_return_value (invocation, g_variant_new ("(v)", g_variant_new_string (prop_name))); } @@ -1644,10 +1644,10 @@ test_async_method_call (GDBusConnection *connection, g_variant_get (parameters, "(&s&sv)", &iface_name, &prop_name, &value); g_assert_cmpstr (iface_name, ==, "org.example.Foo"); - g_assert (property != NULL); + g_assert_nonnull (property); g_assert_cmpstr (prop_name, ==, property->name); - g_assert (property->flags & G_DBUS_PROPERTY_INFO_FLAGS_WRITABLE); - g_assert (g_variant_is_of_type (value, G_VARIANT_TYPE (property->signature))); + g_assert_true (property->flags & G_DBUS_PROPERTY_INFO_FLAGS_WRITABLE); + g_assert_true (g_variant_is_of_type (value, G_VARIANT_TYPE (property->signature))); g_dbus_method_invocation_return_value (invocation, g_variant_new ("()")); g_variant_unref (value); } @@ -1658,7 +1658,7 @@ test_async_method_call (GDBusConnection *connection, g_variant_get (parameters, "(&s)", &iface_name); g_assert_cmpstr (iface_name, ==, "org.example.Foo"); - g_assert (property == NULL); + g_assert_null (property); g_dbus_method_invocation_return_value (invocation, g_variant_new_parsed ("({ 'PropertyUno': < 'uno' >," " 'NotWritable': < 'notwrite' > },)")); @@ -1683,14 +1683,14 @@ ensure_result_cb (GObject *source, if (user_data == NULL) { /* Expected an error */ - g_assert (reply == NULL); + g_assert_null (reply); } else { /* Expected a reply of a particular format. */ gchar *str; - g_assert (reply != NULL); + g_assert_nonnull (reply); str = g_variant_print (reply, TRUE); g_assert_cmpstr (str, ==, (const gchar *) user_data); g_free (str); @@ -1733,20 +1733,20 @@ test_async_properties (void) c = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error); g_assert_no_error (error); - g_assert (c != NULL); + g_assert_nonnull (c); registration_id = g_dbus_connection_register_object (c, "/foo", (GDBusInterfaceInfo *) &foo_interface_info, &vtable, NULL, NULL, &error); g_assert_no_error (error); - g_assert (registration_id); + g_assert_cmpuint (registration_id, !=, 0); registration_id2 = g_dbus_connection_register_object (c, "/foo", (GDBusInterfaceInfo *) &foo2_interface_info, &vtable, NULL, NULL, &error); g_assert_no_error (error); - g_assert (registration_id); + g_assert_cmpuint (registration_id, !=, 0); test_async_case (c, NULL, "random", "()"); From ae3bd19108291563a12ddcca016f76ca9e813cf2 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 15:13:33 +0100 Subject: [PATCH 10/16] gresource: Improve resource unregistration performance slightly Rather than iterating over the list twice: once to find the resource, and once to re-find its link and delete it, just use `g_list_delete_link()` to delete what was found. This has the lovely side-effect of squashing a false positive from scan-build, which thought there was a use-after-free of `resource` in the caller, due to `g_resource_unref()` being called on it here. Signed-off-by: Philip Withnall Helps: #1767 --- gio/gresource.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/gio/gresource.c b/gio/gresource.c index 8ce00d49d..d19adf312 100644 --- a/gio/gresource.c +++ b/gio/gresource.c @@ -1029,14 +1029,16 @@ g_resources_register_unlocked (GResource *resource) static void g_resources_unregister_unlocked (GResource *resource) { - if (g_list_find (registered_resources, resource) == NULL) + GList *resource_link = g_list_find (registered_resources, resource); + + if (resource_link == NULL) { g_warning ("Tried to remove not registered resource"); } else { - registered_resources = g_list_remove (registered_resources, resource); - g_resource_unref (resource); + g_resource_unref (resource_link->data); + registered_resources = g_list_delete_link (registered_resources, resource_link); } } From ad0532f2bff97e02624bb995f7a5d86e07c6fc4a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 15:39:41 +0100 Subject: [PATCH 11/16] 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 3c6c60611f4279d2ab51db4d4e62c13f3ad425cb Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 15:52:31 +0100 Subject: [PATCH 12/16] girnode: Simplify NULL node handling All of the indications in the surrounding code are that `node` should never be `NULL`, but the error handling for it did actually allow it to be `NULL` iff its `parent` was also `NULL`. That made scan-build (kind of legitimately) warn about `NULL` pointer dereferences of `node`. Avoid that by unambiguously using an assertion to prevent `NULL` nodes. Signed-off-by: Philip Withnall Helps: #1767 --- girepository/girnode.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/girepository/girnode.c b/girepository/girnode.c index 3a0983014..efc022d72 100644 --- a/girepository/girnode.c +++ b/girepository/girnode.c @@ -594,8 +594,7 @@ gi_ir_node_get_full_size_internal (GIIrNode *parent, GList *l; size_t size, n; - if (node == NULL && parent != NULL) - g_error ("Caught NULL node, parent=%s", parent->name); + g_assert (node != NULL); g_debug ("node %p type '%s'", node, gi_ir_node_type_to_string (node->type)); From 96552fc9047bac079b08511b656348b5a4ccb30f Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 15:58:20 +0100 Subject: [PATCH 13/16] 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 4b7f6ffe4ced3ed351c05279e2608a7010910ddb Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 16:01:20 +0100 Subject: [PATCH 14/16] 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 0814be8befd7522cc5bf2ef299ef0ea6c4a20a76 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 19:27:34 +0100 Subject: [PATCH 15/16] 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) From c844abc75918aeae7761676d51a03e30d31d1a20 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 12 Apr 2024 19:35:05 +0100 Subject: [PATCH 16/16] ci: Force-enable -Dglib_debug for scan-build jobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several of the assertions in GLib (particularly on hot paths in `gobject.c`) are protected behind `#if G_ENABLE_DEBUG`. In order for scan-build to see them, the scan-build CI job needs to make sure that a debug build is definitely enabled — not just rely on it being implicitly enabled via the combination of other build options. Signed-off-by: Philip Withnall Helps: #1767 --- .gitlab-ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 96db39184..71d90536e 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -748,6 +748,7 @@ scan-build: --prefix=$HOME/glib-installed --localstatedir=/var --libdir=lib + -Dglib_debug=enabled -Dsystemtap=true -Ddtrace=true -Dinstalled_tests=true