From dd521e724c4c2d4f074ffcf30a9e7a844fcfb7d2 Mon Sep 17 00:00:00 2001
From: Klaus Wenninger <klaus.wenninger@aon.at>
Date: Sat, 18 May 2019 06:17:36 +0200
Subject: [PATCH] Fix: fence-lib: avoid use-after-free on early failure return

Bailing out in case of non-existant fence-agent is such a case.
---
 fencing/commands.c             | 30 +++++++++++++++++++++---------
 include/crm/fencing/internal.h |  5 +++--
 lib/fencing/st_client.c        | 28 +++++++++++++++++++++-------
 3 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/fencing/commands.c b/fencing/commands.c
index af5324a..d47a5ea 100644
--- a/fencing/commands.c
+++ b/fencing/commands.c
@@ -107,6 +107,7 @@ typedef struct async_command_s {
     int last_timeout_signo;
 
     stonith_device_t *active_on;
+    stonith_device_t *activating_on;
 } async_command_t;
 
 static xmlNode *stonith_construct_async_reply(async_command_t * cmd, const char *output,
@@ -301,6 +302,19 @@ get_active_cmds(stonith_device_t * device)
     return counter;
 }
 
+static void
+fork_cb(GPid pid, gpointer user_data)
+{
+    async_command_t *cmd = (async_command_t *) user_data;
+    stonith_device_t * device = cmd->activating_on;
+
+    crm_debug("Operation %s%s%s on %s now running with pid=%d, timeout=%ds",
+                  cmd->action, cmd->victim ? " for node " : "", cmd->victim ? cmd->victim : "",
+                  device->id, pid, cmd->timeout);
+    cmd->active_on = device;
+    cmd->activating_on = NULL;
+}
+
 static gboolean
 stonith_device_execute(stonith_device_t * device)
 {
@@ -387,19 +401,17 @@ stonith_device_execute(stonith_device_t * device)
                                    cmd->victim_nodeid,
                                    cmd->timeout, device->params, device->aliases);
 
-    /* for async exec, exec_rc is pid if positive and error code if negative/zero */
-    exec_rc = stonith_action_execute_async(action, (void *)cmd, cmd->done_cb);
-
-    if (exec_rc > 0) {
-        crm_debug("Operation %s%s%s on %s now running with pid=%d, timeout=%ds",
-                  cmd->action, cmd->victim ? " for node " : "", cmd->victim ? cmd->victim : "",
-                  device->id, exec_rc, cmd->timeout);
-        cmd->active_on = device;
+    /* for async exec, exec_rc is negative for early error exit
+       otherwise handling of success/errors is done via callbacks */
+    cmd->activating_on = device;
+    exec_rc = stonith_action_execute_async(action, (void *)cmd,
+                                           cmd->done_cb, fork_cb);
 
-    } else {
+    if (exec_rc < 0) {
         crm_warn("Operation %s%s%s on %s failed: %s (%d)",
                  cmd->action, cmd->victim ? " for node " : "", cmd->victim ? cmd->victim : "",
                  device->id, pcmk_strerror(exec_rc), exec_rc);
+        cmd->activating_on = NULL;
         cmd->done_cb(0, exec_rc, NULL, cmd);
     }
     return TRUE;
diff --git a/include/crm/fencing/internal.h b/include/crm/fencing/internal.h
index 90673bb..24df230 100644
--- a/include/crm/fencing/internal.h
+++ b/include/crm/fencing/internal.h
@@ -24,11 +24,12 @@ void stonith__destroy_action(stonith_action_t *action);
 void stonith__action_result(stonith_action_t *action, int *rc, char **output,
                             char **error_output);
 
-GPid
+int
 stonith_action_execute_async(stonith_action_t * action,
                              void *userdata,
                              void (*done) (GPid pid, int rc, const char *output,
-                                           gpointer user_data));
+                                           gpointer user_data),
+                             void (*fork_cb) (GPid pid, gpointer user_data));
 
 int stonith__execute(stonith_action_t *action);
 
diff --git a/lib/fencing/st_client.c b/lib/fencing/st_client.c
index 0c1eadc..c38f356 100644
--- a/lib/fencing/st_client.c
+++ b/lib/fencing/st_client.c
@@ -54,6 +54,7 @@ struct stonith_action_s {
     int async;
     void *userdata;
     void (*done_cb) (GPid pid, gint status, const char *output, gpointer user_data);
+    void (*fork_cb) (GPid pid, gpointer user_data);
 
     svc_action_t *svc_action;
 
@@ -835,6 +836,10 @@ stonith_action_async_forked(svc_action_t *svc_action)
     action->pid = svc_action->pid;
     action->svc_action = svc_action;
 
+    if (action->fork_cb) {
+        (action->fork_cb) (svc_action->pid, action->userdata);
+    }
+
     crm_trace("Child process %d performing action '%s' successfully forked",
               action->pid, action->action);
 }
@@ -916,25 +921,34 @@ internal_stonith_action_execute(stonith_action_t * action)
     return rc;
 }
 
-GPid
+/*!
+ * \internal
+ * \brief Kick off execution of an async stonith action
+ *
+ * \param[in,out] action        Action to be executed
+ * \param[in,out] userdata      Datapointer to be passed to callbacks
+ * \param[in]     done          Callback to notify action has failed/succeeded
+ * \param[in]     fork_callback Callback to notify successful fork of child
+ *
+ * \return pcmk_ok if ownership of action has been taken, -errno otherwise
+ */
+int
 stonith_action_execute_async(stonith_action_t * action,
                              void *userdata,
                              void (*done) (GPid pid, int rc, const char *output,
-                                           gpointer user_data))
+                                           gpointer user_data),
+                             void (*fork_cb) (GPid pid, gpointer user_data))
 {
-    int rc = 0;
-
     if (!action) {
         return -1;
     }
 
     action->userdata = userdata;
     action->done_cb = done;
+    action->fork_cb = fork_cb;
     action->async = 1;
 
-    rc = internal_stonith_action_execute(action);
-
-    return rc < 0 ? rc : action->pid;
+    return internal_stonith_action_execute(action);
 }
 
 /*!
-- 
1.8.3.1