[Patch] Fix a crash in vpn starters

[Tambet Ingo <tambet ximian com>]

Hey,

Attached is a patch to fix a race condition when starting vpn sessions
and something goes wrong early on. Instead of returning an error
message, the vpn helpers emit a dbus signal, which might (and sometimes
does, as described at
https://bugzilla.novell.com/show_bug.cgi?id=214950) arrive to NM before
the reply to the startConnection() call. When the reply finally does
arrive, stuff is already freed and in inconsistent state and bad things
happen (again, as described at that bugzilla link).

I'd like to apply the patch to both branches. Additionally, for HEAD.
I'd also remove all mentions of unused VPN signals which this patch
removes (stable API jadda-jadda in 0.6).

Tambet

Index: include/NetworkManagerVPN.h
===================================================================
RCS file: /cvs/gnome/NetworkManager/include/NetworkManagerVPN.h,v
retrieving revision 1.1
diff -u -r1.1 NetworkManagerVPN.h
--- include/NetworkManagerVPN.h	6 Dec 2005 23:36:26 -0000	1.1
+++ include/NetworkManagerVPN.h	27 Oct 2006 08:43:08 -0000
@@ -42,6 +42,7 @@
 #define NM_DBUS_VPN_ALREADY_STOPPED		"AlreadyStopped"
 #define NM_DBUS_VPN_WRONG_STATE			"WrongState"
 #define NM_DBUS_VPN_BAD_ARGUMENTS			"BadArguments"
+#define NM_DBUS_VPN_LAUNCH_FAILED			"LaunchFailed"
 
 
 /*
Index: vpn-daemons/openvpn/src/nm-openvpn-service.c
===================================================================
RCS file: /cvs/gnome/NetworkManager/vpn-daemons/openvpn/src/nm-openvpn-service.c,v
retrieving revision 1.10.2.4
diff -u -r1.10.2.4 nm-openvpn-service.c
--- vpn-daemons/openvpn/src/nm-openvpn-service.c	27 Sep 2006 15:10:26 -0000	1.10.2.4
+++ vpn-daemons/openvpn/src/nm-openvpn-service.c	27 Oct 2006 08:43:08 -0000
@@ -1009,7 +1009,7 @@
  * Parse message arguments and start the VPN connection.
  *
  */
-static gboolean
+static DBusMessage *
 nm_openvpn_dbus_handle_start_vpn (DBusMessage *message, NmOpenVPNData *data)
 {
   char **		data_items = NULL;
@@ -1022,10 +1022,11 @@
   const char *	user_name = NULL;
   DBusError		error;
   gboolean		success = FALSE;
-  gint			openvpn_fd = -1;	
+  gint			openvpn_fd = -1;
+  DBusMessage *	reply = NULL;
 
-  g_return_val_if_fail (message != NULL, FALSE);
-  g_return_val_if_fail (data != NULL, FALSE);
+  g_return_val_if_fail (message != NULL, NULL);
+  g_return_val_if_fail (data != NULL, NULL);
 
   nm_openvpn_set_state (data, NM_VPN_STATE_STARTING);
 
@@ -1039,22 +1040,32 @@
 			      DBUS_TYPE_INVALID))
     {
       nm_warning ("Could not process the request because its arguments were invalid.  dbus said: '%s'", error.message);
-      nm_openvpn_dbus_signal_failure (data, NM_DBUS_VPN_SIGNAL_VPN_CONFIG_BAD);
+	  reply = nm_dbus_create_error_message (message,
+											NM_DBUS_INTERFACE_OPENVPN,
+											"InvalidArguments",
+											"Invalid method arguments.");
       dbus_error_free (&error);
       goto out;
     }
 
   if (!nm_openvpn_config_options_validate (data_items, num_items))
     {
-      nm_openvpn_dbus_signal_failure (data, NM_DBUS_VPN_SIGNAL_VPN_CONFIG_BAD);
+	  reply = nm_dbus_create_error_message (message, NM_DBUS_INTERFACE_OPENVPN, NM_DBUS_VPN_BAD_ARGUMENTS,
+											"Invalid VPN options.");
       goto out;
     }
 
   /* Now we can finally try to activate the VPN */
-  if ((openvpn_fd = nm_openvpn_start_openvpn_binary (data, data_items, num_items, password_items, num_passwords)) >= 0) {
-    // Everything ok
-    success = TRUE;
-  }
+  if ((openvpn_fd = nm_openvpn_start_openvpn_binary (data, data_items, num_items, password_items, num_passwords)) < 0)
+    {
+	  reply = nm_dbus_create_error_message (message, NM_DBUS_INTERFACE_OPENVPN, NM_DBUS_VPN_LAUNCH_FAILED,
+											"Failed to run openvpn binary.");
+	  goto out;
+	}
+
+  // Everything ok
+  reply = dbus_message_new_method_return (message);
+  success = TRUE;
 
   
 out:
