The branch, master has been updated
via 4af3faace48 nsswitch/wb_common.c: fix socket fd and memory leaks of
global state
via 91b30a7261e nsswitch/wb_common.c: don't operate on a stale
wb_global_ctx.key
via 836823e5047 nsswitch/wb_common.c: winbind_destructor can always use
get_wb_global_ctx()
via 4faf806412c nsswitch/wb_common.c: fix build without HAVE_PTHREAD
via 62af25d44e5 nsswitch: add test for pthread_key_delete missuse (bug
15464)
via 19fb9a97dff .gitlab-ci: Allow ext4 jobs to run on shared runners
via b1e83b6cede .gitlab-ci: make it explicit that some tests require
ext4/5.15 kernel
via 416ff2c651f .gitlab-ci: restore starting ubuntu2204-samba-o3 for
the default pipeline
from 0f1443d968c smbd: make vfs_stat_fsp() a no-op on fake file-handles
https://git.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit 4af3faace481d23869b64485b791bdd43d8972c5
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
commit 91b30a7261e6455d3a4f31728c23e4849e3945b9
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>
commit 836823e5047d0eb18e66707386ba03b812adfaf8
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>
commit 4faf806412c4408db25448b1f67c09359ec2f81f
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>
commit 62af25d44e542548d8cdecb061a6001e0071ee76
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>
commit 19fb9a97dff2c0222d89a19bc9b0cd27f0306408
Author: Andrew Bartlett <abart...@samba.org>
Date: Wed Sep 6 09:37:19 2023 +1200
.gitlab-ci: Allow ext4 jobs to run on shared runners
At the time of this commit, GitLab shared runners
tagged "gce" were 2x AMD EPYC 7B12 with 8GB ram.
Pair-Programmed-With: Stefan Metzmacher <me...@samba.org>
Signed-off-by: Andrew Bartlett <abart...@samba.org>
Signed-off-by: Stefan Metzmacher <me...@samba.org>
commit b1e83b6cede6ad50e417a6cff583a9ab25f8c980
Author: Stefan Metzmacher <me...@samba.org>
Date: Thu Sep 14 10:42:55 2023 +0200
.gitlab-ci: make it explicit that some tests require ext4/5.15 kernel
This is better then requiring private runners,
as we'll be able to use shared runners for ext4 soon.
Signed-off-by: Stefan Metzmacher <me...@samba.org>
Reviewed-by: Andrew Bartlett <abart...@samba.org>
commit 416ff2c651fcbfae83cdf3b6f3c3317d1c146d3f
Author: Stefan Metzmacher <me...@samba.org>
Date: Wed Sep 13 17:07:34 2023 +0200
.gitlab-ci: restore starting ubuntu2204-samba-o3 for the default pipeline
This got lost in commit bcc22d00569551cfa25851c8c267ec9decc63d21
for ubuntu1804-samba-o3 at the time...
Signed-off-by: Stefan Metzmacher <me...@samba.org>
Reviewed-by: Andrew Bartlett <abart...@samba.org>
-----------------------------------------------------------------------
Summary of changes:
.gitlab-ci-default-runners.yml | 4 +-
.gitlab-ci-main.yml | 75 ++++++++++++-----
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 +++++
7 files changed, 280 insertions(+), 60 deletions(-)
create mode 100644 nsswitch/b15464-testcase.c
create mode 100755 testprogs/blackbox/b15464-testcase.sh
Changeset truncated at 500 lines:
diff --git a/.gitlab-ci-default-runners.yml b/.gitlab-ci-default-runners.yml
index 2dea6e82c49..f73f868d39c 100644
--- a/.gitlab-ci-default-runners.yml
+++ b/.gitlab-ci-default-runners.yml
@@ -23,8 +23,8 @@
#
#
# Our current private runner 'docker', 'samba-ci-private', 'shared' and
-# 'ubuntu1804'. It runs with an ubuntu1804 kernel and privides an ext4
filesystem
-# and similar RAM as the n1-standard-2 runners.
+# 'ubuntu2204'. It runs with an ubuntu2204 kernel (5.15) and provides an
+# ext4 filesystem and similar RAM as the n1-standard-2 runners.
#
.shared_runner_build:
diff --git a/.gitlab-ci-main.yml b/.gitlab-ci-main.yml
index 3925d48e330..9c1ddc69fd5 100644
--- a/.gitlab-ci-main.yml
+++ b/.gitlab-ci-main.yml
@@ -409,41 +409,77 @@ samba-codecheck:
# settings -> CI/CD -> Environment variables
- if: $SUPPORT_PRIVATE_TEST == "yes"
-.needs_samba-def-build-private:
+.needs_ext4_support:
+ # All runners provide an ext4 filesystem
+ #
+ # Note: we don't use
+ # extends: .shared_template_test_only
+ # as that somehow resets the needs section
+ # and generates problems for something
+ # like this (which is used below)
+ #
+ # .needs_samba-SOME-build-ext4:
+ # extends:
+ # - .needs_samba-SOME-build
+ # - .needs_ext4_support
+ #
+ # So we only set stage again instead...
+ stage: test_only
+
+.needs_5_15_kernel:
+ # Our private runners are based on
+ # ubuntu2204 with a 5.15 kernel.
+ #
+ # And they also provide an ext4 filesystem
+ extends: .private_test_only
+
+.needs_samba-def-build-ext4:
extends:
- .needs_samba-def-build
- - .private_test_only
+ - .needs_ext4_support
-.needs_samba-mit-build-private:
+.needs_samba-mit-build-ext4:
extends:
- .needs_samba-mit-build
- - .private_test_only
+ - .needs_ext4_support
-.needs_samba-h5l-build-private:
+.needs_samba-h5l-build-ext4:
extends:
- .needs_samba-h5l-build
- - .private_test_only
+ - .needs_ext4_support
-.needs_samba-without-smb1-build-private:
+.needs_samba-without-smb1-build-5_15:
+ # Currently this doesn't strictly
+ # require a kernel >= 5.15, but only
+ # ext4 support.
+ #
+ # But we want to make sure that
+ # our private runners keep working
+ # and at least do a single job.
+ #
+ # In future we'll be able to run
+ # tests with io_uring in this
+ # setup, which will requires a
+ # 5.15 kernel in order to be useful.
extends:
- .needs_samba-without-smb1-build
- - .private_test_only
+ - .needs_5_15_kernel
-.needs_samba-nt4-build-private:
+.needs_samba-nt4-build-ext4:
extends:
- .needs_samba-nt4-build
- - .private_test_only
+ - .needs_ext4_support
-.needs_samba-no-opath-build-private:
+.needs_samba-no-opath-build-ext4:
extends:
- .needs_samba-no-opath-build
- - .private_test_only
+ - .needs_ext4_support
samba-fileserver:
- extends: .needs_samba-h5l-build-private
+ extends: .needs_samba-h5l-build-ext4
samba-fileserver-without-smb1:
- extends: .needs_samba-without-smb1-build-private
+ extends: .needs_samba-without-smb1-build-5_15
# This is a full build without the AD DC so we test the build with MIT
# Kerberos from the default system (Ubuntu 22.04 at this stage).
@@ -453,19 +489,19 @@ samba-ktest-mit:
extends: .shared_template
samba-ad-dc-1:
- extends: .needs_samba-def-build-private
+ extends: .needs_samba-def-build-ext4
samba-nt4:
- extends: .needs_samba-nt4-build-private
+ extends: .needs_samba-nt4-build-ext4
samba-addc-mit-1:
- extends: .needs_samba-mit-build-private
+ extends: .needs_samba-mit-build-ext4
samba-no-opath1:
- extends: .needs_samba-no-opath-build-private
+ extends: .needs_samba-no-opath-build-ext4
samba-no-opath2:
- extends: .needs_samba-no-opath-build-private
+ extends: .needs_samba-no-opath-build-ext4
# 'pages' is a special job which can publish artifacts in `public` dir to
gitlab pages
pages:
@@ -585,6 +621,7 @@ ubuntu2204-samba-o3:
# (this uses the same variable as autobuild.py)
- if: $AUTOBUILD_SKIP_SAMBA_O3 == "1"
when: never
+ - when: on_success
# All other jobs do not want code coverage.
.samba-o3-template:
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;
--
Samba Shared Repository
