[PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create
Christof Schmitt
cs at samba.org
Wed Nov 29 19:23:34 UTC 2017
On Wed, Nov 29, 2017 at 08:31:07AM +0100, Volker Lendecke via samba-technical wrote:
> On Tue, Nov 28, 2017 at 02:28:14PM -0700, Christof Schmitt via samba-technical wrote:
> > From 62a2a4e2afdc5bdfdf7687400b0eed46c833fb07 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/2] pthreadpool: Move creating of thread to new function
> >
> > No functional change, but this simplifies error handling.
>
> Pushed, thanks!
As discussed in a side-thread, my initial patch lets the error surface
back to the client. Here is an updated patch that avoids this issue. If
no thread can be created, but another still exists, let that handle the
request. If no thread exists, then fallback to processing the job
synchronously, as the system might be hitting a resource limit.
Christof
-------------- next part --------------
From 3eb8f77f96e8fc9e8e30d23e62715eee801ab162 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/2] 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 | 77 ++++++++++++++++++++++++-------------------
1 file changed, 43 insertions(+), 34 deletions(-)
diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c
index 23885aa..9138fa2 100644
--- a/lib/pthreadpool/pthreadpool.c
+++ b/lib/pthreadpool/pthreadpool.c
@@ -521,14 +521,54 @@ 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);
+
+ 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 +610,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 1751dd9ca808771d393d9fcdff278c1e04704dcf 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/2] 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 9138fa2..874ddd2 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;
@@ -597,6 +602,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;
}
@@ -612,8 +620,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
More information about the samba-technical
mailing list