[PATCH 1/6] Make winbind client library thread-safe by adding context

Volker Lendecke Volker.Lendecke at SerNet.DE
Tue Feb 24 04:20:41 MST 2015


On Mon, Feb 23, 2015 at 01:19:37AM +0000, Matthew Newton wrote:
> Rather than keep state in global variables, store the current
> context such as the winbind file descriptor in a struct that is
> passed in. This makes the winbind client library thread-safe.
> 
> Signed-off-by: Matthew Newton <matthew-git at newtoncomputing.co.uk>
> ---
>  nsswitch/wb_common.c                   |  180 ++++++++++++++++++++++----------
>  nsswitch/winbind_client.h              |   21 ++--
>  nsswitch/winbind_struct_protocol.h     |    9 ++
>  source4/torture/winbind/struct_based.c |    2 +-
>  4 files changed, 150 insertions(+), 62 deletions(-)
> 
> diff --git a/nsswitch/wb_common.c b/nsswitch/wb_common.c
> index 44bfaf4..e2a209b 100644
> --- a/nsswitch/wb_common.c
> +++ b/nsswitch/wb_common.c
> @@ -6,6 +6,7 @@
>     Copyright (C) Tim Potter 2000
>     Copyright (C) Andrew Tridgell 2000
>     Copyright (C) Andrew Bartlett 2002
> +   Copyright (C) Matthew Newton 2015
>  
>  
>     This library is free software; you can redistribute it and/or
> @@ -26,10 +27,13 @@
>  #include "system/select.h"
>  #include "winbind_client.h"
>  
> -/* Global variables.  These are effectively the client state information */
> +/* Global context */
>  
> -int winbindd_fd = -1;           /* fd for winbindd socket */
> -static int is_privileged = 0;
> +static struct winbindd_context wb_global_ctx = {
> +	.winbindd_fd = -1,
> +	.is_privileged = 0,
> +	.our_pid = 0
> +};
>  
>  /* Free a response structure */
>  
> @@ -64,15 +68,26 @@ static void init_response(struct winbindd_response *response)
>  
>  /* Close established socket */
>  
> +static void winbind_close_sock(struct winbindd_context *ctx)
> +{
> +	if (!ctx) {
> +		return;
> +	}
> +
> +	if (ctx->winbindd_fd != -1) {
> +		close(ctx->winbindd_fd);
> +		ctx->winbindd_fd = -1;
> +	}
> +}
> +
> +/* Destructor for global context to ensure fd is closed */
> +
>  #if HAVE_FUNCTION_ATTRIBUTE_DESTRUCTOR
>  __attribute__((destructor))
>  #endif
> -static void winbind_close_sock(void)
> +static void winbind_destructor(void)
>  {
> -	if (winbindd_fd != -1) {
> -		close(winbindd_fd);
> -		winbindd_fd = -1;
> -	}
> +	winbind_close_sock(&wb_global_ctx);
>  }
>  
>  #define CONNECT_TIMEOUT 30
> @@ -333,43 +348,52 @@ static const char *winbindd_socket_dir(void)
>  
>  /* Connect to winbindd socket */
>  
> -static int winbind_open_pipe_sock(int recursing, int need_priv)
> +static int winbind_open_pipe_sock(struct winbindd_context *ctx,
> +				  int recursing, int need_priv)
>  {
>  #ifdef HAVE_UNIXSOCKET
> -	static pid_t our_pid;
>  	struct winbindd_request request;
>  	struct winbindd_response response;
> +
>  	ZERO_STRUCT(request);
>  	ZERO_STRUCT(response);
>  
> -	if (our_pid != getpid()) {
> -		winbind_close_sock();
> -		our_pid = getpid();
> +	if (!ctx) {
> +		return -1;
> +	}
> +
> +	if (ctx->our_pid != getpid()) {
> +		winbind_close_sock(ctx);
> +		ctx->our_pid = getpid();
>  	}
>  
> -	if ((need_priv != 0) && (is_privileged == 0)) {
> -		winbind_close_sock();
> +	if ((need_priv != 0) && (ctx->is_privileged == 0)) {
> +		winbind_close_sock(ctx);
>  	}
>  
> -	if (winbindd_fd != -1) {
> -		return winbindd_fd;
> +	if (ctx->winbindd_fd != -1) {
> +		return ctx->winbindd_fd;
>  	}
>  
>  	if (recursing) {
>  		return -1;
>  	}
>  
> -	if ((winbindd_fd = winbind_named_pipe_sock(winbindd_socket_dir())) == -1) {
> +	ctx->winbindd_fd = winbind_named_pipe_sock(winbindd_socket_dir());
> +
> +	if (ctx->winbindd_fd == -1) {
>  		return -1;
>  	}
>  
> -	is_privileged = 0;
> +	ctx->is_privileged = 0;
>  
>  	/* version-check the socket */
>  
>  	request.wb_flags = WBFLAG_RECURSE;
> -	if ((winbindd_request_response(WINBINDD_INTERFACE_VERSION, &request, &response) != NSS_STATUS_SUCCESS) || (response.data.interface_version != WINBIND_INTERFACE_VERSION)) {
> -		winbind_close_sock();
> +	if ((winbindd_request_response(ctx, WINBINDD_INTERFACE_VERSION, &request,
> +				       &response) != NSS_STATUS_SUCCESS) ||
> +	    (response.data.interface_version != WINBIND_INTERFACE_VERSION)) {
> +		winbind_close_sock(ctx);
>  		return -1;
>  	}
>  
> @@ -383,22 +407,24 @@ static int winbind_open_pipe_sock(int recursing, int need_priv)
>  	 * thus response.extra_data.data will not be NULL even though
>  	 * winbindd response did not write over it due to a failure */
>  	ZERO_STRUCT(response);
> -	if (winbindd_request_response(WINBINDD_PRIV_PIPE_DIR, &request, &response) == NSS_STATUS_SUCCESS) {
> +	if (winbindd_request_response(ctx, WINBINDD_PRIV_PIPE_DIR, &request,
> +				      &response) == NSS_STATUS_SUCCESS) {
>  		int fd;
> -		if ((fd = winbind_named_pipe_sock((char *)response.extra_data.data)) != -1) {
> -			close(winbindd_fd);
> -			winbindd_fd = fd;
> -			is_privileged = 1;
> +		fd = winbind_named_pipe_sock((char *)response.extra_data.data);
> +		if (fd != -1) {
> +			close(ctx->winbindd_fd);
> +			ctx->winbindd_fd = fd;
> +			ctx->is_privileged = 1;
>  		}
>  	}
>  
> -	if ((need_priv != 0) && (is_privileged == 0)) {
> +	if ((need_priv != 0) && (ctx->is_privileged == 0)) {
>  		return -1;
>  	}
>  
>  	SAFE_FREE(response.extra_data.data);
>  
> -	return winbindd_fd;
> +	return ctx->winbindd_fd;
>  #else
>  	return -1;
>  #endif /* HAVE_UNIXSOCKET */
> @@ -406,8 +432,8 @@ static int winbind_open_pipe_sock(int recursing, int need_priv)
>  
>  /* Write data to winbindd socket */
>  
> -static int winbind_write_sock(void *buffer, int count, int recursing,
> -			      int need_priv)
> +static int winbind_write_sock(struct winbindd_context *ctx, void *buffer,
> +			      int count, int recursing, int need_priv)
>  {
>  	int fd, result, nwritten;
>  
> @@ -415,7 +441,7 @@ static int winbind_write_sock(void *buffer, int count, int recursing,
>  
>   restart:
>  
> -	fd = winbind_open_pipe_sock(recursing, need_priv);
> +	fd = winbind_open_pipe_sock(ctx, recursing, need_priv);
>  	if (fd == -1) {
>  		errno = ENOENT;
>  		return -1;
> @@ -437,7 +463,7 @@ static int winbind_write_sock(void *buffer, int count, int recursing,
>  
>  		ret = poll(&pfd, 1, -1);
>  		if (ret == -1) {
> -			winbind_close_sock();
> +			winbind_close_sock(ctx);
>  			return -1;                   /* poll error */
>  		}
>  
> @@ -447,7 +473,7 @@ static int winbind_write_sock(void *buffer, int count, int recursing,
>  
>  			/* Pipe has closed on remote end */
>  
> -			winbind_close_sock();
> +			winbind_close_sock(ctx);
>  			goto restart;
>  		}
>  
> @@ -460,7 +486,7 @@ static int winbind_write_sock(void *buffer, int count, int recursing,
>  
>  			/* Write failed */
>  
> -			winbind_close_sock();
> +			winbind_close_sock(ctx);
>  			return -1;
>  		}
>  
> @@ -472,13 +498,14 @@ static int winbind_write_sock(void *buffer, int count, int recursing,
>  
>  /* Read data from winbindd socket */
>  
> -static int winbind_read_sock(void *buffer, int count)
> +static int winbind_read_sock(struct winbindd_context *ctx,
> +			     void *buffer, int count)
>  {
>  	int fd;
>  	int nread = 0;
>  	int total_time = 0;
>  
> -	fd = winbind_open_pipe_sock(false, false);
> +	fd = winbind_open_pipe_sock(ctx, false, false);
>  	if (fd == -1) {
>  		return -1;
>  	}
> @@ -498,7 +525,7 @@ static int winbind_read_sock(void *buffer, int count)
>  
>  		ret = poll(&pfd, 1, 5000);
>  		if (ret == -1) {
> -			winbind_close_sock();
> +			winbind_close_sock(ctx);
>  			return -1;                   /* poll error */
>  		}
>  
> @@ -506,7 +533,7 @@ static int winbind_read_sock(void *buffer, int count)
>  			/* Not ready for read yet... */
>  			if (total_time >= 30) {
>  				/* Timeout */
> -				winbind_close_sock();
> +				winbind_close_sock(ctx);
>  				return -1;
>  			}
>  			total_time += 5;
> @@ -526,7 +553,7 @@ static int winbind_read_sock(void *buffer, int count)
>  				   can do here is just return -1 and fail since the
>  				   transaction has failed half way through. */
>  
> -				winbind_close_sock();
> +				winbind_close_sock(ctx);
>  				return -1;
>  			}
>  
> @@ -540,7 +567,8 @@ static int winbind_read_sock(void *buffer, int count)
>  
>  /* Read reply */
>  
> -static int winbindd_read_reply(struct winbindd_response *response)
> +static int winbindd_read_reply(struct winbindd_context *ctx,
> +			       struct winbindd_response *response)
>  {
>  	int result1, result2 = 0;
>  
> @@ -550,7 +578,7 @@ static int winbindd_read_reply(struct winbindd_response *response)
>  
>  	/* Read fixed length response */
>  
> -	result1 = winbind_read_sock(response,
> +	result1 = winbind_read_sock(ctx, response,
>  				    sizeof(struct winbindd_response));
>  	if (result1 == -1) {
>  		return -1;
> @@ -578,7 +606,7 @@ static int winbindd_read_reply(struct winbindd_response *response)
>  			return -1;
>  		}
>  
> -		result2 = winbind_read_sock(response->extra_data.data,
> +		result2 = winbind_read_sock(ctx, response->extra_data.data,
>  					    extra_data_len);
>  		if (result2 == -1) {
>  			winbindd_free_response(response);
> @@ -595,7 +623,8 @@ static int winbindd_read_reply(struct winbindd_response *response)
>   * send simple types of requests
>   */
>  
> -NSS_STATUS winbindd_send_request(int req_type, int need_priv,
> +NSS_STATUS winbindd_send_request(struct winbindd_context *ctx,
> +				 int req_type, int need_priv,
>  				 struct winbindd_request *request)
>  {
>  	struct winbindd_request lrequest;
> @@ -615,7 +644,7 @@ NSS_STATUS winbindd_send_request(int req_type, int need_priv,
>  
>  	winbindd_init_request(request, req_type);
>  
> -	if (winbind_write_sock(request, sizeof(*request),
> +	if (winbind_write_sock(ctx, request, sizeof(*request),
>  			       request->wb_flags & WBFLAG_RECURSE,
>  			       need_priv) == -1)
>  	{
> @@ -626,7 +655,7 @@ NSS_STATUS winbindd_send_request(int req_type, int need_priv,
>  	}
>  
>  	if ((request->extra_len != 0) &&
> -	    (winbind_write_sock(request->extra_data.data,
> +	    (winbind_write_sock(ctx, request->extra_data.data,
>  				request->extra_len,
>  				request->wb_flags & WBFLAG_RECURSE,
>  				need_priv) == -1))
> @@ -644,7 +673,8 @@ NSS_STATUS winbindd_send_request(int req_type, int need_priv,
>   * Get results from winbindd request
>   */
>  
> -NSS_STATUS winbindd_get_response(struct winbindd_response *response)
> +NSS_STATUS winbindd_get_response(struct winbindd_context *ctx,
> +				 struct winbindd_response *response)
>  {
>  	struct winbindd_response lresponse;
>  
> @@ -656,7 +686,7 @@ NSS_STATUS winbindd_get_response(struct winbindd_response *response)
>  	init_response(response);
>  
>  	/* Wait for reply */
> -	if (winbindd_read_reply(response) == -1) {
> +	if (winbindd_read_reply(ctx, response) == -1) {
>  		/* Set ENOENT for consistency.  Required by some apps */
>  		errno = ENOENT;
>  
> @@ -678,38 +708,78 @@ NSS_STATUS winbindd_get_response(struct winbindd_response *response)
>  
>  /* Handle simple types of requests */
>  
> -NSS_STATUS winbindd_request_response(int req_type,
> -			    struct winbindd_request *request,
> -			    struct winbindd_response *response)
> +NSS_STATUS winbindd_request_response(struct winbindd_context *ctx,
> +				     int req_type,
> +				     struct winbindd_request *request,
> +				     struct winbindd_response *response)
>  {
>  	NSS_STATUS status = NSS_STATUS_UNAVAIL;
>  	int count = 0;
> +	struct winbindd_context *wb_ctx = ctx;
> +
> +	if (ctx == NULL) {
> +		wb_ctx = &wb_global_ctx;
> +	}
>  
>  	while ((status == NSS_STATUS_UNAVAIL) && (count < 10)) {
> -		status = winbindd_send_request(req_type, 0, request);
> +		status = winbindd_send_request(wb_ctx, req_type, 0, request);
>  		if (status != NSS_STATUS_SUCCESS)
>  			return(status);
> -		status = winbindd_get_response(response);
> +		status = winbindd_get_response(wb_ctx, response);
>  		count += 1;
>  	}
>  
>  	return status;
>  }
>  
> -NSS_STATUS winbindd_priv_request_response(int req_type,
> +NSS_STATUS winbindd_priv_request_response(struct winbindd_context *ctx,
> +					  int req_type,
>  					  struct winbindd_request *request,
>  					  struct winbindd_response *response)
>  {
>  	NSS_STATUS status = NSS_STATUS_UNAVAIL;
>  	int count = 0;
> +	struct winbindd_context *wb_ctx;
> +
> +	if (ctx == NULL) {
> +		wb_ctx = &wb_global_ctx;
> +	}
>  
>  	while ((status == NSS_STATUS_UNAVAIL) && (count < 10)) {
> -		status = winbindd_send_request(req_type, 1, request);
> +		status = winbindd_send_request(wb_ctx, req_type, 1, request);
>  		if (status != NSS_STATUS_SUCCESS)
>  			return(status);
> -		status = winbindd_get_response(response);
> +		status = winbindd_get_response(wb_ctx, response);
>  		count += 1;
>  	}
>  
>  	return status;
>  }
> +
> +/* Create and free winbindd context */
> +
> +struct winbindd_context *winbindd_ctx_create(void)
> +{
> +	struct winbindd_context *ctx;
> +
> +	ctx = malloc(sizeof(struct winbindd_context));
> +
> +	if (!ctx) {
> +		return NULL;
> +	}
> +
> +	ctx->winbindd_fd = -1;
> +	ctx->is_privileged = 0;
> +	ctx->our_pid = 0;

Maybe call calloc and just set winbindd_fd?

> +	return ctx;
> +}
> +
> +void winbindd_ctx_free(struct winbindd_context *ctx)
> +{
> +	winbind_close_sock(ctx);
> +
> +	if (ctx) {
> +		free(ctx);
> +	}

free() deals fine with a NULL pointer, so the if-statement
is not necessary here.

> diff --git a/nsswitch/winbind_client.h b/nsswitch/winbind_client.h
> index 905a189..16e7418 100644
> --- a/nsswitch/winbind_client.h
> +++ b/nsswitch/winbind_client.h
> @@ -6,6 +6,7 @@
>     Copyright (C) Tim Potter 2000
>     Copyright (C) Andrew Tridgell 2000
>     Copyright (C) Andrew Bartlett 2002
> +   Copyright (C) Matthew Newton 2015
>  
>  
>     This library is free software; you can redistribute it and/or
> @@ -29,15 +30,23 @@
>  #include "winbind_struct_protocol.h"
>  
>  void winbindd_free_response(struct winbindd_response *response);
> -NSS_STATUS winbindd_send_request(int req_type, int need_priv,
> +NSS_STATUS winbindd_send_request(struct winbindd_context *ctx,
> +				 int req_type, int need_priv,
>  				 struct winbindd_request *request);
> -NSS_STATUS winbindd_get_response(struct winbindd_response *response);
> -NSS_STATUS winbindd_request_response(int req_type,
> -			    struct winbindd_request *request,
> -			    struct winbindd_response *response);
> -NSS_STATUS winbindd_priv_request_response(int req_type,
> +NSS_STATUS winbindd_get_response(struct winbindd_context *ctx,
> +				 struct winbindd_response *response);
> +NSS_STATUS winbindd_request_response(struct winbindd_context *ctx,
> +				     int req_type,
> +				     struct winbindd_request *request,
> +				     struct winbindd_response *response);
> +NSS_STATUS winbindd_priv_request_response(struct winbindd_context *ctx,
> +					  int req_type,
>  					  struct winbindd_request *request,
>  					  struct winbindd_response *response);
> +
> +struct winbindd_context *winbindd_ctx_create(void);
> +void winbindd_ctx_free(struct winbindd_context *ctx);
> +
>  #define winbind_env_set() \
>  	(strcmp(getenv(WINBINDD_DONT_ENV)?getenv(WINBINDD_DONT_ENV):"0","1") == 0)
>  
> diff --git a/nsswitch/winbind_struct_protocol.h b/nsswitch/winbind_struct_protocol.h
> index 0dffa4b..f818d3f 100644
> --- a/nsswitch/winbind_struct_protocol.h
> +++ b/nsswitch/winbind_struct_protocol.h
> @@ -5,6 +5,7 @@
>  
>     Copyright (C) Tim Potter 2000
>     Copyright (C) Gerald Carter 2006
> +   Copyright (C) Matthew Newton 2015
>  
>     You are free to use this interface definition in any way you see
>     fit, including without restriction, using this header in your own
> @@ -508,4 +509,12 @@ struct winbindd_response {
>  	} extra_data;
>  };
>  
> +/* Winbind context */
> +
> +struct winbindd_context {
> +	int winbindd_fd;	/* winbind file descriptor */
> +	bool is_privileged;	/* using the privileged socket? */
> +	pid_t our_pid;		/* calling process pid */
> +};

Is it possible to not expose the internals of the struct
here? Just an anonymous

struct winbindd_context;

in winbind_client.h, and the real struct definition where
it's needed?

Volker


More information about the samba-technical mailing list