From 3843835918c40f05877ef6d9ead53220cb416fbc Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Fri, 26 Aug 2016 16:29:47 +1000 Subject: [PATCH 1/3] ctdb-daemon: Schedule running of callback if there are no event scripts The callback should never be called before an immediate return. The callback might reply to a control and the caller of ctdb_event_script_callback_v() may not have assigned/stolen the pointer to control structure into the private data. Therefore, calling the callback can dereference an uninitialised pointer to the control structure when attempting to reply. ctdb_event_script_callback_v() must succeed when there are no event scripts. On success the caller will mark the call as asynchronous and expect the callback to be called. Given that it can't be called before return then it needs to be scheduled. BUG: https://bugzilla.samba.org/show_bug.cgi?id=12180 Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs (cherry picked from commit 9076c44f35bf309b9e183bae98829f7154b93f33) --- ctdb/server/eventscript.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/ctdb/server/eventscript.c b/ctdb/server/eventscript.c index 5bc2ee8..0e74f5c 100644 --- a/ctdb/server/eventscript.c +++ b/ctdb/server/eventscript.c @@ -699,6 +699,62 @@ static int remove_callback(struct event_script_callback *callback) return 0; } +struct schedule_callback_state { + struct ctdb_context *ctdb; + void (*callback)(struct ctdb_context *, int, void *); + void *private_data; + int status; + struct tevent_immediate *im; +}; + +static void schedule_callback_handler(struct tevent_context *ctx, + struct tevent_immediate *im, + void *private_data) +{ + struct schedule_callback_state *state = + talloc_get_type_abort(private_data, + struct schedule_callback_state); + + if (state->callback != NULL) { + state->callback(state->ctdb, state->status, + state->private_data); + } + talloc_free(state); +} + +static int +schedule_callback_immediate(struct ctdb_context *ctdb, + void (*callback)(struct ctdb_context *, + int, void *), + void *private_data, + int status) +{ + struct schedule_callback_state *state; + struct tevent_immediate *im; + + state = talloc_zero(ctdb, struct schedule_callback_state); + if (state == NULL) { + DEBUG(DEBUG_ERR, (__location__ " out of memory\n")); + return -1; + } + im = tevent_create_immediate(state); + if (im == NULL) { + DEBUG(DEBUG_ERR, (__location__ " out of memory\n")); + talloc_free(state); + return -1; + } + + state->ctdb = ctdb; + state->callback = callback; + state->private_data = private_data; + state->status = status; + state->im = im; + + tevent_schedule_immediate(im, ctdb->ev, + schedule_callback_handler, state); + return 0; +} + /* run the event script in the background, calling the callback when finished @@ -817,8 +873,14 @@ static int ctdb_event_script_callback_v(struct ctdb_context *ctdb, /* Nothing to do? */ if (state->scripts->num_scripts == 0) { - callback(ctdb, 0, private_data); + int ret = schedule_callback_immediate(ctdb, callback, + private_data, 0); talloc_free(state); + if (ret != 0) { + DEBUG(DEBUG_ERR, + ("Unable to schedule callback for 0 scripts\n")); + return 1; + } return 0; } -- 2.9.3 From c59b771f29d962f30fef5bd515faced2a57fb944 Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Fri, 26 Aug 2016 16:38:56 +1000 Subject: [PATCH 2/3] ctdb-daemon: Handle failure immediately, do housekeeping later The callback should never be called before an immediate return. The callback might reply to a control and the caller of ctdb_event_script_callback_v() may not have assigned/stolen the pointer to control structure into the private data. Therefore, calling the callback can dereference an uninitialised pointer to the control structure when attempting to reply. An event script isn't being run until the child has been forked. So update relevant state and set the destructor after this. If the child can't be forked then free the state and return with an error. The callback will not be called and the caller will process the error correctly. BUG: https://bugzilla.samba.org/show_bug.cgi?id=12180 Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs (cherry picked from commit 582518c7e89b279e34147bdb0b04b73056fac048) --- ctdb/server/eventscript.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/ctdb/server/eventscript.c b/ctdb/server/eventscript.c index 0e74f5c..087b78f 100644 --- a/ctdb/server/eventscript.c +++ b/ctdb/server/eventscript.c @@ -863,14 +863,6 @@ static int ctdb_event_script_callback_v(struct ctdb_context *ctdb, state->current = 0; state->child = 0; - if (call == CTDB_EVENT_MONITOR) { - ctdb->current_monitor = state; - } - - talloc_set_destructor(state, event_script_destructor); - - ctdb->active_events++; - /* Nothing to do? */ if (state->scripts->num_scripts == 0) { int ret = schedule_callback_immediate(ctdb, callback, @@ -886,11 +878,18 @@ static int ctdb_event_script_callback_v(struct ctdb_context *ctdb, state->scripts->scripts[0].status = fork_child_for_script(ctdb, state); if (state->scripts->scripts[0].status != 0) { - /* Callback is called from destructor, with fail result. */ talloc_free(state); - return 0; + return -1; } + if (call == CTDB_EVENT_MONITOR) { + ctdb->current_monitor = state; + } + + ctdb->active_events++; + + talloc_set_destructor(state, event_script_destructor); + if (!timeval_is_zero(&state->timeout)) { tevent_add_timer(ctdb->ev, state, timeval_current_ofs(state->timeout.tv_sec, -- 2.9.3 From 0f82ad80929ef7aa9785865ac58c63dd862a75ce Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Wed, 31 Aug 2016 08:29:13 +1000 Subject: [PATCH 3/3] 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 Reviewed-by: Amitay Isaacs (cherry picked from commit 9d975b860d52030a702723c70791c6a2829107c0) --- ctdb/server/ctdb_takeover.c | 11 +++++++---- ctdb/server/eventscript.c | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/ctdb/server/ctdb_takeover.c b/ctdb/server/ctdb_takeover.c index a613aa0..012cec8 100644 --- a/ctdb/server/ctdb_takeover.c +++ b/ctdb/server/ctdb_takeover.c @@ -501,7 +501,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; @@ -530,6 +530,7 @@ static int32_t ctdb_do_takeip(struct ctdb_context *ctdb, return -1; } + state->c = talloc_steal(ctdb, c); return 0; } @@ -638,7 +639,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; @@ -670,6 +671,7 @@ static int32_t ctdb_do_updateip(struct ctdb_context *ctdb, return -1; } + state->c = talloc_steal(ctdb, c); return 0; } @@ -1001,8 +1003,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__); @@ -1034,6 +1036,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 087b78f..2293eee 100644 --- a/ctdb/server/eventscript.c +++ b/ctdb/server/eventscript.c @@ -1068,7 +1068,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)); @@ -1084,7 +1084,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; } -- 2.9.3