From 6486495254ed3a02410cfde28954317350a43a92 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 22 Sep 2011 19:15:48 +0100 Subject: [PATCH] Fix two crashes when dbus_g_proxy_new_for_peer is used on a bus The first part of the bug is that when NameOwnerChanged is received with a dbus_g_proxy_new_for_peer (which has no name) alive, checking whether it was affected by the NameOwnerChanged caused a NULL dereference and segfault. The second part of the bug is that if the last proxy in existence is for a peer, when it was unregistered there would be no owner_match_rules, causing a crash. Both are exercised in the new test added here. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=41126 --- .gitignore | 1 + dbus/dbus-gproxy.c | 5 +- test/core/Makefile.am | 3 + test/core/peer-on-bus.c | 148 +++++++++++++++++++++++++++++++++++++++++++++++ test/core/run-test.sh | 1 + 5 files changed, 156 insertions(+), 2 deletions(-) create mode 100644 test/core/peer-on-bus.c diff --git a/.gitignore b/.gitignore index c568858..bfc16df 100644 --- a/.gitignore +++ b/.gitignore @@ -198,6 +198,7 @@ test/core/run-with-tmp-session-bus.conf test/core/peer-client test/core/peer-server test/core/test-dbus-glib +/test/core/test-peer-on-bus test/core/test-profile test/core/test-proxy-peer test/core/test-service-glib diff --git a/dbus/dbus-gproxy.c b/dbus/dbus-gproxy.c index ee5d2bd..23ef040 100644 --- a/dbus/dbus-gproxy.c +++ b/dbus/dbus-gproxy.c @@ -676,7 +676,7 @@ unassociate_proxies (gpointer key, gpointer val, gpointer user_data) manager = priv->manager; - if (!strcmp (priv->name, name)) + if (priv->name != NULL && !strcmp (priv->name, name)) { if (!priv->for_owner) { @@ -1139,7 +1139,8 @@ dbus_g_proxy_manager_unregister (DBusGProxyManager *manager, manager->proxy_lists = NULL; } - if (g_hash_table_size (manager->owner_match_rules) == 0) + if (manager->owner_match_rules != NULL && + g_hash_table_size (manager->owner_match_rules) == 0) { g_hash_table_destroy (manager->owner_match_rules); manager->owner_match_rules = NULL; diff --git a/test/core/Makefile.am b/test/core/Makefile.am index ef6cb26..65d7731 100644 --- a/test/core/Makefile.am +++ b/test/core/Makefile.am @@ -54,6 +54,7 @@ noinst_PROGRAMS = \ peer-client \ test-types \ test-30574 \ + test-peer-on-bus \ test-proxy-peer \ test-registrations \ test-variant-recursion \ @@ -133,6 +134,8 @@ test_types_SOURCES = \ test_gvariant_SOURCES = \ test-gvariant.c +test_peer_on_bus_SOURCES = peer-on-bus.c + CLEANFILES = \ $(BUILT_SOURCES) \ run-with-tmp-session-bus.conf diff --git a/test/core/peer-on-bus.c b/test/core/peer-on-bus.c new file mode 100644 index 0000000..7c5147d --- /dev/null +++ b/test/core/peer-on-bus.c @@ -0,0 +1,148 @@ +/* Regression test for object registration and unregistration + * + * Copyright © 2009 Collabora Ltd. + * Copyright © 2009-2011 Nokia Corporation + * + * In preparation for dbus-glib relicensing (if it ever happens), this file is + * licensed under (at your option) either the AFL v2.1, the GPL v2 or later, + * or an MIT/X11-style license: + * + * Licensed under the Academic Free License version 2.1 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * Permission is hereby granted, free of charge, to any person + * obtaining a copy of this software and associated documentation + * files (the "Software"), to deal in the Software without + * restriction, including without limitation the rights to use, copy, + * modify, merge, publish, distribute, sublicense, and/or sell copies + * of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + */ + +#include + +#include +#include + +#include "test/lib/util.h" + +GMainLoop *loop = NULL; + +typedef struct { + DBusGConnection *bus; + DBusGConnection *bus2; +} Fixture; + +static void +setup (Fixture *f, + gconstpointer path_to_use) +{ + f->bus = dbus_g_bus_get_private (DBUS_BUS_SESSION, NULL, NULL); + g_assert (f->bus != NULL); + + f->bus2 = dbus_g_bus_get_private (DBUS_BUS_SESSION, NULL, NULL); + g_assert (f->bus2 != NULL); +} + +static void +teardown (Fixture *f, + gconstpointer test_data G_GNUC_UNUSED) +{ + if (f->bus != NULL) + { + test_run_until_disconnected (f->bus, NULL); + dbus_g_connection_unref (f->bus); + } + + if (f->bus2 != NULL) + { + test_run_until_disconnected (f->bus2, NULL); + dbus_g_connection_unref (f->bus2); + } + + dbus_shutdown (); +} + +static void +destroy_cb (DBusGProxy *proxy G_GNUC_UNUSED, + gpointer user_data) +{ + gboolean *destroyed = user_data; + + *destroyed = TRUE; +} + +static void +test_name_owner_changed (Fixture *f, + gconstpointer test_data G_GNUC_UNUSED) +{ + DBusGProxy *peer; + DBusGProxy *named; + gboolean destroyed = FALSE; + + g_test_bug ("41126"); + + /* bus has a proxy for bus2... */ + named = dbus_g_proxy_new_for_name (f->bus, + dbus_bus_get_unique_name (dbus_g_connection_get_connection (f->bus2)), + "/", "org.freedesktop.DBus.Peer"); + /* ... and also a proxy for the peer (i.e. the dbus-daemon) */ + peer = dbus_g_proxy_new_for_peer (f->bus, "/", "org.freedesktop.DBus.Peer"); + + g_signal_connect (G_OBJECT (named), "destroy", G_CALLBACK (destroy_cb), + &destroyed); + + /* Disconnect bus2, to provoke a NameOwnerChanged signal on bus */ + test_run_until_disconnected (f->bus2, NULL); + dbus_g_connection_unref (f->bus2); + f->bus2 = NULL; + + /* Wait for that NameOwnerChanged to be processed */ + while (!destroyed) + g_main_context_iteration (NULL, TRUE); + + g_signal_handlers_disconnect_by_func (named, destroy_cb, &destroyed); + + /* The first part of the bug was that we'd never get here, because checking + * whether 'peer' was affected by the NameOwnerChanged caused a NULL + * dereference and segfault. If we get here, all is OK. + * + * The second part of the bug was that if the last proxy in existence was + * for a peer, when it was unregistered there would be no owner_match_rules, + * causing a crash. Unref named before peer, to exercise that. */ + + g_object_unref (named); + g_object_unref (peer); +} + +int +main (int argc, char **argv) +{ + g_setenv ("DBUS_FATAL_WARNINGS", "1", TRUE); + g_type_init (); + g_log_set_always_fatal (G_LOG_LEVEL_WARNING | G_LOG_LEVEL_CRITICAL); + dbus_g_type_specialized_init (); + g_test_bug_base ("https://bugs.freedesktop.org/show_bug.cgi?id="); + g_test_init (&argc, &argv, NULL); + + g_test_add ("/peer-on-bus/name-owner-changed", Fixture, NULL, + setup, test_name_owner_changed, teardown); + + return g_test_run (); +} diff --git a/test/core/run-test.sh b/test/core/run-test.sh index 09e28b1..a61372b 100755 --- a/test/core/run-test.sh +++ b/test/core/run-test.sh @@ -49,4 +49,5 @@ else ${DBUS_TOP_BUILDDIR}/libtool --mode=execute $DEBUG $DBUS_TOP_BUILDDIR/test/core/test-variant-recursion || die "test-variant-recursion failed" ${DBUS_TOP_BUILDDIR}/libtool --mode=execute $DEBUG $DBUS_TOP_BUILDDIR/test/core/test-gvariant || die "test-gvariant failed" ${DBUS_TOP_BUILDDIR}/libtool --mode=execute $DEBUG $DBUS_TOP_BUILDDIR/test/core/test-30574 || die "test-30574 failed" + ${DBUS_TOP_BUILDDIR}/libtool --mode=execute $DEBUG $DBUS_TOP_BUILDDIR/test/core/test-peer-on-bus || die "test-peer-on-bus failed" fi -- 1.7.6.3