Re: Broken: apache/httpd#1667 (2.4.x - 59393dd)

[Nikita Popov]

Thanks for the explanation!

On Fri, Jun 4, 2021 at 12:00 PM Ruediger Pluem  wrote:
>
>
>
> On 6/4/21 8:35 AM, Nikita Popov wrote:
> > Is it ok that we include an os-specific API into the main code, not
> > into the APR project?
>
> You mean because the patch uses pthread_sigmask / sigprocmask?
> IMHO this is ok as:
>
> 1. Prefork is not OS generic.
> 2. APR currently does not offer what we need here. Getting an appropriate API 
> in a stable version of APR would require
>to release a new minor version of APR (1.8.x) first.
> 3. It would require raising the minimum version for APR in httpd to 1.8. This 
> might be fine for trunk, but should not be
>done for 2.4.x where we would like to backport this fix to.
>
> Regards
>
> Rüdiger
>

Re: Broken: apache/httpd#1667 (2.4.x - 59393dd)

[Ruediger Pluem]

On 6/4/21 8:35 AM, Nikita Popov wrote:
> Is it ok that we include an os-specific API into the main code, not
> into the APR project?

You mean because the patch uses pthread_sigmask / sigprocmask?
IMHO this is ok as:

1. Prefork is not OS generic.
2. APR currently does not offer what we need here. Getting an appropriate API 
in a stable version of APR would require
   to release a new minor version of APR (1.8.x) first.
3. It would require raising the minimum version for APR in httpd to 1.8. This 
might be fine for trunk, but should not be
   done for 2.4.x where we would like to backport this fix to.

Regards

Rüdiger

Re: Broken: apache/httpd#1667 (2.4.x - 59393dd)

[Nikita Popov]

Is it ok that we include an os-specific API into the main code, not
into the APR project?

On Fri, Jun 4, 2021 at 9:53 AM Ruediger Pluem  wrote:
>
>
>
> On 6/3/21 10:36 PM, Yann Ylavic wrote:
> > On Thu, Jun 3, 2021 at 8:27 PM Ruediger Pluem  wrote:
> >>
> >> Looking at the stack trace, would we need to call 
> >> apr_setup_signal_thread() (if we have APR_THREADS) before we run
> >> ap_run_child_init and unblock all signals afterwards in case one of the 
> >> init child hooks creates threads like mod_watchdog does?
> >
> > Agreed, what about the attached patch?
>
> Looks good to me.
>
> Regards
>
> Rüdiger
>

Re: Broken: apache/httpd#1667 (2.4.x - 59393dd)

[Ruediger Pluem]

On 6/3/21 10:36 PM, Yann Ylavic wrote:
> On Thu, Jun 3, 2021 at 8:27 PM Ruediger Pluem  wrote:
>>
>> Looking at the stack trace, would we need to call apr_setup_signal_thread() 
>> (if we have APR_THREADS) before we run
>> ap_run_child_init and unblock all signals afterwards in case one of the init 
>> child hooks creates threads like mod_watchdog does?
> 
> Agreed, what about the attached patch?

Looks good to me.

Regards

Rüdiger

Re: Broken: apache/httpd#1667 (2.4.x - 59393dd)

[Yann Ylavic]

On Thu, Jun 3, 2021 at 8:27 PM Ruediger Pluem  wrote:
>
> Looking at the stack trace, would we need to call apr_setup_signal_thread() 
> (if we have APR_THREADS) before we run
> ap_run_child_init and unblock all signals afterwards in case one of the init 
> child hooks creates threads like mod_watchdog does?

Agreed, what about the attached patch?

Regards;
Yann.
Index: server/mpm/prefork/prefork.c
===
--- server/mpm/prefork/prefork.c	(revision 1890421)
+++ server/mpm/prefork/prefork.c	(working copy)
@@ -376,11 +376,23 @@ static void stop_listening(int sig)
 static int requests_this_child;
 static int num_listensocks = 0;
 
