Re: svn commit: r1874011 - /httpd/httpd/trunk/server/mpm/event/event.c
On 02/14/2020 05:17 PM, Marion & Christophe JAILLET wrote: > Hi, > > The same code exists in 'worker', should it be fixed as well? I would think so. Regards Rüdiger
Re: svn commit: r1874007 - in /httpd/httpd/trunk: CHANGES modules/ssl/ssl_util_ocsp.c
On 02/14/2020 06:05 PM, Marion & Christophe JAILLET wrote: > Hi, > > purely speculative, but does a: > apr_table_set(headers, "Connection", "close"); > > around line 812 of md_oscp.c also makes sense? In general I guess it could make sense, but I am not sure if this is the correct way to do it here, since we are not talking HTTP on a bare socket like in mod_ssl, but using libcurl where the same effect possibly should be done differently. Regards Rüdiger
Re: svn commit: r1874007 - in /httpd/httpd/trunk: CHANGES modules/ssl/ssl_util_ocsp.c
On 2/14/20 6:05 PM, Marion & Christophe JAILLET wrote: > Hi, > > purely speculative, but does a: > apr_table_set(headers, "Connection", "close"); > > around line 812 of md_oscp.c also makes sense? > I think it makes absolutely sense. Giovanni > CJ > > Le 14/02/2020 à 10:38, rpl...@apache.org a écrit : >> Author: rpluem >> Date: Fri Feb 14 09:38:12 2020 >> New Revision: 1874007 >> >> URL: http://svn.apache.org/viewvc?rev=1874007=rev >> Log: >> * modules/ssl/ssl_util_ocsp.c (serialize_request): Set the Connection header >> to close to indicate that we do not want to keep the HTTP connection to >> the >> OCSP responder alive. We don't reuse the connections currently and if the >> OCSP responder keeps the connection alive this could cause us to wait for >> keepalive timeout of the OCSP responder to timeout until we finish our >> reading of the OCSP response. >> >> PR: 64135 >> >> >> Modified: >> httpd/httpd/trunk/CHANGES >> httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c >> >> Modified: httpd/httpd/trunk/CHANGES >> URL: >> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1874007=1874006=1874007=diff >> == >> --- httpd/httpd/trunk/CHANGES [utf-8] (original) >> +++ httpd/httpd/trunk/CHANGES [utf-8] Fri Feb 14 09:38:12 2020 >> @@ -1,6 +1,9 @@ >> -*- coding: utf-8 >> -*- >> Changes with Apache 2.5.1 >> + *) mod_ssl: Do not keep connections to OCSP responders alive when doing >> + OCSP requests. PR 64135. [Ruediger Pluem] >> + >> *) mod_ssl: Disable client verification on ACME ALPN challenges. Fixes >> github >> issue mod_md#172 (https://github.com/icing/mod_md/issues/172). >> [Michael Kaufmann , Stefan Eissing] >> >> Modified: httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c >> URL: >> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c?rev=1874007=1874006=1874007=diff >> == >> --- httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c (original) >> +++ httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c Fri Feb 14 09:38:12 2020 >> @@ -46,6 +46,7 @@ static BIO *serialize_request(OCSP_REQUE >> BIO_printf(bio, "%s%s%s HTTP/1.0\r\n" >> "Host: %s:%d\r\n" >> "Content-Type: application/ocsp-request\r\n" >> + "Connection: close\r\n" >> "Content-Length: %d\r\n" >> "\r\n", >> uri->path ? uri->path : "/", >> >>
Re: svn commit: r1874007 - in /httpd/httpd/trunk: CHANGES modules/ssl/ssl_util_ocsp.c
Hi, purely speculative, but does a: apr_table_set(headers, "Connection", "close"); around line 812 of md_oscp.c also makes sense? CJ Le 14/02/2020 à 10:38, rpl...@apache.org a écrit : Author: rpluem Date: Fri Feb 14 09:38:12 2020 New Revision: 1874007 URL: http://svn.apache.org/viewvc?rev=1874007=rev Log: * modules/ssl/ssl_util_ocsp.c (serialize_request): Set the Connection header to close to indicate that we do not want to keep the HTTP connection to the OCSP responder alive. We don't reuse the connections currently and if the OCSP responder keeps the connection alive this could cause us to wait for keepalive timeout of the OCSP responder to timeout until we finish our reading of the OCSP response. PR: 64135 Modified: httpd/httpd/trunk/CHANGES httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c Modified: httpd/httpd/trunk/CHANGES URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1874007=1874006=1874007=diff == --- httpd/httpd/trunk/CHANGES [utf-8] (original) +++ httpd/httpd/trunk/CHANGES [utf-8] Fri Feb 14 09:38:12 2020 @@ -1,6 +1,9 @@ -*- coding: utf-8 -*- Changes with Apache 2.5.1 + *) mod_ssl: Do not keep connections to OCSP responders alive when doing + OCSP requests. PR 64135. [Ruediger Pluem] + *) mod_ssl: Disable client verification on ACME ALPN challenges. Fixes github issue mod_md#172 (https://github.com/icing/mod_md/issues/172). [Michael Kaufmann , Stefan Eissing] Modified: httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c?rev=1874007=1874006=1874007=diff == --- httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c (original) +++ httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c Fri Feb 14 09:38:12 2020 @@ -46,6 +46,7 @@ static BIO *serialize_request(OCSP_REQUE BIO_printf(bio, "%s%s%s HTTP/1.0\r\n" "Host: %s:%d\r\n" "Content-Type: application/ocsp-request\r\n" + "Connection: close\r\n" "Content-Length: %d\r\n" "\r\n", uri->path ? uri->path : "/",
Re: svn commit: r1874011 - /httpd/httpd/trunk/server/mpm/event/event.c
Hi, The same code exists in 'worker', should it be fixed as well? CJ Le 14/02/2020 à 11:47, jor...@apache.org a écrit : Author: jorton Date: Fri Feb 14 10:47:36 2020 New Revision: 1874011 URL: http://svn.apache.org/viewvc?rev=1874011=rev Log: * server/mpm/event/event.c (event_open_logs): Avoid UBSan exception calling memcpy(,NULL,0) at startup. Thanks to rpluem. Modified: httpd/httpd/trunk/server/mpm/event/event.c Modified: httpd/httpd/trunk/server/mpm/event/event.c URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1874011=1874010=1874011=diff == --- httpd/httpd/trunk/server/mpm/event/event.c (original) +++ httpd/httpd/trunk/server/mpm/event/event.c Fri Feb 14 10:47:36 2020 @@ -3616,8 +3616,9 @@ static int event_open_logs(apr_pool_t * new_max = num_buckets; } new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int)); -memcpy(new_ptr, retained->idle_spawn_rate, - retained->mpm->num_buckets * sizeof(int)); +if (retained->idle_spawn_rate) /* NULL at startup */ +memcpy(new_ptr, retained->idle_spawn_rate, + retained->mpm->num_buckets * sizeof(int)); retained->idle_spawn_rate = new_ptr; retained->mpm->max_buckets = new_max; }
Re: [PATCH] event idle_spawn_rate initialization?
On Fri, Feb 14, 2020 at 11:33:50AM +0100, Ruediger Pluem wrote: > On 02/14/2020 10:08 AM, Joe Orton wrote: > > I've been playing with UBSan[1] which catches undefined behaviour, found a > > couple of interesting things so far. > > > > One is with event, I messed with the line numbers but the error is: > > > > event.c:3620:13: runtime error: null pointer passed as argument 2, which is > > declared to never be null > > > > from the memcpy() in this code: > > https://github.com/apache/httpd/blob/trunk/server/mpm/event/event.c#L3619 > > > > new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int)); > > memcpy(new_ptr, retained->idle_spawn_rate, > >retained->mpm->num_buckets * sizeof(int)); > > I guess the above only does not crash because retained->mpm->num_buckets is 0 > at the same time. > But this is probably something we should not rely on with all memcpy > implementations. A, yes, that makes more sense now. Thanks a lot. ... > I guess there is no need for two cases here. We should only avoid the call to > memcpy > if retained->idle_spawn_rate is NULL. The initialization happens then in the > block starting > at line 3624. Gotcha. Done in r1874011. Regards, Joe
Re: [PATCH] event idle_spawn_rate initialization?
On 02/14/2020 10:08 AM, Joe Orton wrote:
> I've been playing with UBSan[1] which catches undefined behaviour, found a
> couple of interesting things so far.
>
> One is with event, I messed with the line numbers but the error is:
>
> event.c:3620:13: runtime error: null pointer passed as argument 2, which is
> declared to never be null
>
> from the memcpy() in this code:
> https://github.com/apache/httpd/blob/trunk/server/mpm/event/event.c#L3619
>
> new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
> memcpy(new_ptr, retained->idle_spawn_rate,
>retained->mpm->num_buckets * sizeof(int));
I guess the above only does not crash because retained->mpm->num_buckets is 0
at the same time.
But this is probably something we should not rely on with all memcpy
implementations.
> retained->idle_spawn_rate = new_ptr;
> retained->mpm->max_buckets = new_max;
>
> At startup it seems retained->idle_spawn_rate is NULL, and you can't
> (shouldn't?!) memcpy() from NULL. I am not sure what the right fix is
> here, is that array supposed to be initialized to something other than
> zero here, or somewhere else? Not obvious if the loop below will
> initialize it properly:
I think stuff is correctly initialized in the block starting at line 3624.
>
> https://github.com/apache/httpd/blob/trunk/server/mpm/event/event.c#L3624
>
> Is something like this correct? (also this could use apr_pmemdup rather
> than palloc+memcpy now I think about it)
>
> --- a/server/mpm/event/event.c
> +++ b/server/mpm/event/event.c
> @@ -3615,9 +3615,15 @@ static int event_open_logs(apr_pool_t * p, apr_pool_t
> * plog,
> if (new_max < num_buckets) {
> new_max = num_buckets;
> }
> -new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
> -memcpy(new_ptr, retained->idle_spawn_rate,
> - retained->mpm->num_buckets * sizeof(int));
> +if (retained->idle_spawn_rate) {
> +new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
> +memcpy(new_ptr, retained->idle_spawn_rate,
> + retained->mpm->num_buckets * sizeof(int));
> +}
> +else {
> +/* ### should initialize array to something other than 0?? */
> +new_ptr = apr_pcalloc(ap_pglobal, new_max * sizeof(int));
> +}
I guess there is no need for two cases here. We should only avoid the call to
memcpy
if retained->idle_spawn_rate is NULL. The initialization happens then in the
block starting
at line 3624.
Regards
Rüdiger
[PATCH] event idle_spawn_rate initialization?
I've been playing with UBSan[1] which catches undefined behaviour, found a
couple of interesting things so far.
One is with event, I messed with the line numbers but the error is:
event.c:3620:13: runtime error: null pointer passed as argument 2, which is
declared to never be null
from the memcpy() in this code:
https://github.com/apache/httpd/blob/trunk/server/mpm/event/event.c#L3619
new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
memcpy(new_ptr, retained->idle_spawn_rate,
retained->mpm->num_buckets * sizeof(int));
retained->idle_spawn_rate = new_ptr;
retained->mpm->max_buckets = new_max;
At startup it seems retained->idle_spawn_rate is NULL, and you can't
(shouldn't?!) memcpy() from NULL. I am not sure what the right fix is
here, is that array supposed to be initialized to something other than
zero here, or somewhere else? Not obvious if the loop below will
initialize it properly:
https://github.com/apache/httpd/blob/trunk/server/mpm/event/event.c#L3624
Is something like this correct? (also this could use apr_pmemdup rather
than palloc+memcpy now I think about it)
--- a/server/mpm/event/event.c
+++ b/server/mpm/event/event.c
@@ -3615,9 +3615,15 @@ static int event_open_logs(apr_pool_t * p, apr_pool_t *
plog,
if (new_max < num_buckets) {
new_max = num_buckets;
}
-new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
-memcpy(new_ptr, retained->idle_spawn_rate,
- retained->mpm->num_buckets * sizeof(int));
+if (retained->idle_spawn_rate) {
+new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
+memcpy(new_ptr, retained->idle_spawn_rate,
+ retained->mpm->num_buckets * sizeof(int));
+}
+else {
+/* ### should initialize array to something other than 0?? */
+new_ptr = apr_pcalloc(ap_pglobal, new_max * sizeof(int));
+}
retained->idle_spawn_rate = new_ptr;
retained->mpm->max_buckets = new_max;
}
[1] https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
