[network-manager-openvpn/th/handle-child-process-bgo761299: 1/5] service: fix handling child processes and their cleanup

[Thomas Haller <thaller src gnome org>]

commit 48cda180bd0e09c2b3d7d94126f22a18d26b8e8c
Author: Thomas Haller <thaller redhat com>
Date:   Fri Jan 29 17:07:25 2016 +0100

    service: fix handling child processes and their cleanup
    
    Previously, ensure_killed() would never be canceled, meaning
    2 seconds after sending SIGTERM to the openvpn process we would
    always send a SIGKILL (to whatever process happens to be around).
    
    Also, the child-watch callback would sooner or later be invoked
    when the process finally dies. But since we never unsubscribe
    the watcher, we can get the signal for a previously died process.
    Also, waitpid() is wrong in a child-watch.
    
    Redo this and keep track of the pending processes in a list
    @pids_pending_list. The currently active process is the one
    with matching priv->pid.

 src/nm-openvpn-service.c |  117 ++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 97 insertions(+), 20 deletions(-)
---
diff --git a/src/nm-openvpn-service.c b/src/nm-openvpn-service.c
index 4ca2e3d..dbca720 100644
--- a/src/nm-openvpn-service.c
+++ b/src/nm-openvpn-service.c
@@ -50,6 +50,7 @@
 #include "nm-openvpn-service.h"
 #include "nm-utils.h"
 #include "utils.h"
+#include "nm-macros-internal.h"
 
 #if !defined(DIST_VERSION)
 # define DIST_VERSION VERSION