+#if APR_HAS_THREADS
+static void child_sigmask(sigset_t *new_mask, sigset_t *old_mask)
+{
+#if defined(SIGPROCMASK_SETS_THREAD_MASK)
+sigprocmask(SIG_SETMASK, new_mask, old_mask);
+#else
+pthread_sigmask(SIG_SETMASK, new_mask, old_mask);
+#endif
+}
+#endif
+
 static void child_main(int child_num_arg, int child_bucket)
 {
 #if APR_HAS_THREADS
 apr_thread_t *thd = NULL;
 apr_os_thread_t osthd;
+sigset_t sig_mask;
 #endif
 apr_pool_t *ptrans;
 apr_allocator_t *allocator;
@@ -446,8 +458,30 @@ static void child_main(int child_num_arg, int chil
 clean_child_exit(APEXIT_CHILDFATAL);
 }
 
+#if APR_HAS_THREADS
+/* Save the signal mask and Use apr_setup_signal_thread to block all
+ * the signals from being received by threads potentially created by
+ * child_init() hooks.
+ */
+child_sigmask(NULL, _mask);
+{
+apr_status_t rv;
+rv = apr_setup_signal_thread();
+if (rv != APR_SUCCESS) {
+ap_log_error(APLOG_MARK, APLOG_EMERG, rv, ap_server_conf, APLOGNO()
+ "Couldn't initialize signal thread");
+clean_child_exit(APEXIT_CHILDFATAL);
+}
+}
+#endif /* APR_HAS_THREADS */
+
 ap_run_child_init(pchild, ap_server_conf);
 
+#if APR_HAS_THREADS
+/* Restore the original signal mask in the main child thread. */
+child_sigmask(_mask, NULL);
+#endif
+
 ap_create_sb_handle(, pchild, my_child_num, 0);
 
 (void) ap_update_child_status(sbh, SERVER_READY, (request_rec *) NULL);

Re: Broken: apache/httpd#1667 (2.4.x - 59393dd)

[Ruediger Pluem]

On 6/3/21 7:23 PM, Travis CI wrote:
> apache
> 
> /
> 
> httpd
> 
> 
> 
> branch icon2.4.x 
> 
> build has failed
> Build #1667 was broken 
> 
> arrow to build time
> clock icon19 mins and 23 secs
> 
> Eric Covener avatarEric Covener
> 
> 59393dd CHANGESET → 
> 
> 
> correct CVE year
> 

Looking at the stack trace, would we need to call apr_setup_signal_thread() (if 
we have APR_THREADS) before we run
ap_run_child_init and unblock all signals afterwards in case one of the init 
child hooks creates threads like mod_watchdog does?

Thread 2 (Thread 0x7f4c98b2cbc0 (LWP 6768)):
#0  run_cleanups (cref=0x55753eaa5930) at memory/unix/apr_pools.c:2704
#1  pool_clear_debug (pool=pool@entry=0x55753eaa58a0, file_line=0x7f4c971a9eda 
"prefork.c:227") at memory/unix/apr_pools.c:1839
#2  0x7f4c982540e9 in pool_destroy_debug (pool=0x55753eaa58a0, 
file_line=) at memory/unix/apr_pools.c:1937
#3  0x7f4c98254960 in apr_pool_destroy_debug (pool=0x55753eaa58a0, 
file_line=0x7f4c971a9eda "prefork.c:227") at
memory/unix/apr_pools.c:1986
#4  0x7f4c971a825a in clean_child_exit (code=code@entry=0) at prefork.c:227
#5  0x7f4c971a829b in just_die (sig=) at prefork.c:355
#6  
#7  0x7f4c97d36cb9 in poll () from /lib/x86_64-linux-gnu/libc.so.6
#8  0x7f4c9825e24b in poll (__timeout=2000, __nfds=1, __fds=0x7ffd22f53c90) 
at /usr/include/x86_64-linux-gnu/bits/poll2.h:46
#9  apr_wait_for_io_or_timeout (f=f@entry=0x0, s=s@entry=0x55753e85a9a0, 
for_read=for_read@entry=1) at support/unix/waitio.c:51
#10 0x7f4c9825796a in apr_socket_recv (sock=sock@entry=0x55753e85a9a0, 
buf=buf@entry=0x7ffd22f53cf0 "\220\063\310>uU",
len=len@entry=0x7ffd22f53ce8) at network_io/unix/sendrecv.c:87
#11 0x55753da86db4 in ap_lingering_close (c=c@entry=0x55753e8879f0) at 
connection.c:182
#12 0x7f4c971a883d in child_main (child_num_arg=child_num_arg@entry=6, 
child_bucket=child_bucket@entry=0) at prefork.c:616
#13 0x7f4c971a8b34 in make_child (s=0x55753eb0a1a0, slot=6) at prefork.c:717
#14 0x7f4c971a958e in perform_idle_server_maintenance (p=) 
at prefork.c:821
#15 prefork_run (_pconf=, plog=, s=) at prefork.c:1014
#16 0x55753da5e9fe in ap_run_mpm (pconf=0x55753e703bb0, 
plog=0x55753e732c10, s=0x55753eb0a1a0) at mpm_common.c:94
#17 0x55753da568d5 in main (argc=, argv=) at 
main.c:819

Thread 1 (Thread 0x7f4c7a55d700 (LWP 6769)):
#0  0x7f4c7fe70be0 in ?? () from /lib/x86_64-linux-gnu/libgpg-error.so.0
#1  0x7f4c97c65161 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x7f4c97c6525a in exit () from /lib/x86_64-linux-gnu/libc.so.6
#3  0x7f4c971a827a in clean_child_exit (code=code@entry=0) at prefork.c:236
#4  0x7f4c971a829b in just_die (sig=) at prefork.c:355
#5  
#6  0x7f4c97d38e1f in select () from /lib/x86_64-linux-gnu/libc.so.6
#7  0x7f4c9825ff75 in apr_sleep (t=t@entry=10) at time/unix/time.c:246
#8  0x7f4c8f625e58 in wd_worker (thread=, 
data=0x55753ec281f0) at mod_watchdog.c:159
#9  0x7f4c9825f53f in dummy_worker (opaque=0x55753eb5c160) at 
threadproc/unix/thread.c:147
#10 0x7f4c9801a6db in start_thread () from 
/lib/x86_64-linux-gnu/libpthread.so.0
#11 0x7f4c97d4371f in clone () from /lib/x86_64-linux-gnu/libc.so.6

Regards

Rüdiger