vanishing messaging context names

[Jeremy Allison]

On Wed, Mar 29, 2017 at 10:53:04AM +1300, Andrew Bartlett wrote:
> 
> I agree.  I've been bitten by it before. 
> 
> In any case, here is a patch.  The result of irpc_add_name() is no
> longer removed by talloc_free(), but is removed when the stream or task
> is terminated.  In source3 this is left as-was, but it is safe because
> only the notify server registers a name.
> 
> Now we just need tests to show it stays around, that don't rely on
> side-effects of other tests and test test order.

Hmmm. I can see the point of it, but I think it's a band-aid
around the lack of proper handling of fork() inside the
source4 code.

If you look inside lib/util/server_id_db.c, there is already
server_id_db_reinit(), called in source3 from messaging_reinit(),
which is called from reinit_after_fork(), and *that* is
called inside every child created using fork() inside source3/

I think the proper fix is going to be doing the same thing
inside the bin/samba binary for forked tasks. Otherwise we
will forever be fighting this inside source4-forked children.

I'd rather bite the bullet and fix this properly for all
time now :-).

> From f2febc44e10e34186d5a29f3c8b9a1b2fdc69776 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Tue, 28 Mar 2017 22:02:13 +1300
> Subject: [PATCH] messaging: Move names cleanup from the destructor on
>  server_id_db
> 
> Instead, it is on the messaging_context in source3, and in imessaging_cleanup() in source4
> 
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  lib/util/server_id_db.c           | 14 +++++++++-----
>  lib/util/server_id_db.h           |  1 +
>  source3/lib/messages.c            |  4 ++++
>  source4/lib/messaging/messaging.c | 17 ++++++++++++++++-
>  4 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/util/server_id_db.c b/lib/util/server_id_db.c
> index e190f457526..059d766d94a 100644
> --- a/lib/util/server_id_db.c
> +++ b/lib/util/server_id_db.c
> @@ -37,8 +37,6 @@ struct server_id_db {
>  	char *names;
>  };
>  
> -static int server_id_db_destructor(struct server_id_db *db);
> -
>  struct server_id_db *server_id_db_init(TALLOC_CTX *mem_ctx,
>  				       struct server_id pid,
>  				       const char *base_path,
> @@ -64,8 +62,6 @@ struct server_id_db *server_id_db_init(TALLOC_CTX *mem_ctx,
>  		return NULL;
>  	}
>  
> -	talloc_set_destructor(db, server_id_db_destructor);
> -
>  	return db;
>  }
>  
> @@ -80,7 +76,15 @@ struct server_id server_id_db_pid(struct server_id_db *db)
>  	return db->pid;
>  }
>  
> -static int server_id_db_destructor(struct server_id_db *db)
> +/* 
> + * This MUST be called when the messaging context on which this
> + * server_id is finished with, but only in the same process, ie do not
> + * call it in a child. 
> + *
> + * For this reason, use in a talloc destructor should be carefully
> + * considered, and avoided if possible.
> + */
> +int server_id_db_cleanup(struct server_id_db *db)
>  {
>  	char *name = NULL;
>  
> diff --git a/lib/util/server_id_db.h b/lib/util/server_id_db.h
> index 2dcce622939..a1a5591f136 100644
> --- a/lib/util/server_id_db.h
> +++ b/lib/util/server_id_db.h
> @@ -46,5 +46,6 @@ int server_id_db_traverse_read(struct server_id_db *db,
>  					 const struct server_id *servers,
>  					 void *private_data),
>  			       void *private_data);
> +int server_id_db_cleanup(struct server_id_db *db);
>  
>  #endif
> diff --git a/source3/lib/messages.c b/source3/lib/messages.c
> index d7ad49d7c30..31b9905b0ff 100644
> --- a/source3/lib/messages.c
> +++ b/source3/lib/messages.c
> @@ -227,6 +227,10 @@ static int messaging_context_destructor(struct messaging_context *ctx)
>  		}
>  	}
>  
> +	if (ctx->names_db) {
> +		server_id_db_cleanup(ctx->names_db);
> +	}
> +	
>  	return 0;
>  }
>  
> diff --git a/source4/lib/messaging/messaging.c b/source4/lib/messaging/messaging.c
> index 4d75f0976dc..5a858483227 100644
> --- a/source4/lib/messaging/messaging.c
> +++ b/source4/lib/messaging/messaging.c
> @@ -296,9 +296,24 @@ NTSTATUS imessaging_send_ptr(struct imessaging_context *msg, struct server_id se
>  */
>  int imessaging_cleanup(struct imessaging_context *msg)
>  {
> -	if (!msg) {
> +	if (msg == NULL || msg->names == NULL) {
>  		return 0;
>  	}
> +	/* 
> +	 * Trigger the server_id cleanup, triggering the removal of
> +	 * any registered names.  This call is important because it is
> +	 * made regardless of if the talloc_free() eventually happens
> +	 * (and a destructor could remove a parent's name in a child,
> +	 * so we don't want that).
> +	 *
> +	 * This is called before the model_ops->terminate() in
> +	 * stream_terminate_connection() and task_server_terminate()
> +	 *
> +	 * PID-based sockets are cleaned up by the parent task in
> +	 * process_standard or the destructor on the referenced
> +	 * messages_dgm context (which checks getpid()).
> +	 */
> +	server_id_db_cleanup(msg->names);
>  	return 0;
>  }
>  
> -- 
> 2.11.0
>