Re: svn commit: r1876674 - in /httpd/httpd/trunk: include/ap_mmn.h include/http_protocol.h modules/http2/h2_request.c server/protocol.c

[Yann Ylavic]

On Fri, Apr 17, 2020 at 9:59 PM Ruediger Pluem
>
> On 4/17/20 6:47 PM, ylavic
> >
> > -if (r->status != HTTP_OK) {
> > +if (!ap_check_request_header(r)) {
> >  access_status = r->status;
> >  goto die_early;
>
> This means we now die_early where we did not before and may not reply with 
> custom error pages configured for the vhost.

See my reply for r1876664, die_early will also call ap_die() finally
so custom error pages are still in the place.

There are two semantic changes compared to before r1876664 (the first
commit) though, first there are cases where we previously called
ap_send_error_response() only while now we ap_die() always (almost).
It did not make sense to me to call ap_die() on read_request_line()
failure but not later, if ErrorDocument is relevant with request line
errors I think it is for later ones.
Second, this commit (r1876674) checks the Expect header in
ap_check_request_header(), that is before post_read_request hooks,
while we cheched (and failed) after that before. I think this change
makes sense too.

Regards,
Yann.

Re: svn commit: r1876664 - in /httpd/httpd/trunk: modules/http2/h2_request.c server/protocol.c

[Yann Ylavic]

On Fri, Apr 17, 2020 at 9:43 PM Ruediger Pluem
>
> On 4/17/20 3:07 PM, ylavic
> >
> > -apr_brigade_destroy(tmp_bb);
>
> Why don't we destroy it any longer?

tmp_bb is reused below in the die_early code, and always _cleanup()
after use in ap_read_request().
If the brigade is not used anymore, there is no real advantage in
using _destroy() over _cleanup() IMHO, the former will unregister the
cleanup callback (thus walk the list) but the callback is a noop after
apr_brigade_cleanup() anyway..

>
> > +if (r->status != HTTP_OK) {
> > +access_status = r->status;
> > +goto die_early;
>
> Why do we die_early here and not just die later?

While both "die_early" and "die" labels finally call ap_die(), the
former is branched only before we call any hook, thus before anything
can call ap_die() by itself. It matters because ap_die() can recurse,
so we should set r->status = HTTP_OK only for the first call.

But thinking more about it, die_early is also cleaning up the input
filters (for potential further reads to always return APR_EOF), and
this may be suitable only for the case where we could not read the
request line or the headers.
So possibly the code is missing a third label (e.g. die_first) in
between die_early and die (at r->status = HTTP_OK), which we would
branch instead of die_early when failing with a "suitable" input state
(but before post_read_request() hooks still after which we can only
call ap_die() without resetting r->status).
Actually it depends on whether (or not) we want ap_die() (i.e.
ErrorDocument configuration) to be able to read the body at these
points of failure, and if so we should probably insert the HTTP_IN
filter earlier too.

WDYT?

>
> > @@ -1498,10 +1478,23 @@ request_rec *ap_read_request(conn_rec *c
> >   * HTTP/1.1 mentions twice (S9, S14.23) that a request MUST contain
> >   * a Host: header, and the server MUST respond with 400 if it 
> > doesn't.
> >   */
> > -access_status = HTTP_BAD_REQUEST;
> >  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00569)
> >"client sent HTTP/1.1 request without hostname "
> >"(see RFC2616 section 14.23): %s", r->uri);
> > +access_status = HTTP_BAD_REQUEST;
> > +goto die_early;
>
> Why do we die_early here and not just die later?

Same here.

Regards,
Yann.

Re: svn commit: r1876674 - in /httpd/httpd/trunk: include/ap_mmn.h include/http_protocol.h modules/http2/h2_request.c server/protocol.c

[Ruediger Pluem]

On 4/17/20 6:47 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Fri Apr 17 16:47:42 2020
> New Revision: 1876674
> 
> URL: http://svn.apache.org/viewvc?rev=1876674&view=rev
> Log:
> core, h2: common ap_parse_request_line() and ap_check_request_header() code.
> 
> Extract parsing/validation code from read_request_line() and ap_read_request()
> into ap_parse_request_line() and ap_check_request_header() helpers such that
> mod_http2 can validate its HTTP/1 request with the same/configured policy.
> 
> Modified:
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/include/http_protocol.h
> httpd/httpd/trunk/modules/http2/h2_request.c
> httpd/httpd/trunk/server/protocol.c
> 

> 
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1876674&r1=1876673&r2=1876674&view=diff
> ==
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Fri Apr 17 16:47:42 2020

> @@ -1444,50 +1526,13 @@ request_rec *ap_read_request(conn_rec *c
>  }
>  }
>  
> -/* update what we think the virtual host is based on the headers we've
> - * now read. may update status.
> - */
> -strict_host_check = (conf->strict_host_check == AP_CORE_CONFIG_ON);
> -access_status = ap_update_vhost_from_headers_ex(r, strict_host_check);
> -if (strict_host_check && access_status != HTTP_OK) { 
> - if (r->server == ap_server_conf) { 
> - ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(10156)
> -   "Requested hostname '%s' did not match any 
> ServerName/ServerAlias "
> -   "in the global server configuration ", 
> r->hostname);
> - } else { 
> - ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(10157)
> -   "Requested hostname '%s' did not match any 
> ServerName/ServerAlias "
> -   "in the matching virtual host (default vhost for "
> -   "current connection is %s:%u)", 
> -   r->hostname, r->server->defn_name, 
> r->server->defn_line_number);
> - }
> - goto die_early;
> -}
> -if (r->status != HTTP_OK) { 
> +if (!ap_check_request_header(r)) {
>  access_status = r->status;
>  goto die_early;

This means we now die_early where we did not before and may not reply with 
custom error pages configured for the vhost.

>  }
>  
> -if ((!r->hostname && (r->proto_num >= HTTP_VERSION(1, 1)))
> -|| ((r->proto_num == HTTP_VERSION(1, 1))
> -&& !apr_table_get(r->headers_in, "Host"))) {
> -/*
> - * Client sent us an HTTP/1.1 or later request without telling us the
> - * hostname, either with a full URL or a Host: header. We therefore
> - * need to (as per the 1.1 spec) send an error.  As a special case,
> - * HTTP/1.1 mentions twice (S9, S14.23) that a request MUST contain
> - * a Host: header, and the server MUST respond with 400 if it 
> doesn't.
> - */
> -ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00569)
> -  "client sent HTTP/1.1 request without hostname "
> -  "(see RFC2616 section 14.23): %s", r->uri);
> -access_status = HTTP_BAD_REQUEST;
> -goto die_early;
> -}
> -
>  /* we may have switched to another server */
>  r->per_dir_config = r->server->lookup_defaults;
> -conf = ap_get_core_module_config(r->server->module_config);
>  
>  /* Toggle to the Host:-based vhost's timeout mode to fetch the
>   * request body and send the response body, if needed.

Regards

Rüdiger

Re: svn commit: r1876664 - in /httpd/httpd/trunk: modules/http2/h2_request.c server/protocol.c

[Ruediger Pluem]

On 4/17/20 3:07 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Fri Apr 17 13:07:46 2020
> New Revision: 1876664
> 
> URL: http://svn.apache.org/viewvc?rev=1876664&view=rev
> Log:
> core, h2: send EOR for early HTTP request failure.
> 
> The core output filters depend on EOR being sent at some point for correct
> accounting of setaside limits and lifetime.
> 
> Rework ap_read_request() early failure (including in post_read_request() 
> hooks)
> so that it always sends the EOR after ap_die().
> 
> Apply the same scheme in h2_request_create_rec() which is the HTTP/2 to HTTP/1
> counterpart.
> 
> Modified:
> httpd/httpd/trunk/modules/http2/h2_request.c
> httpd/httpd/trunk/server/protocol.c
> 

> Modified: httpd/httpd/trunk/server/protocol.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1876664&r1=1876663&r2=1876664&view=diff
> ==
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Fri Apr 17 13:07:46 2020

> @@ -1453,14 +1444,12 @@ request_rec *ap_read_request(conn_rec *c
>  }
>  }
>  
> -apr_brigade_destroy(tmp_bb);

Why don't we destroy it any longer?

> -
>  /* update what we think the virtual host is based on the headers we've
>   * now read. may update status.
>   */
> -
> -access_status = ap_update_vhost_from_headers_ex(r, 
> conf->strict_host_check == AP_CORE_CONFIG_ON);
> -if (conf->strict_host_check == AP_CORE_CONFIG_ON && access_status != 
> HTTP_OK) { 
> +strict_host_check = (conf->strict_host_check == AP_CORE_CONFIG_ON);
> +access_status = ap_update_vhost_from_headers_ex(r, strict_host_check);
> +if (strict_host_check && access_status != HTTP_OK) { 
>   if (r->server == ap_server_conf) { 
>   ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(10156)
> "Requested hostname '%s' did not match any 
> ServerName/ServerAlias "
> @@ -1472,22 +1461,13 @@ request_rec *ap_read_request(conn_rec *c
> "current connection is %s:%u)", 
> r->hostname, r->server->defn_name, 
> r->server->defn_line_number);
>   }
> - r->status = access_status;
> + goto die_early;
>  }
> -
> -access_status = r->status;
> -
> -/* Toggle to the Host:-based vhost's timeout mode to fetch the
> - * request body and send the response body, if needed.
> - */
> -if (cur_timeout != r->server->timeout) {
> -apr_socket_timeout_set(csd, r->server->timeout);
> -cur_timeout = r->server->timeout;
> +if (r->status != HTTP_OK) { 
> +access_status = r->status;
> +goto die_early;

Why do we die_early here and not just die later?

>  }
>  
> -/* we may have switched to another server */
> -r->per_dir_config = r->server->lookup_defaults;
> -
>  if ((!r->hostname && (r->proto_num >= HTTP_VERSION(1, 1)))
>  || ((r->proto_num == HTTP_VERSION(1, 1))
>  && !apr_table_get(r->headers_in, "Host"))) {
> @@ -1498,10 +1478,23 @@ request_rec *ap_read_request(conn_rec *c
>   * HTTP/1.1 mentions twice (S9, S14.23) that a request MUST contain
>   * a Host: header, and the server MUST respond with 400 if it 
> doesn't.
>   */
> -access_status = HTTP_BAD_REQUEST;
>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00569)
>"client sent HTTP/1.1 request without hostname "
>"(see RFC2616 section 14.23): %s", r->uri);
> +access_status = HTTP_BAD_REQUEST;
> +goto die_early;

Why do we die_early here and not just die later?


> +}
> +
> +/* we may have switched to another server */
> +r->per_dir_config = r->server->lookup_defaults;
> +conf = ap_get_core_module_config(r->server->module_config);
> +
> +/* Toggle to the Host:-based vhost's timeout mode to fetch the
> + * request body and send the response body, if needed.
> + */
> +if (cur_timeout != r->server->timeout) {
> +apr_socket_timeout_set(csd, r->server->timeout);
> +cur_timeout = r->server->timeout;
>  }
>  
>  /*

Regards

Rüdiger

Re: svn commit: r1876619 - /httpd/httpd/trunk/modules/core/mod_watchdog.c

[Yann Ylavic]

On Thu, Apr 16, 2020 at 7:55 PM  wrote:
>
> Author: jorton
> Date: Thu Apr 16 17:55:48 2020
> New Revision: 1876619
>
> URL: http://svn.apache.org/viewvc?rev=1876619&view=rev
> Log:
> * modules/core/mod_watchdog.c (wd_worker): Fix crashes snuck into
>   r1876599 where a destroyed pool was reused.  Rename the "ctx"
>   variable to reflect its purpose.  Also tweak the pool tags.

Sorry for messing up with that, I didn't notice that
apr_pool_destroy() was used :/
Hopefully my intent is better addressed in r1876675..

Re: svn commit: r1876511 - /httpd/httpd/trunk/modules/core/mod_watchdog.c

[Joe Orton]

On Wed, Apr 15, 2020 at 09:50:01AM -0400, Jim Jagielski wrote:
> very, very elegant.

Thank you Jim!

I wonder if the new state variable is redundant with the other state 
variables in the watchdog structure?  I don't understand the watchdog 
model well enough to be confident here.

 struct ap_watchdog_t
{
apr_uint32_t  thread_started; /* set to 1 once thread running */
...
int   singleton;
int   active;


Also reviewing this module more, in the post_config hook:

http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/core/mod_watchdog.c?annotate=1876619#l434

it looks to me like the ap_state_query() call correctly guards against 
multiple post_config runs during start, so there will never be a point 
where the wd_server_conf attached to pconf is reused or reusable.

- in initial startup you get only one post_config invocation if the 
_PRE_CONFIG phase is skipped

- during subsequent server reloads pconf is cleared each time

I may be missing something.

Regards, Joe

> 
> > On Apr 14, 2020, at 8:37 AM, jor...@apache.org wrote:
> > 
> > Author: jorton
> > Date: Tue Apr 14 12:37:17 2020
> > New Revision: 1876511
> > 
> > URL: http://svn.apache.org/viewvc?rev=1876511&view=rev
> > Log:
> > * modules/core/mod_watchdog.c: Switch to simpler logic to avoid the
> >  thread cleanup running before the thread has started, avoiding
> >  mutex operations which both have undefined behaviour:
> > 
> >  a) double-locking an UNNESTED (non-recursive) mutex twice in the parent
> >  b) unlocking a mutex in the spawned thread which was locked by the parent
> > 
> >  (wd_startup, wd_worker_cleanup, wd_worker): Use a boolean to ensure
> >  the cleanup does nothing if the thread wasn't started, drop the mutex.
> > 
> > Modified:
> >httpd/httpd/trunk/modules/core/mod_watchdog.c
> > 
> > Modified: httpd/httpd/trunk/modules/core/mod_watchdog.c
> > URL: 
> > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/core/mod_watchdog.c?rev=1876511&r1=1876510&r2=1876511&view=diff
> > ==
> > --- httpd/httpd/trunk/modules/core/mod_watchdog.c (original)
> > +++ httpd/httpd/trunk/modules/core/mod_watchdog.c Tue Apr 14 12:37:17 2020
> > @@ -24,6 +24,8 @@
> > #include "http_core.h"
> > #include "util_mutex.h"
> > 
> > +#include "apr_atomic.h"
> > +
> > #define AP_WATCHDOG_PGROUP"watchdog"
> > #define AP_WATCHDOG_PVERSION  "parent"
> > #define AP_WATCHDOG_CVERSION  "child"
> > @@ -43,7 +45,7 @@ struct watchdog_list_t
> > 
> > struct ap_watchdog_t
> > {
> > -apr_thread_mutex_t   *startup;
> > +apr_uint32_t  thread_started; /* set to 1 once thread running 
> > */
> > apr_proc_mutex_t *mutex;
> > const char   *name;
> > watchdog_list_t  *callbacks;
> > @@ -74,6 +76,10 @@ static apr_status_t wd_worker_cleanup(vo
> > apr_status_t rv;
> > ap_watchdog_t *w = (ap_watchdog_t *)data;
> > 
> > +/* Do nothing if the thread wasn't started. */
> > +if (apr_atomic_read32(&w->thread_started) != 1)
> > +return APR_SUCCESS;
> > +
> > if (w->is_running) {
> > watchdog_list_t *wl = w->callbacks;
> > while (wl) {
> > @@ -110,7 +116,8 @@ static void* APR_THREAD_FUNC wd_worker(a
> > w->pool = apr_thread_pool_get(thread);
> > w->is_running = 1;
> > 
> > -apr_thread_mutex_unlock(w->startup);
> > +apr_atomic_set32(&w->thread_started, 1); /* thread started */
> > +
> > if (w->mutex) {
> > while (w->is_running) {
> > if (ap_mpm_query(AP_MPMQ_MPM_STATE, &mpmq_s) != APR_SUCCESS) {
> > @@ -264,10 +271,7 @@ static apr_status_t wd_startup(ap_watchd
> > {
> > apr_status_t rc;
> > 
> > -/* Create thread startup mutex */
> > -rc = apr_thread_mutex_create(&w->startup, APR_THREAD_MUTEX_UNNESTED, 
> > p);
> > -if (rc != APR_SUCCESS)
> > -return rc;
> > +apr_atomic_set32(&w->thread_started, 0);
> > 
> > if (w->singleton) {
> > /* Initialize singleton mutex in child */
> > @@ -277,22 +281,12 @@ static apr_status_t wd_startup(ap_watchd
> > return rc;
> > }
> > 
> > -/* This mutex fixes problems with a fast start/fast end, where the pool
> > - * cleanup was being invoked before the thread completely spawned.
> > - */
> > -apr_thread_mutex_lock(w->startup);
> > -apr_pool_pre_cleanup_register(p, w, wd_worker_cleanup);
> > -
> > /* Start the newly created watchdog */
> > rc = apr_thread_create(&w->thread, NULL, wd_worker, w, p);
> > -if (rc) {
> > -apr_pool_cleanup_kill(p, w, wd_worker_cleanup);
> > +if (rc == APR_SUCCESS) {
> > +apr_pool_pre_cleanup_register(p, w, wd_worker_cleanup);
> > }
> > 
> > -apr_thread_mutex_lock(w->startup);
> > -apr_thread_mutex_unlock(w->startup);
> > -apr_thread_mutex_destroy(w->startup);
> > -
> > return rc;
> > }
> > 
> > 
> > 
> 

Re: mod_http2 crashes libhttpd.dll in 2.4.43

[Steffen]

Tested trunk r1876616. All fine now.

Thanks! for fixing the regression introduced in 2.4.43 GA

Made new Windows binaries of mod_http2  available at AL.

Steffen


On Thursday 16/04/2020 at 19:21, Stefan Eissing  wrote:

🤢🙈

Fixed in 
and apache trunk as r1876616 and proposed for backport.

~Stefan



Am 16.04.2020 um 13:51 schrieb Rainer Jung :

If I get this right, there is an element in elts, that has a valid 
string key ("H2_STREAM_ID") bis a NULL value (2nd screenshot) and the 
condition check in line 527 of the first screen shot checks key 
against NULL and empty, but value only against empty but not against 
NULL. So the empty check derefences NULL.


Nut sure what the correct fix is:

- make sure the H2_STREAM_IS value is never NULL (but maybe empty)
- add NULL check for the value to the list of checks in 527
- something else

At least the debug info provided by Steffen seems to be a good fit to 
the stream id handling changes in the revision in question (but i have 
not checked, whether the NULL is new). I think a fix is close :)


Thanks and regards,

Rainer

Am 16.04.2020 um 10:12 schrieb Steffen:


More info.
See CallStack
  http://www.apachelounge.com/download/VS16/modules/CallStack.png
and
  http://www.apachelounge.com/download/VS16/modules/autos.png
Below we had:
libhttpd!ap_get_server_built+0x5d9
mod_cgi+0x14aa
libhttpd!ap_run_handler+0x35
libhttpd!ap_invoke_handler+0x10f
libhttpd!ap_internal_redirect_handler+0x29a
libhttpd!ap_process_request+0xf
mod_http2+0x188ef
libhttpd!ap_run_process_connection+0x35
mod_http2+0x185ba
mod_http2+0x1c36e
ucrtbase!beginthreadex+0x142
kernel32!BaseThreadInitThunk+0x14
ntdll!RtlUserThreadStart+0x21
Steffen
On Tuesday 14/04/2020 at 14:13, Eric Covener wrote:


On Tue, Apr 14, 2020 at 8:09 AM Ruediger Pluem  
wrote:





On 4/14/20 12:22 PM, Steffen wrote:




This is the post above of backtrace


Thanks.




By accident I've seen that Perl comes with GDB. This might help as 
well.
I called httpd.exe from GDB with "-X -e debug" and then called a Perl 
URL in the browser.


Excerpt below:



Somehow the below wasn't visible in the original mail.



Thread 100 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 4936.0x23e0]
0x7ffbe57515d9 in libhttpd!ap_get_server_built () from 
X:\Apps\Apache24\bin\libhttpd.dll

(gdb) bt
#0 0x7ffbe57515d9 in libhttpd!ap_get_server_built () from 
X:\Apps\Apache24\bin\libhttpd.dll
#1 0x7ffbe44d14aa in ?? () from 
X:\Apps\Apache24\modules\mod_cgi.so
#2 0x7ffbe575ee85 in libhttpd!ap_run_handler () from 
X:\Apps\Apache24\bin\libhttpd.dll
#3 0x7ffbe575da7f in libhttpd!ap_invoke_handler () from 
X:\Apps\Apache24\bin\libhttpd.dll
#4 0x7ffbe575a62a in libhttpd!ap_internal_redirect_handler () from 
X:\Apps\Apache24\bin\libhttpd.dll
#5 0x7ffbe575a6af in libhttpd!ap_process_request () from 
X:\Apps\Apache24\bin\libhttpd.dll
#6 0x7ffbe22888ef in ?? () from 
X:\Apps\Apache24\modules\mod_http2.so
#7 0x7ffbe5761545 in libhttpd!ap_run_process_connection () from 
X:\Apps\Apache24\bin\libhttpd.dll
#8 0x7ffbe22885ba in ?? () from 
X:\Apps\Apache24\modules\mod_http2.so
#9 0x7ffbe228c36e in ?? () from 
X:\Apps\Apache24\modules\mod_http2.so
#10 0x7ffbe9e30e72 in ucrtbase!_beginthreadex () from 
C:\Windows\System32\ucrtbase.dll
#11 0x7ffbea107bd4 in KERNEL32!BaseThreadInitThunk () from 
C:\Windows\System32\kernel32.dll
#12 0x7ffbebecced1 in ntdll!RtlUserThreadStart () from 
C:\Windows\SYSTEM32\ntdll.dll

#13 0x in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb)




Unfortunately this stacktrace does not help. One reason might be that 
the debugging symbols are missing.
It is very strange that it segfaults in ap_get_server_built, a simple 
function just returning a pointer
to a static string constant. Furthermore ap_get_server_built is not 
called by mod_cgi.
Can the crash be repeated against a binary with debugging symbols that 
are then used to generate the stacktrace?
As I am not a Windows guy, I unfortunately cannot provide any 
instructions how to do this.


My experience on windows is that if the PDB's are not 110% right you
will get all kinds of misleading stuff above the first ?? in the
displayed backtrace.