@@ -1063,7 +1074,8 @@
   dbus_free_string_array (user_routes);
   if (!success)
     nm_openvpn_set_state (data, NM_VPN_STATE_STOPPED);
-  return success;
+
+  return reply;
 }
 
 
@@ -1127,8 +1139,7 @@
 
     case NM_VPN_STATE_STOPPED:
       nm_openvpn_cancel_quit_timer (data);
-      nm_openvpn_dbus_handle_start_vpn (message, data);
-      reply = dbus_message_new_method_return (message);
+      reply = nm_openvpn_dbus_handle_start_vpn (message, data);
       break;
 
     default:
Index: vpn-daemons/pptp/src/nm-pptp-service.c
===================================================================
RCS file: /cvs/gnome/NetworkManager/vpn-daemons/pptp/src/Attic/nm-pptp-service.c,v
retrieving revision 1.4.2.2
diff -u -r1.4.2.2 nm-pptp-service.c
--- vpn-daemons/pptp/src/nm-pptp-service.c	27 Sep 2006 15:10:28 -0000	1.4.2.2
+++ vpn-daemons/pptp/src/nm-pptp-service.c	27 Oct 2006 08:43:08 -0000
@@ -590,7 +590,8 @@
  * Parse message arguments and start the VPN connection.
  *
  */
-static gboolean nm_pptp_dbus_handle_start_vpn (DBusMessage *message, NmPPTPData *data)
+static DBusMessage *
+nm_pptp_dbus_handle_start_vpn (DBusMessage *message, NmPPTPData *data)
 {
   char **		data_items = NULL;
   int		num_items = -1;
@@ -602,10 +603,11 @@
   const char *	user_name = NULL;
   DBusError		error;
   gboolean		success = FALSE;
-  gint			pptp_fd = -1;	
+  gint			pptp_fd = -1;
+  DBusMessage *	reply = NULL;
 
-  g_return_val_if_fail (message != NULL, FALSE);
-  g_return_val_if_fail (data != NULL, FALSE);
+  g_return_val_if_fail (message != NULL, NULL);
+  g_return_val_if_fail (data != NULL, NULL);
 
   nm_pptp_set_state (data, NM_VPN_STATE_STARTING);
 
@@ -619,30 +621,40 @@
 			      DBUS_TYPE_INVALID))
     {
       nm_warning ("Could not process the request because its arguments were invalid.  dbus said: '%s'", error.message);
-      nm_pptp_dbus_signal_failure (data, NM_DBUS_VPN_SIGNAL_VPN_CONFIG_BAD);
+	  reply = nm_dbus_create_error_message (message,
+											NM_DBUS_INTERFACE_VPN,
+											"InvalidArguments",
+											"Invalid method arguments.");
       dbus_error_free (&error);
       goto out;
     }
 
   if (!nm_pptp_config_options_validate (data_items, num_items))
     {
-      nm_pptp_dbus_signal_failure (data, NM_DBUS_VPN_SIGNAL_VPN_CONFIG_BAD);
+      reply = nm_dbus_create_error_message (message, NM_DBUS_INTERFACE_PPTP, NM_DBUS_VPN_BAD_ARGUMENTS,
+											"Invalid VPN options.");
       goto out;
     }
 
   /* Now we can finally try to activate the VPN */
-  if ((pptp_fd = nm_pptp_start_pptp_binary (data, data_items, num_items)) >= 0)
+  if ((pptp_fd = nm_pptp_start_pptp_binary (data, data_items, num_items)) < 0)
     {
-      success = TRUE;
+	  reply = nm_dbus_create_error_message (message, NM_DBUS_INTERFACE_PPTP, NM_DBUS_VPN_LAUNCH_FAILED,
+											"Failed to run vpnc binary.");
+	  goto out;
     }
 
+  reply = dbus_message_new_method_return (message);
+  success = TRUE;
+
 out:
   dbus_free_string_array (data_items);
   dbus_free_string_array (password_items);
   dbus_free_string_array (user_routes);
   if (!success)
     nm_pptp_set_state (data, NM_VPN_STATE_STOPPED);
-  return success;
+
+  return reply;
 }
 
 
@@ -705,8 +717,7 @@
 
     case NM_VPN_STATE_STOPPED:
       nm_pptp_cancel_quit_timer (data);
-      nm_pptp_dbus_handle_start_vpn (message, data);
-      reply = dbus_message_new_method_return (message);
+      reply = nm_pptp_dbus_handle_start_vpn (message, data);
       break;
 
     default:
