The branch, v4-18-test has been updated via 82d6f8a6ce3 nsswitch/wb_common.c: fix socket fd and memory leaks of global state via 3d8e8ed1594 nsswitch/wb_common.c: don't operate on a stale wb_global_ctx.key via 5b9b8b31582 nsswitch/wb_common.c: winbind_destructor can always use get_wb_global_ctx() via 0ebaac2afe9 nsswitch/wb_common.c: fix build without HAVE_PTHREAD via cb71db6827f nsswitch: add test for pthread_key_delete missuse (bug 15464) from 5cf6870718c libsmb: Fix test for smbc_getxattr
https://git.samba.org/?p=samba.git;a=shortlog;h=v4-18-test - Log ----------------------------------------------------------------- commit 82d6f8a6ce3918b51a9422101823328084a27ffa Author: Stefan Metzmacher <me...@samba.org> Date: Thu Sep 7 15:59:59 2023 +0200 nsswitch/wb_common.c: fix socket fd and memory leaks of global state When we are called in wb_atfork_child() or winbind_destructor(), wb_thread_ctx_destructor() is not called for the global state of the current nor any other thread, which means we would leak the related memory and socket fds. Now we maintain a global list protected by a global mutex. We traverse the list and close all socket fds, which are no longer used (winbind_destructor) or no longer valid in the current process (wb_atfork_child), in addition we 'autofree' the ones, which are only visible internally as global (per thread) context. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15464 Tested-by: Krzysztof Piotr Oledzki <o...@ans.pl> Signed-off-by: Stefan Metzmacher <me...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> Autobuild-User(master): Stefan Metzmacher <me...@samba.org> Autobuild-Date(master): Thu Sep 14 18:53:07 UTC 2023 on atb-devel-224 (cherry picked from commit 4af3faace481d23869b64485b791bdd43d8972c5) Autobuild-User(v4-18-test): Jule Anger <jan...@samba.org> Autobuild-Date(v4-18-test): Mon Sep 18 17:25:43 UTC 2023 on atb-devel-224 commit 3d8e8ed15942374939c95384b5cd03b0162000ad Author: Stefan Metzmacher <me...@samba.org> Date: Fri Sep 8 09:56:47 2023 +0200 nsswitch/wb_common.c: don't operate on a stale wb_global_ctx.key If nss_winbind is loaded into a process that uses fork multiple times without any further calls into nss_winbind, wb_atfork_child handler was using a wb_global_ctx.key that was no longer registered in the pthread library, so we operated on a slot that was potentially reused by other libraries or the main application. Which is likely to cause memory corruption. So we better don't call pthread_key_delete() in wb_atfork_child(). BUG: https://bugzilla.samba.org/show_bug.cgi?id=15464 Reported-by: Krzysztof Piotr Oledzki <o...@ans.pl> Tested-by: Krzysztof Piotr Oledzki <o...@ans.pl> Signed-off-by: Stefan Metzmacher <me...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> (cherry picked from commit 91b30a7261e6455d3a4f31728c23e4849e3945b9) commit 5b9b8b315821c429ecfcb9153aa5308e3c9f5086 Author: Stefan Metzmacher <me...@samba.org> Date: Fri Sep 8 09:53:42 2023 +0200 nsswitch/wb_common.c: winbind_destructor can always use get_wb_global_ctx() The HAVE_PTHREAD logic inside of get_wb_global_ctx() will do all required magic. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15464 Signed-off-by: Stefan Metzmacher <me...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> (cherry picked from commit 836823e5047d0eb18e66707386ba03b812adfaf8) commit 0ebaac2afe94cf09599970962c66a7cc2761625c Author: Stefan Metzmacher <me...@samba.org> Date: Thu Sep 7 16:02:32 2023 +0200 nsswitch/wb_common.c: fix build without HAVE_PTHREAD BUG: https://bugzilla.samba.org/show_bug.cgi?id=15464 Signed-off-by: Stefan Metzmacher <me...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> (cherry picked from commit 4faf806412c4408db25448b1f67c09359ec2f81f) commit cb71db6827f2575799d65c8a3560e1748a389889 Author: Stefan Metzmacher <me...@samba.org> Date: Fri Sep 8 13:57:26 2023 +0200 nsswitch: add test for pthread_key_delete missuse (bug 15464) This is based on https://bugzilla.samba.org/attachment.cgi?id=18081 written by Krzysztof Piotr Oledzki <o...@ans.pl> BUG: https://bugzilla.samba.org/show_bug.cgi?id=15464 Signed-off-by: Stefan Metzmacher <me...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> (cherry picked from commit 62af25d44e542548d8cdecb061a6001e0071ee76) ----------------------------------------------------------------------- Summary of changes: nsswitch/b15464-testcase.c | 77 +++++++++++++++++ nsswitch/wb_common.c | 152 +++++++++++++++++++++++++--------- nsswitch/wscript_build | 5 ++ source3/selftest/tests.py | 6 ++ testprogs/blackbox/b15464-testcase.sh | 21 +++++ 5 files changed, 222 insertions(+), 39 deletions(-) create mode 100644 nsswitch/b15464-testcase.c create mode 100755 testprogs/blackbox/b15464-testcase.sh Changeset truncated at 500 lines: diff --git a/nsswitch/b15464-testcase.c b/nsswitch/b15464-testcase.c new file mode 100644 index 00000000000..decb474a81e --- /dev/null +++ b/nsswitch/b15464-testcase.c @@ -0,0 +1,77 @@ +#include "replace.h" +#include "system/wait.h" +#include "system/threads.h" +#include <assert.h> + +int main(int argc, const char *argv[]) +{ + pid_t pid; + int wstatus; + pthread_key_t k1; + pthread_key_t k2; + pthread_key_t k3; + char *val = NULL; + const char *nss_winbind = (argc >= 2 ? argv[1] : "bin/plugins/libnss_winbind.so.2"); + void *nss_winbind_handle = NULL; + union { + int (*fn)(void); + void *symbol; + } nss_winbind_endpwent = { .symbol = NULL, }; + + /* + * load and invoke something simple like + * _nss_winbind_endpwent in order to + * get the libnss_winbind internal going + */ + nss_winbind_handle = dlopen(nss_winbind, RTLD_NOW); + printf("%d: nss_winbind[%s] nss_winbind_handle[%p]\n", + getpid(), nss_winbind, nss_winbind_handle); + assert(nss_winbind_handle != NULL); + + nss_winbind_endpwent.symbol = dlsym(nss_winbind_handle, + "_nss_winbind_endpwent"); + printf("%d: nss_winbind_handle[%p] _nss_winbind_endpwent[%p]\n", + getpid(), nss_winbind_handle, nss_winbind_endpwent.symbol); + assert(nss_winbind_endpwent.symbol != NULL); + (void)nss_winbind_endpwent.fn(); + + val = malloc(1); + assert(val != NULL); + + pthread_key_create(&k1, NULL); + pthread_setspecific(k1, val); + printf("%d: k1=%d\n", getpid(), k1); + + pid = fork(); + if (pid) { + free(val); + wait(&wstatus); + return WEXITSTATUS(wstatus); + } + + pthread_key_create(&k2, NULL); + pthread_setspecific(k2, val); + + printf("%d: Hello after fork, k1=%d, k2=%d\n", getpid(), k1, k2); + + pid = fork(); + + if (pid) { + free(val); + wait(&wstatus); + return WEXITSTATUS(wstatus); + } + + pthread_key_create(&k3, NULL); + pthread_setspecific(k3, val); + + printf("%d: Hello after fork2, k1=%d, k2=%d, k3=%d\n", getpid(), k1, k2, k3); + + if (k1 == k2 || k2 == k3) { + printf("%d: FAIL inconsistent keys\n", getpid()); + return 1; + } + + printf("%d: OK consistent keys\n", getpid()); + return 0; +} diff --git a/nsswitch/wb_common.c b/nsswitch/wb_common.c index d569e761ebe..b7f84435a4e 100644 --- a/nsswitch/wb_common.c +++ b/nsswitch/wb_common.c @@ -26,6 +26,7 @@ #include "replace.h" #include "system/select.h" #include "winbind_client.h" +#include "lib/util/dlinklist.h" #include <assert.h> #ifdef HAVE_PTHREAD_H @@ -37,74 +38,113 @@ static __thread char client_name[32]; /* Global context */ struct winbindd_context { + struct winbindd_context *prev, *next; int winbindd_fd; /* winbind file descriptor */ bool is_privileged; /* using the privileged socket? */ pid_t our_pid; /* calling process pid */ + bool autofree; /* this is a thread global context */ }; static struct wb_global_ctx { - bool initialized; #ifdef HAVE_PTHREAD pthread_once_t control; pthread_key_t key; + bool key_initialized; +#ifdef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP +#define WB_GLOBAL_MUTEX_INITIALIZER PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP #else - bool dummy; +#define WB_GLOBAL_MUTEX_INITIALIZER PTHREAD_MUTEX_INITIALIZER #endif +#define WB_GLOBAL_LIST_LOCK do { \ + int __pret = pthread_mutex_lock(&wb_global_ctx.list_mutex); \ + assert(__pret == 0); \ +} while(0) +#define WB_GLOBAL_LIST_UNLOCK do { \ + int __pret = pthread_mutex_unlock(&wb_global_ctx.list_mutex); \ + assert(__pret == 0); \ +} while(0) + pthread_mutex_t list_mutex; +#else /* => not HAVE_PTHREAD */ +#define WB_GLOBAL_LIST_LOCK do { } while(0) +#define WB_GLOBAL_LIST_UNLOCK do { } while(0) +#endif /* not HAVE_PTHREAD */ + struct winbindd_context *list; } wb_global_ctx = { #ifdef HAVE_PTHREAD .control = PTHREAD_ONCE_INIT, + .list_mutex = WB_GLOBAL_MUTEX_INITIALIZER, #endif + .list = NULL, }; static void winbind_close_sock(struct winbindd_context *ctx); +static void winbind_ctx_free_locked(struct winbindd_context *ctx); +static void winbind_cleanup_list(void); #ifdef HAVE_PTHREAD static void wb_thread_ctx_initialize(void); -static void wb_atfork_child(void) +static void wb_atfork_prepare(void) { - struct winbindd_context *ctx = NULL; - int ret; + WB_GLOBAL_LIST_LOCK; +} - ctx = (struct winbindd_context *)pthread_getspecific(wb_global_ctx.key); - if (ctx == NULL) { - return; - } +static void wb_atfork_parent(void) +{ + WB_GLOBAL_LIST_UNLOCK; +} - ret = pthread_setspecific(wb_global_ctx.key, NULL); - assert(ret == 0); +static void wb_atfork_child(void) +{ + wb_global_ctx.list_mutex = (pthread_mutex_t)WB_GLOBAL_MUTEX_INITIALIZER; - winbind_close_sock(ctx); - free(ctx); + if (wb_global_ctx.key_initialized) { + int ret; - ret = pthread_key_delete(wb_global_ctx.key); - assert(ret == 0); + /* + * After a fork the child still believes + * it is the same thread as in the parent. + * So pthread_getspecific() would return the + * value of the thread that called fork(). + * + * But we don't want that behavior, so + * we just clear the reference and let + * winbind_cleanup_list() below 'autofree' + * the parent threads global context. + */ + ret = pthread_setspecific(wb_global_ctx.key, NULL); + assert(ret == 0); + } - wb_global_ctx.control = (pthread_once_t)PTHREAD_ONCE_INIT; + /* + * But we need to close/cleanup the global state + * of the parents threads. + */ + winbind_cleanup_list(); } static void wb_thread_ctx_destructor(void *p) { struct winbindd_context *ctx = (struct winbindd_context *)p; - winbind_close_sock(ctx); - free(ctx); + winbindd_ctx_free(ctx); } static void wb_thread_ctx_initialize(void) { int ret; - ret = pthread_atfork(NULL, - NULL, + ret = pthread_atfork(wb_atfork_prepare, + wb_atfork_parent, wb_atfork_child); assert(ret == 0); ret = pthread_key_create(&wb_global_ctx.key, wb_thread_ctx_destructor); assert(ret == 0); + + wb_global_ctx.key_initialized = true; } -#endif static struct winbindd_context *get_wb_thread_ctx(void) { @@ -129,9 +169,14 @@ static struct winbindd_context *get_wb_thread_ctx(void) *ctx = (struct winbindd_context) { .winbindd_fd = -1, .is_privileged = false, - .our_pid = 0 + .our_pid = 0, + .autofree = true, }; + WB_GLOBAL_LIST_LOCK; + DLIST_ADD_END(wb_global_ctx.list, ctx); + WB_GLOBAL_LIST_UNLOCK; + ret = pthread_setspecific(wb_global_ctx.key, ctx); if (ret != 0) { free(ctx); @@ -139,6 +184,7 @@ static struct winbindd_context *get_wb_thread_ctx(void) } return ctx; } +#endif /* HAVE_PTHREAD */ static struct winbindd_context *get_wb_global_ctx(void) { @@ -147,7 +193,8 @@ static struct winbindd_context *get_wb_global_ctx(void) static struct winbindd_context _ctx = { .winbindd_fd = -1, .is_privileged = false, - .our_pid = 0 + .our_pid = 0, + .autofree = false, }; #endif @@ -155,9 +202,11 @@ static struct winbindd_context *get_wb_global_ctx(void) ctx = get_wb_thread_ctx(); #else ctx = &_ctx; + if (ctx->prev == NULL && ctx->next == NULL) { + DLIST_ADD_END(wb_global_ctx.list, ctx); + } #endif - wb_global_ctx.initialized = true; return ctx; } @@ -231,6 +280,30 @@ static void winbind_close_sock(struct winbindd_context *ctx) } } +static void winbind_ctx_free_locked(struct winbindd_context *ctx) +{ + winbind_close_sock(ctx); + DLIST_REMOVE(wb_global_ctx.list, ctx); + free(ctx); +} + +static void winbind_cleanup_list(void) +{ + struct winbindd_context *ctx = NULL, *next = NULL; + + WB_GLOBAL_LIST_LOCK; + for (ctx = wb_global_ctx.list; ctx != NULL; ctx = next) { + next = ctx->next; + + if (ctx->autofree) { + winbind_ctx_free_locked(ctx); + } else { + winbind_close_sock(ctx); + } + } + WB_GLOBAL_LIST_UNLOCK; +} + /* Destructor for global context to ensure fd is closed */ #ifdef HAVE_DESTRUCTOR_ATTRIBUTE @@ -240,22 +313,18 @@ __attribute__((destructor)) #endif static void winbind_destructor(void) { - struct winbindd_context *ctx; - - if (!wb_global_ctx.initialized) { - return; +#ifdef HAVE_PTHREAD + if (wb_global_ctx.key_initialized) { + int ret; + ret = pthread_key_delete(wb_global_ctx.key); + assert(ret == 0); + wb_global_ctx.key_initialized = false; } -#ifdef HAVE_PTHREAD_H - ctx = (struct winbindd_context *)pthread_getspecific(wb_global_ctx.key); - if (ctx == NULL) { - return; - } -#else - ctx = get_wb_global_ctx(); -#endif + wb_global_ctx.control = (pthread_once_t)PTHREAD_ONCE_INIT; +#endif /* HAVE_PTHREAD */ - winbind_close_sock(ctx); + winbind_cleanup_list(); } #define CONNECT_TIMEOUT 30 @@ -937,11 +1006,16 @@ struct winbindd_context *winbindd_ctx_create(void) ctx->winbindd_fd = -1; + WB_GLOBAL_LIST_LOCK; + DLIST_ADD_END(wb_global_ctx.list, ctx); + WB_GLOBAL_LIST_UNLOCK; + return ctx; } void winbindd_ctx_free(struct winbindd_context *ctx) { - winbind_close_sock(ctx); - free(ctx); + WB_GLOBAL_LIST_LOCK; + winbind_ctx_free_locked(ctx); + WB_GLOBAL_LIST_UNLOCK; } diff --git a/nsswitch/wscript_build b/nsswitch/wscript_build index 3247b6c2b7c..4e62bb4c946 100644 --- a/nsswitch/wscript_build +++ b/nsswitch/wscript_build @@ -15,6 +15,11 @@ if bld.CONFIG_SET('HAVE_PTHREAD'): deps='wbclient pthread', for_selftest=True ) + bld.SAMBA_BINARY('b15464-testcase', + source='b15464-testcase.c', + deps='replace pthread dl', + for_selftest=True + ) # The nss_wrapper code relies strictly on the linux implementation and # name, so compile but do not install a copy under this name. diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 0c834ed48b5..ea17ead3eda 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -67,6 +67,8 @@ except KeyError: samba4bindir = bindir() config_h = os.path.join(samba4bindir, "default/include/config.h") +bbdir = os.path.join(srcdir(), "testprogs/blackbox") + # check available features config_hash = dict() f = open(config_h, 'r') @@ -936,6 +938,10 @@ if with_pthreadpool: [os.path.join(samba3srcdir, "script/tests/test_libwbclient_threads.sh"), "$DOMAIN", "$DC_USERNAME"]) + plantestsuite("b15464_testcase", "none", + [os.path.join(bbdir, "b15464-testcase.sh"), + binpath("b15464-testcase"), + binpath("plugins/libnss_winbind.so.2")]) plantestsuite("samba3.test_nfs4_acl", "none", [os.path.join(bindir(), "test_nfs4_acls"), diff --git a/testprogs/blackbox/b15464-testcase.sh b/testprogs/blackbox/b15464-testcase.sh new file mode 100755 index 00000000000..b0c88260d4c --- /dev/null +++ b/testprogs/blackbox/b15464-testcase.sh @@ -0,0 +1,21 @@ +#!/bin/sh +# Blackbox wrapper for bug 15464 +# Copyright (C) 2023 Stefan Metzmacher + +if [ $# -lt 2 ]; then + cat <<EOF +Usage: b15464-testcase.sh B15464_TESTCASE LIBNSS_WINBIND +EOF + exit 1 +fi + +b15464_testcase=$1 +libnss_winbind=$2 +shift 2 +failed=0 + +. $(dirname $0)/subunit.sh + +testit "run b15464-testcase" $VALGRIND $b15464_testcase $libnss_winbind || failed=$(expr $failed + 1) + +testok $0 $failed -- Samba Shared Repository