[PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create
Christof Schmitt
cs at samba.org
Thu Dec 7 19:04:02 UTC 2017
On Thu, Dec 07, 2017 at 08:11:32AM +0100, Andreas Schneider wrote:
> On Wednesday, 6 December 2017 23:15:21 CET Christof Schmitt wrote:
> > On Wed, Dec 06, 2017 at 09:47:10PM +0100, Andreas Schneider wrote:
> > > On Wednesday, 6 December 2017 21:42:54 CET Christof Schmitt wrote:
> > > > On Wed, Dec 06, 2017 at 09:38:02PM +0100, Andreas Schneider wrote:
> > > > > > If we decide to go with cmocka anyway, the best approach would be to
> > > > > > put
> > > > > > the new test in a new file and implement it completely with cmocka.
> > > > > > Existing tests could then be converted later.
> > > > >
> > > > > I thought that we would go that way for master. Just write that one
> > > > > test
> > > > > using cmocka. We could do the backport without the test for Samba 4.6,
> > > > > Samba 4.7 includes cmocka in third_party.
> > > >
> > > > Ok, let me work on this approach then.
> >
> > See attached patches.
> >
> > > > > If we want to use cmocka for every test we need to provide a way for
> > > > > mutexes without linking cmocka to a threading library. cmocka only
> > > > > requires libc and it should stay that way. However we could create it
> > > > > in
> > > > > such a way that you can set functions for locking which called at the
> > > > > right positions in cmocka.
> > > >
> > > > I am not sure what you mean here. pthreadpool uses mutexes and threads
> > > > internally and links against pthreads. Is that a problem for cmocka?
> > >
> > > Normally not. But in case there is an issue we have:
> > >
> > > CMOCKA_TEST_ABORT=1 ./mytest
> > >
> > >
> > > For debugging.
> >
> > Thank you. I have not seen a problem so far.
>
> Also you should set:
>
> cmocka_set_message_output(CM_OUTPUT_SUBUNIT);
>
> Then you can run it directly without the shell script.
See attached patches that include both changes: The additional check for
the linker flag and avoiding the shell script by having the test output
the subunit format directly.
Christof
-------------- next part --------------
From 3addce5db0504bc9850fc29b8fa77a327ea20a70 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Tue, 28 Nov 2017 10:49:36 -0700
Subject: [PATCH 1/4] pthreadpool: Move creating of thread to new function
No functional change, but this simplifies error handling.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13170
Signed-off-by: Christof Schmitt <cs at samba.org>
---
lib/pthreadpool/pthreadpool.c | 79 ++++++++++++++++++++++++-------------------
1 file changed, 45 insertions(+), 34 deletions(-)
diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c
index 23885aa..4e1e5d4 100644
--- a/lib/pthreadpool/pthreadpool.c
+++ b/lib/pthreadpool/pthreadpool.c
@@ -521,14 +521,56 @@ static void *pthreadpool_server(void *arg)
}
}
-int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
- void (*fn)(void *private_data), void *private_data)
+static int pthreadpool_create_thread(struct pthreadpool *pool)
{
pthread_attr_t thread_attr;
pthread_t thread_id;
int res;
sigset_t mask, omask;
+ /*
+ * Create a new worker thread. It should not receive any signals.
+ */
+
+ sigfillset(&mask);
+
+ res = pthread_attr_init(&thread_attr);
+ if (res != 0) {
+ return res;
+ }
+
+ res = pthread_attr_setdetachstate(
+ &thread_attr, PTHREAD_CREATE_DETACHED);
+ if (res != 0) {
+ pthread_attr_destroy(&thread_attr);
+ return res;
+ }
+
+ res = pthread_sigmask(SIG_BLOCK, &mask, &omask);
+ if (res != 0) {
+ pthread_attr_destroy(&thread_attr);
+ return res;
+ }
+
+ res = pthread_create(&thread_id, &thread_attr, pthreadpool_server,
+ (void *)pool);
+
+ assert(pthread_sigmask(SIG_SETMASK, &omask, NULL) == 0);
+
+ pthread_attr_destroy(&thread_attr);
+
+ if (res == 0) {
+ pool->num_threads += 1;
+ }
+
+ return res;
+}
+
+int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
+ void (*fn)(void *private_data), void *private_data)
+{
+ int res;
+
res = pthread_mutex_lock(&pool->mutex);
if (res != 0) {
return res;
@@ -570,43 +612,12 @@ int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
return 0;
}
- /*
- * Create a new worker thread. It should not receive any signals.
- */
-
- sigfillset(&mask);
-
- res = pthread_attr_init(&thread_attr);
- if (res != 0) {
- pthread_mutex_unlock(&pool->mutex);
- return res;
- }
-
- res = pthread_attr_setdetachstate(
- &thread_attr, PTHREAD_CREATE_DETACHED);
+ res = pthreadpool_create_thread(pool);
if (res != 0) {
- pthread_attr_destroy(&thread_attr);
- pthread_mutex_unlock(&pool->mutex);
- return res;
- }
-
- res = pthread_sigmask(SIG_BLOCK, &mask, &omask);
- if (res != 0) {
- pthread_attr_destroy(&thread_attr);
pthread_mutex_unlock(&pool->mutex);
return res;
}
- res = pthread_create(&thread_id, &thread_attr, pthreadpool_server,
- (void *)pool);
- if (res == 0) {
- pool->num_threads += 1;
- }
-
- assert(pthread_sigmask(SIG_SETMASK, &omask, NULL) == 0);
-
- pthread_attr_destroy(&thread_attr);
-
pthread_mutex_unlock(&pool->mutex);
return res;
}
--
1.8.3.1
From 927d6f69a1177d8ade815b0256370a140b79d695 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Tue, 28 Nov 2017 10:59:06 -0700
Subject: [PATCH 2/4] pthreadpool: Undo put_job when returning error
When an error is returned to the caller of pthreadpool_add_job, the job
should not be kept in the internal job array. Otherwise the caller might
free the data structure and a later worker thread would still reference
it.
When it is not possible to create a single worker thread, the system
might be out of resources or hitting a configured limit. In this case
fall back to calling the job function synchronously instead of raising
the error to the caller and possibly back to the SMB client.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13170
Signed-off-by: Christof Schmitt <cs at samba.org>
---
lib/pthreadpool/pthreadpool.c | 28 ++++++++++++++++++++++++++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c
index 4e1e5d4..309aba9 100644
--- a/lib/pthreadpool/pthreadpool.c
+++ b/lib/pthreadpool/pthreadpool.c
@@ -429,6 +429,11 @@ static bool pthreadpool_put_job(struct pthreadpool *p,
return true;
}
+static void pthreadpool_undo_put_job(struct pthreadpool *p)
+{
+ p->num_jobs -= 1;
+}
+
static void *pthreadpool_server(void *arg)
{
struct pthreadpool *pool = (struct pthreadpool *)arg;
@@ -599,6 +604,9 @@ int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
* We have idle threads, wake one.
*/
res = pthread_cond_signal(&pool->condvar);
+ if (res != 0) {
+ pthreadpool_undo_put_job(pool);
+ }
pthread_mutex_unlock(&pool->mutex);
return res;
}
@@ -614,8 +622,24 @@ int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
res = pthreadpool_create_thread(pool);
if (res != 0) {
- pthread_mutex_unlock(&pool->mutex);
- return res;
+ if (pool->num_threads == 0) {
+ /*
+ * No thread could be created to run job,
+ * fallback to sync call.
+ */
+ pthreadpool_undo_put_job(pool);
+ pthread_mutex_unlock(&pool->mutex);
+
+ fn(private_data);
+ return pool->signal_fn(job_id, fn, private_data,
+ pool->signal_fn_private_data);
+ }
+
+ /*
+ * At least one thread is still available, let
+ * that one run the queued job.
+ */
+ res = 0;
}
pthread_mutex_unlock(&pool->mutex);
--
1.8.3.1
From d0009ada23dfff6d1f90da1ca8275ac87f0f43c0 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 7 Dec 2017 10:42:30 -0700
Subject: [PATCH 3/4] wscript: Add check for --wrap linker flag
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13170
Signed-off-by: Christof Schmitt <cs at samba.org>
---
wscript | 3 +++
1 file changed, 3 insertions(+)
diff --git a/wscript b/wscript
index 35cb9d1..ab06835 100644
--- a/wscript
+++ b/wscript
@@ -154,6 +154,9 @@ def configure(conf):
else:
conf.define('USING_SYSTEM_CMOCKA', 1)
+ if conf.CHECK_LDFLAGS(['-Wl,--wrap=test']):
+ conf.env['HAVE_LDWRAP'] = True
+ conf.define('HAVE_LDWRAP', 1)
conf.RECURSE('lib/ldb')
--
1.8.3.1
From 3f268e14a38b9fea28468dd703641d5eb95f3c9d Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 6 Dec 2017 15:10:23 -0700
Subject: [PATCH 4/4] pthreadpool: Add test for pthread_create failure
This is implemented using cmocka and the __wrap override for
pthread_create.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13170
Signed-off-by: Christof Schmitt <cs at samba.org
---
lib/pthreadpool/tests_cmocka.c | 176 +++++++++++++++++++++++++++++++++++++++++
lib/pthreadpool/wscript_build | 7 ++
source3/selftest/tests.py | 7 ++
3 files changed, 190 insertions(+)
create mode 100644 lib/pthreadpool/tests_cmocka.c
diff --git a/lib/pthreadpool/tests_cmocka.c b/lib/pthreadpool/tests_cmocka.c
new file mode 100644
index 0000000..75a935f
--- /dev/null
+++ b/lib/pthreadpool/tests_cmocka.c
@@ -0,0 +1,176 @@
+/*
+ * Unix SMB/CIFS implementation.
+ * cmocka tests for thread pool implementation
+ * Copyright (C) Christof Schmitt 2017
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <errno.h>
+#include <pthread.h>
+#include <setjmp.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <talloc.h>
+#include <tevent.h>
+#include <pthreadpool_tevent.h>
+
+#include <cmocka.h>
+
+struct pthreadpool_tevent_test {
+ struct tevent_context *ev;
+ struct pthreadpool_tevent *pool;
+};
+
+static int setup_pthreadpool_tevent(void **state)
+{
+ struct pthreadpool_tevent_test *t;
+ int ret;
+
+ t = talloc(NULL, struct pthreadpool_tevent_test);
+ assert_non_null(t);
+
+ t->ev = tevent_context_init(t);
+ assert_non_null(t->ev);
+
+ ret = pthreadpool_tevent_init(t->ev, 0, &t->pool);
+ assert_return_code(ret, 0);
+
+ *state = t;
+
+ return 0;
+}
+
+static int teardown_pthreadpool_tevent(void **state)
+{
+ struct pthreadpool_tevent_test *t = *state;
+
+ TALLOC_FREE(t);
+
+ return 0;
+}
+
+int __wrap_pthread_create(pthread_t *thread, const pthread_attr_t *attr,
+ void *(*start_routine) (void *), void *arg);
+int __real_pthread_create(pthread_t *thread, const pthread_attr_t *attr,
+ void *(*start_routine) (void *), void *arg);
+
+int __wrap_pthread_create(pthread_t *thread, const pthread_attr_t *attr,
+ void *(*start_routine) (void *), void *arg)
+{
+ int error;
+
+ error = mock_type(int);
+ if (error != 0) {
+ return error;
+ }
+
+ return __real_pthread_create(thread, attr, start_routine, arg);
+}
+
+static void test_job_threadid(void *ptr)
+{
+ pthread_t *threadid = ptr;
+
+ *threadid = pthread_self();
+}
+
+static int test_create_do(struct tevent_context *ev,
+ struct pthreadpool_tevent *pool,
+ bool *in_main_thread)
+{
+ struct tevent_req *req;
+ pthread_t main_thread, worker_thread;
+ bool ok;
+ int ret;
+
+ main_thread = pthread_self();
+
+ req = pthreadpool_tevent_job_send(
+ ev, ev, pool, test_job_threadid, &worker_thread);
+ if (req == NULL) {
+ fprintf(stderr, "pthreadpool_tevent_job_send failed\n");
+ TALLOC_FREE(ev);
+ return ENOMEM;
+ }
+
+ ok = tevent_req_poll(req, ev);
+ if (!ok) {
+ ret = errno;
+ fprintf(stderr, "tevent_req_poll failed: %s\n",
+ strerror(ret));
+ TALLOC_FREE(ev);
+ return ret;
+ }
+
+
+ ret = pthreadpool_tevent_job_recv(req);
+ TALLOC_FREE(req);
+ if (ret != 0) {
+ fprintf(stderr, "tevent_req_recv failed: %s\n",
+ strerror(ret));
+ TALLOC_FREE(ev);
+ return ret;
+ }
+ *in_main_thread = pthread_equal(worker_thread, main_thread);
+
+ return 0;
+}
+
+static void test_create(void **state)
+{
+ struct pthreadpool_tevent_test *t = *state;
+ bool in_main_thread;
+ int ret;
+
+ /*
+ * When pthreadpool cannot create the first worker thread,
+ * this job will run in the sync fallback in the main thread.
+ */
+ will_return(__wrap_pthread_create, EAGAIN);
+ ret = test_create_do(t->ev, t->pool, &in_main_thread);
+ assert_return_code(ret, 0);
+ assert_true(in_main_thread);
+
+ /*
+ * When a thread can be created, the job will run in the worker thread.
+ */
+ will_return(__wrap_pthread_create, 0);
+ ret = test_create_do(t->ev, t->pool, &in_main_thread);
+ assert_return_code(ret, 0);
+ assert_false(in_main_thread);
+
+ /*
+ * Workerthread will still be active for a second; immediately
+ * running another job will also use the worker thread, even
+ * if a new thread cannot be created.
+ */
+ ret = test_create_do(t->ev, t->pool, &in_main_thread);
+ assert_return_code(ret, 0);
+ assert_false(in_main_thread);
+}
+
+int main(int argc, char **argv)
+{
+ const struct CMUnitTest tests[] = {
+ cmocka_unit_test_setup_teardown(test_create,
+ setup_pthreadpool_tevent,
+ teardown_pthreadpool_tevent),
+ };
+
+ cmocka_set_message_output(CM_OUTPUT_SUBUNIT);
+
+ return cmocka_run_group_tests(tests, NULL, NULL);
+}
diff --git a/lib/pthreadpool/wscript_build b/lib/pthreadpool/wscript_build
index d530463..57df255 100644
--- a/lib/pthreadpool/wscript_build
+++ b/lib/pthreadpool/wscript_build
@@ -21,3 +21,10 @@ bld.SAMBA_BINARY('pthreadpooltest',
deps='PTHREADPOOL',
enabled=bld.env.WITH_PTHREADPOOL,
install=False)
+
+bld.SAMBA_BINARY('pthreadpooltest_cmocka',
+ source='tests_cmocka.c',
+ deps='PTHREADPOOL cmocka',
+ ldflags='-Wl,--wrap=pthread_create',
+ enabled=bld.env.WITH_PTHREADPOOL and bld.env['HAVE_LDWRAP'],
+ install=False)
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 5b12355..b95c2bc 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -57,6 +57,9 @@ finally:
have_libarchive = ("HAVE_LIBARCHIVE" in config_hash)
have_linux_kernel_oplocks = ("HAVE_KERNEL_OPLOCKS_LINUX" in config_hash)
have_inotify = ("HAVE_INOTIFY" in config_hash)
+have_ldwrap = ("HAVE_LDWRAP" in config_hash)
+with_pthreadpool = ("WITH_PTHREADPOOL" in config_hash)
+
plantestsuite("samba3.blackbox.success", "nt4_dc:local", [os.path.join(samba3srcdir, "script/tests/test_success.sh")])
plantestsuite("samba3.blackbox.failure", "nt4_dc:local", [os.path.join(samba3srcdir, "script/tests/test_failure.sh")])
@@ -331,6 +334,10 @@ plantestsuite(
"samba3.pthreadpool", "nt4_dc",
[os.path.join(samba3srcdir, "script/tests/test_pthreadpool.sh")])
+if with_pthreadpool and have_ldwrap:
+ plantestsuite("samba3.pthreadpool_cmocka", "nt4_dc",
+ [os.path.join(bindir(), "pthreadpooltest_cmocka")])
+
plantestsuite("samba3.async_req", "nt4_dc",
[os.path.join(samba3srcdir, "script/tests/test_async_req.sh")])
--
1.8.3.1
More information about the samba-technical
mailing list