ctdb-daemon: Don't steal control structure before synchronous reply

If *async_reply isn't set then the calling code will reply to the
control and free the control structure.  In some places the control
structure pointer is stolen onto state before a synchronous exit due
to an error condition.  The error handling then frees state and
returns an error.  The calling code will access-after-free when trying
to reply to the control.

To make this easier to understand, the convention is that any
(immediate) error results in a synchronous reply to the control via an
error return code AND *async_reply not being set.  In this case the
control structure pointer should never be stolen onto state.  State is
never used for a synchronous reply, it is only ever used by a
callback.

Also initialise state->c to NULL so that any premature call to a
callback (e.g. in an immediate error path) is more obvious.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12180

Signed-off-by: Martin Schwenke <martin@meltin.net>
Reviewed-by: Amitay Isaacs <amitay@gmail.com>
(cherry picked from commit 9d975b860d52030a702723c70791c6a2829107c0)

2 files changed, 9 insertions, 6 deletions

diff --git a/ctdb/server/ctdb_takeover.c b/ctdb/server/ctdb_takeover.c
index ede635ebb6a..d10ffeff5f5 100644
--- a/ctdb/server/ctdb_takeover.c
+++ b/ctdb/server/ctdb_takeover.c
@@ -522,7 +522,7 @@ static int32_t ctdb_do_takeip(struct ctdb_context *ctdb,
 	state = talloc(vnn, struct ctdb_do_takeip_state);
 	CTDB_NO_MEMORY(ctdb, state);
 
-	state->c = talloc_steal(ctdb, c);
+	state->c = NULL;
 	state->vnn   = vnn;
 
 	vnn->update_in_flight = true;
@@ -551,6 +551,7 @@ static int32_t ctdb_do_takeip(struct ctdb_context *ctdb,
 		return -1;
 	}
 
+	state->c = talloc_steal(ctdb, c);
 	return 0;
 }
 
@@ -659,7 +660,7 @@ static int32_t ctdb_do_updateip(struct ctdb_context *ctdb,
 	state = talloc(vnn, struct ctdb_do_updateip_state);
 	CTDB_NO_MEMORY(ctdb, state);
 
-	state->c = talloc_steal(ctdb, c);
+	state->c = NULL;
 	state->old = old;
 	state->vnn = vnn;
 
@@ -691,6 +692,7 @@ static int32_t ctdb_do_updateip(struct ctdb_context *ctdb,
 		return -1;
 	}
 
+	state->c = talloc_steal(ctdb, c);
 	return 0;
 }
 
@@ -1003,8 +1005,8 @@ int32_t ctdb_control_release_ip(struct ctdb_context *ctdb,
 		return -1;
 	}
 
-	state->c = talloc_steal(state, c);
-	state->addr = talloc(state, ctdb_sock_addr);       
+	state->c = NULL;
+	state->addr = talloc(state, ctdb_sock_addr);
 	if (state->addr == NULL) {
 		ctdb_set_error(ctdb, "Out of memory at %s:%d",
 			       __FILE__, __LINE__);
@@ -1037,6 +1039,7 @@ int32_t ctdb_control_release_ip(struct ctdb_context *ctdb,
 
 	/* tell the control that we will be reply asynchronously */
 	*async_reply = true;
+	state->c = talloc_steal(state, c);
 	return 0;
 }
 
diff --git a/ctdb/server/eventscript.c b/ctdb/server/eventscript.c
index f555625996e..86d37d901e2 100644
--- a/ctdb/server/eventscript.c
+++ b/ctdb/server/eventscript.c
@@ -1076,7 +1076,7 @@ int32_t ctdb_run_eventscripts(struct ctdb_context *ctdb,
 	state = talloc(ctdb->event_script_ctx, struct eventscript_callback_state);
 	CTDB_NO_MEMORY(ctdb, state);
 
-	state->c = talloc_steal(state, c);
+	state->c = NULL;
 
 	DEBUG(DEBUG_NOTICE,("Running eventscripts with arguments %s\n", indata.dptr));
 
@@ -1092,7 +1092,7 @@ int32_t ctdb_run_eventscripts(struct ctdb_context *ctdb,
 
 	/* tell ctdb_control.c that we will be replying asynchronously */
 	*async_reply = true;
-
+	state->c = talloc_steal(state, c);
 	return 0;
 }