Re: [evolution-patches] [PATCH] prevent evolution from crashing if new-mail-notification is enabled but D-BUS session daemon is not running

[Timo Hoenig <thoenig nouse net>]

Hi,

On Thu, 2005-04-07 at 09:33 +0800, Not Zed wrote:
[...]

> Where exactly is it crashing anyway?  Is it the plugin dereferncing a
> null pointer, or is it somewhere inside of the dbus library?

Even if the plugin fails to get a connection to the D-BUS session bus it
still calls dbus_connection_setup_with_g_main() to set up the connection
with the GLib loop.

It also tries to send messages to the (not established) connection.
Both calls cause Evolution to crash, they happen in libdbus-glib-1 and
libdbus-1 as far as I can tell.

> It seems rather redundant to have to have all these extra checks, if
> it is just a code bug.

I've been debugging and noticed that there is no need to associate the
D-BUS connection with the GLib loop.  The plugin does not receive
anything from D-BUS.  If the D-BUS connection is set up with the GLib
main loop and the D-BUS session bus is killed later on, Evolution will
crash.

Miguel, please tell me if I miss the intention for setting up the
connection with the GLib loop.

About the redundancy of the checks:

Case 1: Evolution starts with a sane D-BUS environment.  The plugin
successfully starts and sends notifications to the session bus.  Once
the session bus dies,  send_dbus_message()  has to return before trying
to send something to D-BUS.

Case 2: Evolution starts without a D-BUS session bus running or a broken
environment.  The plugin should fail to load (should be disabled) since
there is no chance that  send_dbus_message()  will get a connection to
the session bus -- even if a session bus gets invoked at a later time.

> > CC'ing Miguel since I have touched his code (renamed the variable for
> > the D-BUS connection from 'bus' to 'connection'). 
> 
> Why?  This change seems totally redundant.  Its just more typing too.

Since the variable represents a connection to the session bus rather
than the bus itself.  If it is to long, what about 'con'?

Thanks,

   Timo

Index: plugins/new-mail-notify/ChangeLog
===================================================================
RCS file: /cvs/gnome/evolution/plugins/new-mail-notify/ChangeLog,v
retrieving revision 1.6
diff -u -r1.6 ChangeLog
--- plugins/new-mail-notify/ChangeLog	5 Apr 2005 07:28:15 -0000	1.6
+++ plugins/new-mail-notify/ChangeLog	7 Apr 2005 09:29:49 -0000
@@ -1,3 +1,21 @@
+2005-04-07  Timo Hoenig  <thoenig novell com>
+	* new-mail-notify.c (send_dbus_message): connection to the D-BUS
+	session bus is no longer set up with the GLib main loop.
+
+2005-04-06  Timo Hoenig  <thoenig novell com>
+	* new-mail-notify.c (send_dbus_message): renamed D-BUS connection
+	from DBusConnection *bus to DBusConnection *connection
+	* new-mail-notify.c (e_plugin_lib_enable): check if the D-BUS
+	session bus is running is now determined using the D-BUS API
+
+2005-04-06  Timo Hoenig  <thoenig novell com>
+	* new-mail-notify.c (send_dbus_message): added two checks to prevent
+	Evolution to crash if the D-BUS session bus is not running or if
+	D-BUS is not able to allocate memory for the message
+	* new-mail-notify.c (e_plugin_lib_enable): refuse to load the plugin
+	if the address of the D-BUS session bus can not be determined
+	* new-mail-notify.c (e_plugin_lib_enable): new function
+
 2005-03-11  David Malcolm  <dmalcolm redhat com>
 
 	* new-mail-notify.c: preprocessor hackery using the value of
Index: plugins/new-mail-notify/new-mail-notify.c
===================================================================
RCS file: /cvs/gnome/evolution/plugins/new-mail-notify/new-mail-notify.c,v
retrieving revision 1.4
diff -u -r1.4 new-mail-notify.c
--- plugins/new-mail-notify/new-mail-notify.c	5 Apr 2005 07:28:15 -0000	1.4
+++ plugins/new-mail-notify/new-mail-notify.c	7 Apr 2005 09:29:49 -0000
@@ -1,6 +1,7 @@
 /* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */
 /*
- *  Author: Miguel Angel Lopez Hernandez <miguel gulev org mx>
+ *  Authors: Miguel Angel Lopez Hernandez <miguel gulev org mx>
+ *           Timo Hoenig <thoenig novell com>
  *
  *  Copyright 2004 Novell, Inc.
  *
@@ -42,6 +43,7 @@
 GtkWidget *org_gnome_new_mail_config (EPlugin *ep, EConfigHookItemFactoryData *hook_data);
 void org_gnome_new_mail_notify (EPlugin *ep, EMEventTargetFolder *t);
 void org_gnome_message_reading_notify (EPlugin *ep, EMEventTargetMessage *t);
+int  e_plugin_lib_enable(EPluginLib *ep, int enable);
 
 static void
 toggled_cb (GtkWidget *widget, EConfig *config)
@@ -89,28 +91,32 @@
 	GConfClient *client = gconf_client_get_default ();
 
 	if (gconf_client_get_bool(client, GCONF_KEY, NULL)) {
-		DBusConnection *bus;
-		DBusError error;
-		DBusMessage *message;
+		DBusConnection *connection;
+		DBusError      error;
+		DBusMessage    *message;
 
 		/* Get a connection to the session bus */
 		dbus_error_init (&error);
-		bus = dbus_bus_get (DBUS_BUS_SESSION,
+		connection = dbus_bus_get (DBUS_BUS_SESSION,
 				    &error);
 
-		if (!bus) {
+		if (!connection) {
 			printf ("Failed to connect to the D-BUS daemon: %s\n", error.message);
 			dbus_error_free (&error);
+			g_object_unref (client);
+			return;
 		}
 
-		/* Set up this connection to work in a GLib event loop  */
-		dbus_connection_setup_with_g_main (bus, NULL);
-
 		/* Create a new message on the DBUS_INTERFACE */
 		message = dbus_message_new_signal (DBUS_PATH,
 						   DBUS_INTERFACE,
 						   message_name);
 
+		if (message == NULL) {
+			g_object_unref (client);
+			return;
+		}
+
 		/* Appends the data as an argument to the message */
 		dbus_message_append_args (message,
 #if DBUS_VERSION >= 310
@@ -121,7 +127,7 @@
 					  DBUS_TYPE_INVALID);
 
 		/* Sends the message */
-		dbus_connection_send (bus,
+		dbus_connection_send (connection,
 				      message,
 				      NULL);
 
@@ -145,3 +151,24 @@
 {
 	send_dbus_message ("Newmail", t->uri);
 }
+
+int
+e_plugin_lib_enable (EPluginLib *ep, int enable)
+{
+	if (enable) {
+		DBusConnection *connection;
+		DBusError      error;
+
+		dbus_error_init (&error);
+		connection = dbus_bus_get (DBUS_BUS_SESSION, &error);
+		if (!connection) {
+			/* Could not determine address of the D-BUS session bus */
+			/* Plugin will be disabled */
+			dbus_error_free (&error);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+