On Tue, Sep 21, 2021 at 11:52 AM ste...@eissing.org <ste...@eissing.org> wrote:
>
>
>
> > Am 21.09.2021 um 11:48 schrieb Ruediger Pluem <rpl...@apache.org>:
> >
> > I noticed a crash in process_lingering_close
> >
> > The crash happens here:
> >
> > #0 0x00007f5d4af33b28 in apr_socket_timeout_set (sock=sock@entry=0x0,
> > t=t@entry=2000000) at network_io/unix/sockopt.c:86
> > stat = <optimized out>
> > #1 0x00000000004706a4 in process_lingering_close (cs=0x7f5c88000ed0) at
> > event.c:1466
> > csd = 0x0
> > dummybuf = '\000' <repeats 30216 times>...
> > nbytes = 0
> > rv = <optimized out>
> > q = <optimized out>
> > #2 0x000000000047154b in worker_thread (thd=0x1a31480, dummy=<optimized
> > out>) at event.c:2142
> > csd = 0x7f5c88000cd0
> > cs = 0x0
> > te = 0x0
> > ptrans = 0x7f5c88000c58
> > ti = <optimized out>
> > process_slot = 3
> > thread_slot = 1
> > rv = 0
> > is_idle = 0
> > #3 0x00007f5d4a48015a in start_thread () from /lib64/libpthread.so.0
> > No symbol table info available.
> > #4 0x00007f5d49fadf73 in clone () from /lib64/libc.so.6
> >
> >
> > The reason is that in this case a third party module that has hooked into
> > the pre_connection hook returns an error which causes
> > the hook to stop running. This prevents core_pre_connection from running
> > which is APR_HOOK_REALLY_LAST.
> > Hence the socket is not set into c->conn_config and hence
> > ap_get_conn_socket returns NULL.
> > The quick fix that prevents that from happening is
> >
> > Index: server/mpm/event/event.c
> > ===================================================================
> > --- server/mpm/event/event.c (revision 1893352)
> > +++ server/mpm/event/event.c (working copy)
> > @@ -1040,6 +1040,16 @@
> > ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, APLOGNO(00469)
> > "process_socket: connection aborted");
> > c->aborted = 1;
> > + /*
> > + * In case we errored, the pre_connection hook of the core
> > + * module maybe did not run (it is APR_HOOK_REALLY_LAST) and
> > + * hence the socket might not have been set in c->conn_config
> > + * and the lingering close process cannot fetch it from there
> > + * via ap_get_conn_socket. Hence set it in this case.
> > + */
> > + if (!ap_get_conn_socket(c)) {
> > + ap_set_core_module_config(c->conn_config, sock);
> > + }
> > }
> >
> > /**
> >
> >
> > But I think it is incomplete. I think we should do all that
> > core_pre_connection does.
> > We cannot call it from event.c as it is a static in core.c. If we agree
> > that we want to execute what it does I see two ways forward:
> >
> > 1. Make core_pre_connection part of the public API and have the
> > pre_connection hook of core just call it.
> > 2. Create a wrapper around ap_run_pre_connection as a public API and if
> > ap_run_pre_connection returns != OK && != DONE do
> > things like setting c->aborted to 1 and call core_pre_connection or do a
> > subset of it.
> >
> > I would be in favour of 2. as 1. seems to provide an API function for only
> > a very specific case and of limited use. 2. seems to
> > deliver a more "save" version of ap_run_pre_connection that could be used
> > in several locations as a drop in replacement for
> > ap_run_pre_connection.
>
> +1 for approach 2
+1
