Make winbind client library thread-safe by adding context

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@newtoncomputing.co.uk>
Reviewed-by: Volker Lendecke <vl@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>

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,19 @@
 #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;
+struct winbindd_context {
+	int winbindd_fd;	/* winbind file descriptor */
+	bool is_privileged;	/* using the privileged socket? */
+	pid_t our_pid;		/* calling process pid */
+};
+
+static struct winbindd_context wb_global_ctx = {
+	.winbindd_fd = -1,
+	.is_privileged = 0,
+	.our_pid = 0
+};
 
 /* Free a response structure */
 
@@ -64,15 +74,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 +354,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 ((need_priv != 0) && (is_privileged == 0)) {
-		winbind_close_sock();
+	if (ctx->our_pid != getpid()) {
+		winbind_close_sock(ctx);
+		ctx->our_pid = getpid();
 	}
 
-	if (winbindd_fd != -1) {
-		return winbindd_fd;
+	if ((need_priv != 0) && (ctx->is_privileged == 0)) {
+		winbind_close_sock(ctx);
+	}
+
+	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 +413,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 +438,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 +447,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 +469,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 +479,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 +492,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 +504,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 +531,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 +539,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 +559,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 +573,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 +584,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));
 
 	/* We actually send the pointer value of the extra_data field from
@@ -579,7 +613,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);
@@ -596,7 +630,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;
@@ -616,7 +651,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)
 	{
@@ -627,7 +662,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))
@@ -645,7 +680,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;
 
@@ -657,7 +693,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;
 
@@ -679,38 +715,73 @@ 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 = calloc(1, sizeof(struct winbindd_context));
+
+	if (!ctx) {
+		return NULL;
+	}
+
+	ctx->winbindd_fd = -1;
+
+	return ctx;
+}
+
+void winbindd_ctx_free(struct winbindd_context *ctx)
+{
+	winbind_close_sock(ctx);
+	free(ctx);
+}

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
@@ -28,16 +29,26 @@
 #include "winbind_nss_config.h"
 #include "winbind_struct_protocol.h"
 
+struct winbindd_context;
+
+struct winbindd_context *winbindd_ctx_create(void);
+void winbindd_ctx_free(struct winbindd_context *ctx);
+
 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);
+
 #define winbind_env_set() \
 	(strcmp(getenv(WINBINDD_DONT_ENV)?getenv(WINBINDD_DONT_ENV):"0","1") == 0)
 

source4/torture/winbind/struct_based.c

@@ -29,7 +29,7 @@
 
 #define DO_STRUCT_REQ_REP_EXT(op,req,rep,expected,strict,warnaction,cmt) do { \
 	NSS_STATUS __got, __expected = (expected); \
-	__got = winbindd_request_response(op, req, rep); \
+	__got = winbindd_request_response(NULL, op, req, rep); \
 	if (__got != __expected) { \
 		const char *__cmt = (cmt); \
 		if (strict) { \