Reviewed-by: Steven Dake <sdake@xxxxxxxxxx>
On 08/01/2012 08:14 AM, Jan Friesse wrote:
> When service is unloaded, sync shouldn't call sync_init|process|activate
> and abort functions. It happens very rare, but in process of unloading
> all services, totem can recreate membership and bad things can happen
> (service is unloaded, so there may be access to already freed memory,
> ...)
>
> Solution is to fetch services sync handlers in every time when we are
> building service list instead of using precreated one.
>
> Signed-off-by: Jan Friesse <jfriesse@xxxxxxxxxx>
> ---
> exec/sync.c | 64 +++++++++++++++++++++++++++++-----------------------------
> 1 files changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/exec/sync.c b/exec/sync.c
> index f9f8cb4..b09ea53 100644
> --- a/exec/sync.c
> +++ b/exec/sync.c
> @@ -149,10 +149,6 @@ static int my_service_list_entries = 0;
>
> static const struct memb_ring_id sync_ring_id;
>
> -static struct service_entry my_initial_service_list[SERVICES_COUNT_MAX];
> -
> -static int my_initial_service_list_entries;
> -
> static void (*sync_synchronization_completed) (void);
>
> static void sync_deliver_fn (
> @@ -172,6 +168,10 @@ static struct totempg_group sync_group = {
>
> static void *sync_group_handle;
>
> +int (*my_sync_callbacks_retrieve) (
> + int service_id,
> + struct sync_callbacks *callbacks);
> +
> int sync_init (
> int (*sync_callbacks_retrieve) (
> int service_id,
> @@ -179,8 +179,6 @@ int sync_init (
> void (*synchronization_completed) (void))
> {
> unsigned int res;
> - int i;
> - struct sync_callbacks sync_callbacks;
>
> res = totempg_groups_initialize (
> &sync_group_handle,
> @@ -202,25 +200,8 @@ int sync_init (
> }
>
> sync_synchronization_completed = synchronization_completed;
> - for (i = 0; i < SERVICES_COUNT_MAX; i++) {
> - res = sync_callbacks_retrieve (i, &sync_callbacks);
> - if (res == -1) {
> - continue;
> - }
> - if (sync_callbacks.sync_init == NULL) {
> - continue;
> - }
> - my_initial_service_list[my_initial_service_list_entries].state =
> - INIT;
> - my_initial_service_list[my_initial_service_list_entries].service_id = i;
> - strcpy (my_initial_service_list[my_initial_service_list_entries].name,
> - sync_callbacks.name);
> - my_initial_service_list[my_initial_service_list_entries].sync_init = sync_callbacks.sync_init;
> - my_initial_service_list[my_initial_service_list_entries].sync_process = sync_callbacks.sync_process;
> - my_initial_service_list[my_initial_service_list_entries].sync_abort = sync_callbacks.sync_abort;
> - my_initial_service_list[my_initial_service_list_entries].sync_activate = sync_callbacks.sync_activate;
> - my_initial_service_list_entries += 1;
> - }
> + my_sync_callbacks_retrieve = sync_callbacks_retrieve;
> +
> return (0);
> }
>
> @@ -346,7 +327,7 @@ static void sync_service_build_handler (unsigned int nodeid, const void *msg)
> my_service_list[my_service_list_entries].service_id =
> req_exec_service_build_message->service_list[i];
> sprintf (my_service_list[my_service_list_entries].name,
> - "External Service (id = %d)\n",
> + "Uknown External Service (id = %d)\n",
> req_exec_service_build_message->service_list[i]);
> my_service_list[my_service_list_entries].sync_init =
> dummy_sync_init;
> @@ -491,6 +472,8 @@ static void sync_servicelist_build_enter (
> {
> struct req_exec_service_build_message service_build;
> int i;
> + int res;
> + struct sync_callbacks sync_callbacks;
>
> my_state = SYNC_SERVICELIST_BUILD;
> for (i = 0; i < member_list_entries; i++) {
> @@ -505,14 +488,31 @@ static void sync_servicelist_build_enter (
>
> my_processing_idx = 0;
>
> - memcpy (my_service_list, my_initial_service_list,
> - sizeof (struct service_entry) *
> - my_initial_service_list_entries);
> - my_service_list_entries = my_initial_service_list_entries;
> + memset(my_service_list, 0, sizeof (struct service_entry) * SERVICES_COUNT_MAX);
> + my_service_list_entries = 0;
> +
> + for (i = 0; i < SERVICES_COUNT_MAX; i++) {
> + res = my_sync_callbacks_retrieve (i, &sync_callbacks);
> + if (res == -1) {
> + continue;
> + }
> + if (sync_callbacks.sync_init == NULL) {
> + continue;
> + }
> + my_service_list[my_service_list_entries].state = INIT;
> + my_service_list[my_service_list_entries].service_id = i;
> + strcpy (my_service_list[my_service_list_entries].name,
> + sync_callbacks.name);
> + my_service_list[my_service_list_entries].sync_init = sync_callbacks.sync_init;
> + my_service_list[my_service_list_entries].sync_process = sync_callbacks.sync_process;
> + my_service_list[my_service_list_entries].sync_abort = sync_callbacks.sync_abort;
> + my_service_list[my_service_list_entries].sync_activate = sync_callbacks.sync_activate;
> + my_service_list_entries += 1;
> + }
>
> - for (i = 0; i < my_initial_service_list[i].service_id; i++) {
> + for (i = 0; i < my_service_list[i].service_id; i++) {
> service_build.service_list[i] =
> - my_initial_service_list[i].service_id;
> + my_service_list[i].service_id;
> }
> service_build.service_list_entries = i;
>
>
_______________________________________________
discuss mailing list
discuss@xxxxxxxxxxxx
http://lists.corosync.org/mailman/listinfo/discuss