@@ -83,6 +84,7 @@ typedef struct {
        NMOpenvpnPluginIOData *io_data;
        gboolean interactive;
        char *mgt_path;
+       GSList *pids_pending_list;
 } NMOpenvpnPluginPrivate;
 
 typedef struct {
@@ -93,6 +95,12 @@ typedef struct {
        gboolean address;
 } ValidProperty;
 
+typedef struct {
+       GPid pid;
+       guint watch_id;
+       guint kill_id;
+} PidsPendingData;
+
 static ValidProperty valid_properties[] = {
        { NM_OPENVPN_KEY_AUTH,                 G_TYPE_STRING, 0, 0, FALSE },
        { NM_OPENVPN_KEY_CA,                   G_TYPE_STRING, 0, 0, FALSE },
@@ -146,6 +154,28 @@ static ValidProperty valid_secrets[] = {
        { NULL,                                G_TYPE_NONE, FALSE }
 };
 
+/*****************************************************************************/
+
+static void
+pids_pending_data_free (PidsPendingData *data)
+{
+       nm_clear_g_source (&data->watch_id);
+       nm_clear_g_source (&data->kill_id);
+       g_slice_free (PidsPendingData, data);
+}
+
+static PidsPendingData *
+pids_pending_get (GSList *pids_pending_list, GPid pid)
+{
+       for (; pids_pending_list; pids_pending_list = pids_pending_list->next) {
+               if (((PidsPendingData *) pids_pending_list->data)->pid == pid)
+                       return pids_pending_list->data;
+       }
+       g_return_val_if_reached (NULL);
+}
+
+/*****************************************************************************/
+
 static gboolean
 validate_address (const char *address)
 {
@@ -650,6 +680,7 @@ openvpn_watch_cb (GPid pid, gint status, gpointer user_data)
        NMVpnPluginFailure failure = NM_VPN_PLUGIN_FAILURE_CONNECT_FAILED;
        guint error = 0;
        gboolean good_exit = FALSE;
+       PidsPendingData *pid_data = NULL;
 
        if (WIFEXITED (status)) {
                error = WEXITSTATUS (status);
@@ -663,10 +694,23 @@ openvpn_watch_cb (GPid pid, gint status, gpointer user_data)
        else if (WIFSIGNALED (status))
                g_warning ("openvpn[%ld] died with signal %d", (long) pid, WTERMSIG (status));
        else
-               g_warning ("openvpn[%ld] died from an unknown cause", (long) pid);
+               g_warning ("openvpn[%ld] died from an unnatural cause", (long) pid);
+
+       pid_data = pids_pending_get (priv->pids_pending_list, pid);
+       if (!pid_data)
+               g_return_if_reached ();
+       priv->pids_pending_list = g_slist_remove (priv->pids_pending_list , pid_data);
+
+       pid_data->watch_id = 0;
+       pids_pending_data_free (pid_data);
 
        /* Reap child if needed. */
-       waitpid (priv->pid, NULL, WNOHANG);
+       if (priv->pid != pid) {
+               /* the dead child is not the currently active process. Nothing to do, we just
+                * reaped the PID. */
+               return;
+       }
+
        priv->pid = 0;
 
        /* OpenVPN doesn't supply useful exit codes :( */
@@ -932,13 +976,13 @@ nm_openvpn_start_openvpn_binary (NMOpenvpnPlugin *plugin,
        NMOpenvpnPluginPrivate *priv = NM_OPENVPN_PLUGIN_GET_PRIVATE (plugin);
        const char *openvpn_binary, *auth, *connection_type, *tmp, *tmp2, *tmp3, *tmp4;
        GPtrArray *args;
-       GSource *openvpn_watch;
        GPid pid;
        gboolean dev_type_is_tap;
        char *stmp;
        const char *defport, *proto_tcp;
        const char *nm_openvpn_user, *nm_openvpn_group, *nm_openvpn_chroot;
        gchar *bus_name;
+       PidsPendingData *pid_data;
 
        /* Find openvpn */
        openvpn_binary = nm_find_openvpn ();
@@ -1450,13 +1494,17 @@ nm_openvpn_start_openvpn_binary (NMOpenvpnPlugin *plugin,
        }
        free_openvpn_args (args);
 
-       g_message ("openvpn started with pid %d", pid);
+       g_message ("openvpn[%ld] started", (long) pid);
+
+       pid_data = g_slice_new (PidsPendingData);
+       pid_data->pid = pid;
+       pid_data->kill_id = 0;
+       pid_data->watch_id = g_child_watch_add (pid, openvpn_watch_cb, plugin);
 
+       priv->pids_pending_list = g_slist_prepend (priv->pids_pending_list, pid_data);
+
+       g_warn_if_fail (!priv->pid);
        priv->pid = pid;
-       openvpn_watch = g_child_watch_source_new (pid);
-       g_source_set_callback (openvpn_watch, (GSourceFunc) openvpn_watch_cb, plugin, NULL);
-       g_source_attach (openvpn_watch, NULL);
-       g_source_unref (openvpn_watch);
 
        /* Listen to the management socket for a few connection types:
           PASSWORD: Will require username and password
@@ -1532,13 +1580,14 @@ check_need_secrets (NMSettingVpn *s_vpn, gboolean *need_secrets)
 }
 
 static gboolean
-ensure_killed (gpointer data)
+ensure_killed (gpointer user_data)
 {
-       int pid = GPOINTER_TO_INT (data);
+       PidsPendingData *pid_data = user_data;
 
-       if (kill (pid, 0) == 0)
-               kill (pid, SIGKILL);
+       g_message ("openvpn[%ld]: send SIGKILL", (long) pid_data->pid);
 
+       pid_data->kill_id = 0;
+       kill (pid_data->pid, SIGKILL);
        return FALSE;
 }
 
@@ -1548,18 +1597,22 @@ real_disconnect (NMVpnServicePlugin *plugin,
 {
        NMOpenvpnPluginPrivate *priv = NM_OPENVPN_PLUGIN_GET_PRIVATE (plugin);
 
+       g_clear_pointer (&priv->mgt_path, g_free);
+
        if (priv->pid) {
-               if (kill (priv->pid, SIGTERM) == 0)
-                       g_timeout_add (2000, ensure_killed, GINT_TO_POINTER (priv->pid));
-               else
-                       kill (priv->pid, SIGKILL);
+               PidsPendingData *pid_data;
+               GPid pid = priv->pid;
 
-               g_message ("Terminated openvpn daemon with PID %d.", priv->pid);
                priv->pid = 0;
-       }
 
-       g_free (priv->mgt_path);
-       priv->mgt_path = NULL;
+               pid_data = pids_pending_get (priv->pids_pending_list, pid);
+               g_return_val_if_fail (pid_data, TRUE);
+
+               g_message ("openvpn[%ld]: send SIGTERM", (long) pid);
+
+               kill (pid, SIGTERM);
+               pid_data->kill_id = g_timeout_add (2000, ensure_killed, pid_data);
+       }
 
        return TRUE;
 }
@@ -1730,6 +1783,28 @@ nm_openvpn_plugin_init (NMOpenvpnPlugin *plugin)
 }
 
 static void
+dispose (GObject *object)
+{
+       NMOpenvpnPluginPrivate *priv = NM_OPENVPN_PLUGIN_GET_PRIVATE (object);
+
+       while (priv->pids_pending_list) {
+               PidsPendingData *pid_data = priv->pids_pending_list->data;
+
+               priv->pids_pending_list = g_slist_delete_link (priv->pids_pending_list, 
priv->pids_pending_list);
+
+               if (priv->pid == pid_data->pid) {
+                       kill (priv->pid, SIGTERM);
+                       priv->pid = 0;
+                       g_usleep (1);
+               }
+               kill (priv->pid, SIGKILL);
+               pids_pending_data_free (pid_data);
+       }
+
+       G_OBJECT_CLASS (nm_openvpn_plugin_parent_class)->dispose (object);
+}
+
+static void
 nm_openvpn_plugin_class_init (NMOpenvpnPluginClass *plugin_class)
 {
        GObjectClass *object_class = G_OBJECT_CLASS (plugin_class);
@@ -1737,6 +1812,8 @@ nm_openvpn_plugin_class_init (NMOpenvpnPluginClass *plugin_class)
 
        g_type_class_add_private (object_class, sizeof (NMOpenvpnPluginPrivate));
 
+       object_class->dispose = dispose;
+
        /* virtual methods */
        parent_class->connect      = real_connect;
        parent_class->connect_interactive = real_connect_interactive;