[PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list

patches (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
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 <[hidden email]>
> 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!

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
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 <[hidden email]>
> > 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

patches (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
On Wed, Nov 29, 2017 at 12:23:34PM -0700, Christof Schmitt via samba-technical wrote:
> 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.

From a review perspective, this looks good. For the refactoring one of
course RB+. However, pthreadpool being such a fragile piece of code I
would love to see unit tests for this.  This will require explicit
error injects with some DEVELOPER only code, but is it possible to get
this? If you don't have time, I might try to code something up.

On top of your refactoring patch, attached find another pthreadpool
race condition fix. For me it does fix something, but it also
introduces another piece of technical debt. I have to put some more
thought into handling pthread_create failure in the parent atfork
handler. This might require interaction with the users of this code,
don't know yet.

It's running a private autobuild now, running pthreadpooltest manually
gives the expected result: Without the fix it fails, with the fix it
succeeds.

Comments?

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

patch.txt (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
On Thu, Nov 30, 2017 at 01:40:00PM +0100, Volker Lendecke wrote:

> On Wed, Nov 29, 2017 at 12:23:34PM -0700, Christof Schmitt via samba-technical wrote:
> > 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.
>
> From a review perspective, this looks good. For the refactoring one of
> course RB+. However, pthreadpool being such a fragile piece of code I
> would love to see unit tests for this.  This will require explicit
> error injects with some DEVELOPER only code, but is it possible to get
> this? If you don't have time, I might try to code something up.
Yes, see attached patch. I am not sure if that is the best solution, but
it is easy enough. A more complicate approach would be LD_PRELOAD to
overwrite the libc functions.

Note that i also fixed the num_threads += 1 in the first patch to only
run when pthread_create was successful.

> On top of your refactoring patch, attached find another pthreadpool
> race condition fix. For me it does fix something, but it also
> introduces another piece of technical debt. I have to put some more
> thought into handling pthread_create failure in the parent atfork
> handler. This might require interaction with the users of this code,
> don't know yet.
>
> It's running a private autobuild now, running pthreadpooltest manually
> gives the expected result: Without the fix it fails, with the fix it
> succeeds.
>
> Comments?

> From 163df5a06d37eaab665f94a29f22b11bbc17bab6 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <[hidden email]>
> Date: Wed, 29 Nov 2017 16:45:40 +0100
> Subject: [PATCH 1/2] pthreadpool: Fix a race condition
>
> The comment says it all
>
> Signed-off-by: Volker Lendecke <[hidden email]>
> ---
>  lib/pthreadpool/pthreadpool.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c
> index 874ddd23c1d..abf0e8f6f61 100644
> --- a/lib/pthreadpool/pthreadpool.c
> +++ b/lib/pthreadpool/pthreadpool.c
> @@ -103,6 +103,7 @@ static struct pthreadpool *pthreadpools = NULL;
>  static pthread_once_t pthreadpool_atfork_initialized = PTHREAD_ONCE_INIT;
>  
>  static void pthreadpool_prep_atfork(void);
> +static int pthreadpool_create_thread(struct pthreadpool *pool);
>  
>  /*
>   * Initialize a thread pool
> @@ -244,6 +245,27 @@ static void pthreadpool_parent(void)
>       pool = DLIST_PREV(pool)) {
>   ret = pthread_cond_init(&pool->condvar, NULL);
>   assert(ret == 0);
> +
> + if ((pool->num_jobs != 0) && (pool->num_threads == 0)) {
> + /*
> + * We added a job with idle threads and
> + * immediately did a fork before any of the
> + * idle threads got scheduled.
> + *
> + * pthreadpool_prepare killed all helper
> + * threads to get them off pool->condvar, so
> + * we need to create at least one to get
> + * things going again.
> + */
> + ret = pthreadpool_create_thread(pool);
> +
> + /*
> + * TODO: Handle temporary thread creation
> + * failure gracefully.
> + */
> + assert(ret == 0);
Would it make sense to also have a fallback to calling the jobs
synchronously here? I am not sure what else could be done; the system
might be hitting a limit with the number of threads when pthread_create
fails and we should still try to finish the jobs.

Christof

patches (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
On Thursday, 30 November 2017 20:14:35 CET Christof Schmitt via samba-
technical wrote:
> On Thu, Nov 30, 2017 at 01:40:00PM +0100, Volker Lendecke wrote:
> > On Wed, Nov 29, 2017 at 12:23:34PM -0700, Christof Schmitt via samba-
technical wrote:

> > > 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.
> >
> > From a review perspective, this looks good. For the refactoring one of
> > course RB+. However, pthreadpool being such a fragile piece of code I
> > would love to see unit tests for this.  This will require explicit
> > error injects with some DEVELOPER only code, but is it possible to get
> > this? If you don't have time, I might try to code something up.
>
> Yes, see attached patch. I am not sure if that is the best solution, but
> it is easy enough. A more complicate approach would be LD_PRELOAD to
> overwrite the libc functions.

Use mocking support from cmocka to mock pthreadpool_create_thread()?


See https://lwn.net/Articles/558106/



        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
On Mon, Dec 04, 2017 at 01:33:20PM +0100, Andreas Schneider via samba-technical wrote:

> On Thursday, 30 November 2017 20:14:35 CET Christof Schmitt via samba-
> technical wrote:
> > On Thu, Nov 30, 2017 at 01:40:00PM +0100, Volker Lendecke wrote:
> > > On Wed, Nov 29, 2017 at 12:23:34PM -0700, Christof Schmitt via samba-
> technical wrote:
> > > > 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.
> > >
> > > From a review perspective, this looks good. For the refactoring one of
> > > course RB+. However, pthreadpool being such a fragile piece of code I
> > > would love to see unit tests for this.  This will require explicit
> > > error injects with some DEVELOPER only code, but is it possible to get
> > > this? If you don't have time, I might try to code something up.
> >
> > Yes, see attached patch. I am not sure if that is the best solution, but
> > it is easy enough. A more complicate approach would be LD_PRELOAD to
> > overwrite the libc functions.
>
> Use mocking support from cmocka to mock pthreadpool_create_thread()?
>
>
> See https://lwn.net/Articles/558106/
Thanks for the pointer. Mocking the function completely does not work,
as the good path requires creating an actual thread, but the --wrap
option to override the call to pthread_create in the linker is useful.

cmocka looks useful and the pthreadpool tests could be converted to
cmocka. I decided against this approach for now to minimize the changes
and to also allow an easy backport to 4.6 where cmocka is not yet
available.

Christof

patches (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
On Tue, Dec 05, 2017 at 12:24:21PM -0700, Christof Schmitt via samba-technical wrote:

> On Mon, Dec 04, 2017 at 01:33:20PM +0100, Andreas Schneider via samba-technical wrote:
> > On Thursday, 30 November 2017 20:14:35 CET Christof Schmitt via samba-
> > technical wrote:
> > > On Thu, Nov 30, 2017 at 01:40:00PM +0100, Volker Lendecke wrote:
> > > > On Wed, Nov 29, 2017 at 12:23:34PM -0700, Christof Schmitt via samba-
> > technical wrote:
> > > > > 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.
> > > >
> > > > From a review perspective, this looks good. For the refactoring one of
> > > > course RB+. However, pthreadpool being such a fragile piece of code I
> > > > would love to see unit tests for this.  This will require explicit
> > > > error injects with some DEVELOPER only code, but is it possible to get
> > > > this? If you don't have time, I might try to code something up.
> > >
> > > Yes, see attached patch. I am not sure if that is the best solution, but
> > > it is easy enough. A more complicate approach would be LD_PRELOAD to
> > > overwrite the libc functions.
> >
> > Use mocking support from cmocka to mock pthreadpool_create_thread()?
> >
> >
> > See https://lwn.net/Articles/558106/
>
> Thanks for the pointer. Mocking the function completely does not work,
> as the good path requires creating an actual thread, but the --wrap
> option to override the call to pthread_create in the linker is useful.
>
> cmocka looks useful and the pthreadpool tests could be converted to
> cmocka. I decided against this approach for now to minimize the changes
> and to also allow an easy backport to 4.6 where cmocka is not yet
> available.
Another small update. I forgot to clear the inject variable at the end
of the test. That is not relevant for the test added here, but it causes
problems when adding more tests after that.

Christof

patches (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tuesday, 5 December 2017 20:24:21 CET Christof Schmitt wrote:
> On Mon, Dec 04, 2017 at 01:33:20PM +0100, Andreas Schneider via samba-
technical wrote:

> > On Thursday, 30 November 2017 20:14:35 CET Christof Schmitt via samba-
> >
> > technical wrote:
> > > On Thu, Nov 30, 2017 at 01:40:00PM +0100, Volker Lendecke wrote:
> > > > On Wed, Nov 29, 2017 at 12:23:34PM -0700, Christof Schmitt via samba-
> >
> > technical wrote:
> > > > > 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.
> > > >
> > > > From a review perspective, this looks good. For the refactoring one of
> > > > course RB+. However, pthreadpool being such a fragile piece of code I
> > > > would love to see unit tests for this.  This will require explicit
> > > > error injects with some DEVELOPER only code, but is it possible to get
> > > > this? If you don't have time, I might try to code something up.
> > >
> > > Yes, see attached patch. I am not sure if that is the best solution, but
> > > it is easy enough. A more complicate approach would be LD_PRELOAD to
> > > overwrite the libc functions.
> >
> > Use mocking support from cmocka to mock pthreadpool_create_thread()?
> >
> >
> > See https://lwn.net/Articles/558106/
>
> Thanks for the pointer. Mocking the function completely does not work,
> as the good path requires creating an actual thread, but the --wrap
> option to override the call to pthread_create in the linker is useful.

Yes, that's what mocking is for. The test instruments how the mocked function
behaves. So the test can tell the mocked function to call pthread_create() or
return an error.


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
On Wed, Dec 06, 2017 at 08:01:14AM +0100, Andreas Schneider wrote:
> > Thanks for the pointer. Mocking the function completely does not work,
> > as the good path requires creating an actual thread, but the --wrap
> > option to override the call to pthread_create in the linker is useful.
>
> Yes, that's what mocking is for. The test instruments how the mocked function
> behaves. So the test can tell the mocked function to call pthread_create() or
> return an error.

Obviously this is the perfect candidate for cmocka. A small
independent piece of code that needs testing. Yes, I've seen your talk
in Göttingen about cmocka, but still it would be great to get your
support how to use cmocka in this case, now that I have a piece of
code that is contained enough and that needs this. Can you walk us
through adding cmocka here to make pthread_create fail
deterministically?

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Dec 05, 2017 at 04:23:18PM -0700, Christof Schmitt via samba-technical wrote:
> Another small update. I forgot to clear the inject variable at the end
> of the test. That is not relevant for the test added here, but it causes
> problems when adding more tests after that.

Patches look good. RB+ on the first two ones. Regarding the test: Yes,
it's nasty, but do all linkers support the --wrap option? Do we have
to #ifdef this depending on a configure check? If we can't get a
resolution on this in soon, I'd vote to put in the bugfix without the
test with the promise to actually work on the test. I know, famous
last words, but this is important enough to not be held back by a
cmocka vs linker flag discussion.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
On Wed, Dec 06, 2017 at 06:19:04PM +0100, Volker Lendecke wrote:

> On Tue, Dec 05, 2017 at 04:23:18PM -0700, Christof Schmitt via samba-technical wrote:
> > Another small update. I forgot to clear the inject variable at the end
> > of the test. That is not relevant for the test added here, but it causes
> > problems when adding more tests after that.
>
> Patches look good. RB+ on the first two ones. Regarding the test: Yes,
> it's nasty, but do all linkers support the --wrap option? Do we have
> to #ifdef this depending on a configure check? If we can't get a
> resolution on this in soon, I'd vote to put in the bugfix without the
> test with the promise to actually work on the test. I know, famous
> last words, but this is important enough to not be held back by a
> cmocka vs linker flag discussion.

Andreas, please correct me in case i am missing something here:

My understanding is that intercepting the function calls in cmocka
relies on the linker wrap. That is pointed out in the lwn article and
can also be seen in the example provided by cmocka:

https://git.cryptomilk.org/projects/cmocka.git/tree/example/chef_wrap/CMakeLists.txt#n16

The only addition that cmocka then provides is a global data structure
that can be used to pass information from the test to the wrap function.
That data structure is only initialized when a cmocka test fixture is
used. Given that the linker wrap applies to the whole file, this would
require converting all tests to cmocka.

To allow backports, i would suggest to use the approach i submitted in
my patches. That easily applies back to 4.6 where no cmocka is in the
source tree. cmocka support could be added later, independent of the bug
fix here.

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.

Christof

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
On Wednesday, 6 December 2017 21:30:23 CET Christof Schmitt wrote:
> On Wed, Dec 06, 2017 at 06:19:04PM +0100, Volker Lendecke wrote:
> > On Tue, Dec 05, 2017 at 04:23:18PM -0700, Christof Schmitt via samba-
technical wrote:

> > > Another small update. I forgot to clear the inject variable at the end
> > > of the test. That is not relevant for the test added here, but it causes
> > > problems when adding more tests after that.
> >
> > Patches look good. RB+ on the first two ones. Regarding the test: Yes,
> > it's nasty, but do all linkers support the --wrap option? Do we have
> > to #ifdef this depending on a configure check? If we can't get a
> > resolution on this in soon, I'd vote to put in the bugfix without the
> > test with the promise to actually work on the test. I know, famous
> > last words, but this is important enough to not be held back by a
> > cmocka vs linker flag discussion.
>
> Andreas, please correct me in case i am missing something here:
>
> My understanding is that intercepting the function calls in cmocka
> relies on the linker wrap. That is pointed out in the lwn article and
> can also be seen in the example provided by cmocka:
>
> https://git.cryptomilk.org/projects/cmocka.git/tree/example/chef_wrap/CMakeL
> ists.txt#n16

Yes, that's correct.

> The only addition that cmocka then provides is a global data structure
> that can be used to pass information from the test to the wrap function.

Correct.

> That data structure is only initialized when a cmocka test fixture is
> used. Given that the linker wrap applies to the whole file, this would
> require converting all tests to cmocka.

If you want to convert all tests ...

> To allow backports, i would suggest to use the approach i submitted in
> my patches. That easily applies back to 4.6 where no cmocka is in the
> source tree. cmocka support could be added later, independent of the bug
> fix here.
>
> 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.

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.


Cheers,


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
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.

> 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?

Christof

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
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.
>
> > 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.

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
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.

Christof

patches (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
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.

The test looks fine but I think we need a configure check at a prominent place
to see if the linker has the --wrap feature. I know that ld.bfd and ld.gold
do, but I'm not sure on other platforms.


Thanks,


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
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.

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
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

patches (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13170] Fix crash in pthreadpool thread after failure from pthread_create

Samba - samba-technical mailing list
On Thursday, 7 December 2017 20:04:02 CET Christof Schmitt wrote:

> 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.

RB+ for the test :-)


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

12