Re: Segfaults on http_close?

[Eric Wong <e@80x24.org>]

Xiao Yu <xyu@automattic.com> wrote:
> On Fri, Feb 12, 2021 at 6:54 AM Eric Wong <e@80x24.org> wrote:
> >
> > Ping?  It's been a few weeks :>
> 
> Apologies for not getting back to you earlier! I just checked all of
> our logs across our 96 nodes and there's been no segfaults since the
> patch was deployed. I think we can call that fix good and tag a new
> version.
> 
> Thanks again for the quick fix and diagnosing the issue even with the
> limited amount of info I was able to provide.

No problem and thanks for the update!

I've pushed out the final patch as commit f441da11c290373
and will push out a 1.8.1 release in a few.
-------------8<------------
Date: Sat, 13 Feb 2021 01:03:58 +0000
Subject: [PATCH] thrpool: remove stack size changing for all platforms

As compilers and system C libraries change, the using a
non-default stack size is too risky and can lead to
difficult-to-diagnose problems.

Using the default stack size seems to solve the segfaults
at http_close reported by Xiao Yu <xyu@automattic.com>.

Users on modern 64-bit systems were unlikely to find any benefit
in using a small stack size with this code base.
Users on 32-bit systems who wish to continue with a minimal
stack should use "ulimit -s" in startup scripts or configure
their process manager appropriately (e.g. setting the
"LimitSTACK" directive in described in systemd.exec(5)).

Reported-and-tested-by: Xiao Yu <xyu@automattic.com>
Link: https://yhbt.net/cmogstored-public/CABfxMcW+kb5gwq3pSB_89P49EVv+4UkJXz+mUPQTy19AdrwbAg@mail.gmail.com/T/
---
 thrpool.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/thrpool.c b/thrpool.c
index bc67ea0..56a7ef0 100644
--- a/thrpool.c
+++ b/thrpool.c
@@ -12,24 +12,6 @@ struct mog_thr_start_arg {
 	unsigned *do_quit;
 };
 
-/*
- * we can lower this if we can test with lower values, NPTL minimum is 16K.
- * We also use syslog() and *printf() functions which take a lot of
- * stack under glibc, so we'll add BUFSIZ (8192 on glibc) to that
- */
-#if MOG_LIBKQUEUE /* libkqueue uses quite a bit of stack */
-#  define MOG_THR_STACK_SIZE (0)
-#elif defined(__GLIBC__) || defined(__FreeBSD__)
-#  define MOG_THR_STACK_SIZE ((16 * 1024) + MAX(8192,BUFSIZ))
-#  if defined(PTHREAD_STACK_MIN) && (PTHREAD_STACK_MIN > MOG_THR_STACK_SIZE)
-#    undef MOG_THR_STACK_SIZE
-#    define MOG_THR_STACK_SIZE PTHREAD_STACK_MIN
-#  endif
-#else
-#  define MOG_THR_STACK_SIZE (0)
-#endif
-static const size_t stacksize = (size_t)MOG_THR_STACK_SIZE;
-
 static sigset_t quitset;
 
 __attribute__((constructor)) static void thrpool_init(void)
@@ -141,9 +123,6 @@ thrpool_add(struct mog_thrpool *tp, unsigned size, unsigned long *nr_eagain)
 
 	CHECK(int, 0, pthread_attr_init(&attr));
 
-	if (stacksize > 0)
-		CHECK(int, 0, pthread_attr_setstacksize(&attr, stacksize));
-
 	thr = &tp->threads[tp->n_threads].thr;
 
 	CHECK(int, 0, pthread_mutex_lock(&arg.mtx));