pidgin: 964025d6: We really shouldn't be doing a whole lot...

[markdoliner at pidgin.im]

-----------------------------------------------------------------
Revision: 964025d624cbb6d302e3b5f6af14fa60f14bf80f
Ancestor: 72b2f9e235c65523ac3455dc61a8d23f78c14f8a
Author: markdoliner at pidgin.im
Date: 2009-11-04T22:47:18
Branch: im.pidgin.pidgin
URL: http://d.pidgin.im/viewmtn/revision/info/964025d624cbb6d302e3b5f6af14fa60f14bf80f

Modified files:
        pidgin/gtkmain.c

ChangeLog: 

We really shouldn't be doing a whole lot in our signal handler.  The
signal handler can get called in the middle of a malloc, for example,
and if the signal handler tries to allocate memory then the program
can deadlock.

This change works around that problem by having the signal handler
write to a pipe.  Our main program watches the pipe for incoming data
and performs the appropriate action.

This patch is from Shaun Lindsay at Meebo.

Fixes #10324

-------------- next part --------------
============================================================
--- pidgin/gtkmain.c	2095555d2c6d6b5ddf499ef618157af1c49c4e70
+++ pidgin/gtkmain.c	1e442fcffaf22c9766dce90b5650f17a660093aa
@@ -143,6 +143,10 @@ dologin_named(const char *name)
 }
 
 #ifdef HAVE_SIGNAL_H
+static char *segfault_message;
+
+static int signal_sockets[2];
+
 static void sighandler(int sig);
 
 /*
@@ -168,31 +172,60 @@ clean_pid(void)
 	}
 }
 
-char *segfault_message;
+static void sighandler(int sig)
+{
+	ssize_t written;
 
-/*
- * This signal handler shouldn't be touching this much stuff.
- * It should just set a flag and return, and something else in
- * Pidgin should monitor the flag to see if something needs to
- * be done.  Because the signal handler interrupts the program,
- * it could be called in the middle of adding a new connection
- * to the list of connections, and then if we try to disconnect
- * all connections it could lead to a crash because the linked
- * list of connections could be in a weird state.  But, well,
- * this signal handler probably isn't called very often, so it's
- * not a big deal.
- */
-static void
-sighandler(int sig)
+	/*
+	 * We won't do any of the heavy lifting for the signal handling here
+	 * because we have no idea what was interrupted.  Previously this signal
+	 * handler could result in some calls to malloc/free, which can cause
+	 * deadlock in libc when the signal handler was interrupting a previous
+	 * malloc or free.  So instead we'll do an ugly hack where we write the
+	 * signal number to one end of a socket pair.  The other half of the
+	 * socket pair is watched by our main loop.  When the main loop sees new
+	 * data on the socket it reads in the signal and performs the appropriate
+	 * action without fear of interrupting stuff.
+	 */
+	if (sig == SIGSEGV) {
+		fprintf(stderr, "%s", segfault_message);
+		abort();
+		return;
+	}
+
+	written = write(signal_sockets[0], &sig, sizeof(int));
+	if (written < 0 || written != sizeof(int)) {
+		/* This should never happen */
+		purple_debug_error("sighandler", "Received signal %d but only "
+				"wrote %" G_GSSIZE_FORMAT " bytes out of %"
+				G_GSIZE_FORMAT ": %s\n",
+				sig, written, sizeof(int), g_strerror(errno));
+		exit(1);
+	}
+}
+
+static gboolean
+mainloop_sighandler(GIOChannel *source, GIOCondition cond, gpointer data)
 {
+	GIOStatus stat;
+	int sig;
+	gsize bytes_read;
+	GError *error = NULL;
+
+	/* read the signal number off of the io channel */
+	stat = g_io_channel_read_chars(source, (gchar *)&sig, sizeof(int),
+			&bytes_read, &error);
+	if (stat != G_IO_STATUS_NORMAL) {
+		purple_debug_error("sighandler", "Signal callback failed to read "
+				"from signal socket: %s", error->message);
+		purple_core_quit();
+		return FALSE;
+	}
+
 	switch (sig) {
 	case SIGHUP:
 		purple_debug_warning("sighandler", "Caught signal %d\n", sig);
 		break;
-	case SIGSEGV:
-		fprintf(stderr, "%s", segfault_message);
-		abort();
-		break;
 #if defined(USE_GSTREAMER) && !defined(GST_CAN_DISABLE_FORKING)
 /* By default, gstreamer forks when you initialize it, and waitpids for the
  * child.  But if libpurple reaps the child rather than leaving it to
@@ -219,6 +252,8 @@ sighandler(int sig)
 		purple_debug_warning("sighandler", "Caught signal %d\n", sig);
 		purple_core_quit();
 	}
+
+	return TRUE;
 }
 #endif
 
@@ -502,6 +537,8 @@ int main(int argc, char *argv[])
 	sigset_t sigset;
 	RETSIGTYPE (*prev_sig_disp)(int);
 	char errmsg[BUFSIZ];
+	GIOChannel *signal_channel;
+	GIOStatus signal_status;
 #ifndef DEBUG
 	char *segfault_message_tmp;
 	GError *error = NULL;
@@ -592,6 +629,29 @@ int main(int argc, char *argv[])
 		);
 #endif
 
+	/*
+	 * Create a socket pair for receiving unix signals from a signal
+	 * handler.
+	 */
+	if (socketpair(AF_UNIX, SOCK_STREAM, 0, signal_sockets) < 0) {
+		perror("Failed to create sockets for GLib signal handling");
+		exit(1);
+	}
+	signal_channel = g_io_channel_unix_new(signal_sockets[1]);
+
+	/*
+	 * Set the channel encoding to raw binary instead of the default of
+	 * UTF-8, because we'll be sending integers across instead of strings.
+	 */
+	error = NULL;
+	signal_status = g_io_channel_set_encoding(signal_channel, NULL, &error);
+	if (signal_status != G_IO_STATUS_NORMAL) {
+		fprintf(stderr, "Failed to set the signal channel to raw "
+				"binary: %s", error->message);
+		exit(1);
+	}
+	g_io_add_watch(signal_channel, G_IO_IN, mainloop_sighandler, NULL);
+
 	/* Let's not violate any PLA's!!!! */
 	/* jseymour: whatever the fsck that means */
 	/* Robot101: for some reason things like gdm like to block     *