The Samba-Bugzilla – Attachment 12424 Details for
Bug 12180
CTDB crashes running eventscripts
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch for 4.3
v4-3-BZ12180.patch (text/plain), 9.47 KB, created by
Martin Schwenke
on 2016-09-02 00:45:12 UTC
(
hide
)
Description:
Patch for 4.3
Filename:
MIME Type:
Creator:
Martin Schwenke
Created:
2016-09-02 00:45:12 UTC
Size:
9.47 KB
patch
obsolete
>From 14dea25b844990cf43ed571717eebac62b4e7830 Mon Sep 17 00:00:00 2001 >From: Martin Schwenke <martin@meltin.net> >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 <martin@meltin.net> >Reviewed-by: Amitay Isaacs <amitay@gmail.com> >(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 49619b2..bb1891d 100644 >--- a/ctdb/server/eventscript.c >+++ b/ctdb/server/eventscript.c >@@ -680,6 +680,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 >@@ -798,8 +854,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 9a9f11728e9c970aba7f97c4bdb075737d0e7410 Mon Sep 17 00:00:00 2001 >From: Martin Schwenke <martin@meltin.net> >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 <martin@meltin.net> >Reviewed-by: Amitay Isaacs <amitay@gmail.com> >(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 bb1891d..fc3be56 100644 >--- a/ctdb/server/eventscript.c >+++ b/ctdb/server/eventscript.c >@@ -844,14 +844,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, >@@ -867,11 +859,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)) { > event_add_timed(ctdb->ev, state, timeval_current_ofs(state->timeout.tv_sec, state->timeout.tv_usec), ctdb_event_script_timeout, state); > } else { >-- >2.9.3 > > >From 6c61fc98704e3454afad9e98109e8b2c6604faaa Mon Sep 17 00:00:00 2001 >From: Martin Schwenke <martin@meltin.net> >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 <martin@meltin.net> >Reviewed-by: Amitay Isaacs <amitay@gmail.com> >(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 efc80b1..30dbf93 100644 >--- a/ctdb/server/ctdb_takeover.c >+++ b/ctdb/server/ctdb_takeover.c >@@ -487,7 +487,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; >@@ -516,6 +516,7 @@ static int32_t ctdb_do_takeip(struct ctdb_context *ctdb, > return -1; > } > >+ state->c = talloc_steal(ctdb, c); > return 0; > } > >@@ -624,7 +625,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; > >@@ -656,6 +657,7 @@ static int32_t ctdb_do_updateip(struct ctdb_context *ctdb, > return -1; > } > >+ state->c = talloc_steal(ctdb, c); > return 0; > } > >@@ -987,8 +989,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__); >@@ -1020,6 +1022,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 fc3be56..30ca04f 100644 >--- a/ctdb/server/eventscript.c >+++ b/ctdb/server/eventscript.c >@@ -1046,7 +1046,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)); > >@@ -1062,7 +1062,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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Flags:
amitay
:
review-
Actions:
View
Attachments on
bug 12180
:
12422
|
12423
| 12424