Index: vpn-daemons/vpnc/src/nm-vpnc-service.c
===================================================================
RCS file: /cvs/gnome/NetworkManager/vpn-daemons/vpnc/src/nm-vpnc-service.c,v
retrieving revision 1.16.2.3
diff -u -r1.16.2.3 nm-vpnc-service.c
--- vpn-daemons/vpnc/src/nm-vpnc-service.c	27 Sep 2006 15:10:37 -0000	1.16.2.3
+++ vpn-daemons/vpnc/src/nm-vpnc-service.c	27 Oct 2006 08:43:09 -0000
@@ -582,7 +582,8 @@
  * Parse message arguments and start the VPN connection.
  *
  */
-static gboolean nm_vpnc_dbus_handle_start_vpn (DBusMessage *message, NmVpncData *data)
+static DBusMessage *
+nm_vpnc_dbus_handle_start_vpn (DBusMessage *message, NmVpncData *data)
 {
 	char **		data_items = NULL;
 	int			num_items = -1;
@@ -594,10 +595,11 @@
 	const char *	user_name = NULL;
 	DBusError		error;
 	gboolean		success = FALSE;
-	gint			vpnc_fd = -1;	
+	gint			vpnc_fd = -1;
+	DBusMessage *	reply = NULL;
 
-	g_return_val_if_fail (message != NULL, FALSE);
-	g_return_val_if_fail (data != NULL, FALSE);
+	g_return_val_if_fail (message != NULL, NULL);
+	g_return_val_if_fail (data != NULL, NULL);
 
 	nm_vpnc_set_state (data, NM_VPN_STATE_STARTING);
 
@@ -611,34 +613,51 @@
 				    DBUS_TYPE_INVALID))
 	{
 		nm_warning ("Could not process the request because its arguments were invalid.  dbus said: '%s'", error.message);
-		nm_vpnc_dbus_signal_failure (data, NM_DBUS_VPN_SIGNAL_VPN_CONFIG_BAD);
+		reply = nm_dbus_create_error_message (message,
+											  NM_DBUS_INTERFACE_VPN,
+											  "InvalidArguments",
+											  "Invalid method arguments.");
 		dbus_error_free (&error);
 		goto out;
 	}
 
 	if (!nm_vpnc_config_options_validate (data_items, num_items))
 	{
-		nm_vpnc_dbus_signal_failure (data, NM_DBUS_VPN_SIGNAL_VPN_CONFIG_BAD);
+		reply = nm_dbus_create_error_message (message, NM_DBUS_INTERFACE_VPNC, NM_DBUS_VPN_BAD_ARGUMENTS,
+											  "Invalid VPN options.");
 		goto out;
 	}
 
 	/* Now we can finally try to activate the VPN */
-	if ((vpnc_fd = nm_vpnc_start_vpnc_binary (data)) >= 0)
+	if ((vpnc_fd = nm_vpnc_start_vpnc_binary (data)) < 0)
 	{
-		if (nm_vpnc_config_write (vpnc_fd, user_name, password_items, num_passwords, data_items, num_items))
-			success = TRUE;
-		else
-			nm_vpnc_dbus_signal_failure (data, NM_DBUS_VPN_SIGNAL_LAUNCH_FAILED);
-		close (vpnc_fd);
+		reply = nm_dbus_create_error_message (message, NM_DBUS_INTERFACE_VPNC, NM_DBUS_VPN_LAUNCH_FAILED,
+											  "Failed to run vpnc binary.");
+		goto out;
+	}
+
+	if (!nm_vpnc_config_write (vpnc_fd, user_name, password_items, num_passwords, data_items, num_items))
+	{
+		reply = nm_dbus_create_error_message (message, NM_DBUS_INTERFACE_VPNC, NM_DBUS_VPN_BAD_ARGUMENTS,
+											  "Invalid VPN options.");
+		goto out;
 	}
 
+	reply = dbus_message_new_method_return (message);
+	success = TRUE;
+
 out:
+
+	if (vpnc_fd >= 0)
+		close (vpnc_fd);
+
 	dbus_free_string_array (data_items);
 	dbus_free_string_array (password_items);
 	dbus_free_string_array (user_routes);
 	if (!success)
 		nm_vpnc_set_state (data, NM_VPN_STATE_STOPPED);
-	return success;
+
+	return reply;
 }
 
 
@@ -701,8 +720,7 @@
 
 		case NM_VPN_STATE_STOPPED:
 			nm_vpnc_cancel_quit_timer (data);
-			nm_vpnc_dbus_handle_start_vpn (message, data);
-			reply = dbus_message_new_method_return (message);
+			reply = nm_vpnc_dbus_handle_start_vpn (message, data);
 			break;
 
 		default: