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>

diff --git a/ctdb/server/ctdb_takeover.c b/ctdb/server/ctdb_takeover.c
index ff096ce1a3d98cc4e2dada846ae90ba7351931c9..2955b374f2638370442dc9a8fbcac7b083369b32 100644 (file)
--- a/ctdb/server/ctdb_takeover.c
+++ b/ctdb/server/ctdb_takeover.c
@@ -523,7 +523,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;
@@ -552,6 +552,7 @@ static int32_t ctdb_do_takeip(struct ctdb_context *ctdb,
                return -1;
        }
 
+       state->c = talloc_steal(ctdb, c);
        return 0;
 }
 
@@ -663,7 +664,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;
 
@@ -696,6 +697,7 @@ static int32_t ctdb_do_updateip(struct ctdb_context *ctdb,
                return -1;
        }
 
+       state->c = talloc_steal(ctdb, c);
        return 0;
 }
 
@@ -999,8 +1001,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__);
@@ -1033,6 +1035,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 f555625996ecd60245c93c9c7bd0e3e13019be32..86d37d901e2ee2b2fac018b2ef674909d8023b9e 100644 (file)
--- 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;
 }