Also, I wonder if we don't need: Index: modules/http2/h2_task_input.c =================================================================== --- modules/http2/h2_task_input.c (revision 1707888) +++ modules/http2/h2_task_input.c (working copy) @@ -116,7 +116,7 @@ apr_status_t h2_task_input_read(h2_task_input *inp }
if (mode == AP_MODE_INIT) { - return APR_SUCCESS; + return ap_get_brigade(f->c->input_filters, bb, mode, block, readbytes); } if (input->bb) { -- We shouldn't since h2_task_input_read() is called at NETWORK level (hence after mod_ssl which "eats" AP_MODE_INIT), but it looks more correct (as it would be in mod_ssl) since anyway the core_input_filter() can handle it. On Mon, Oct 12, 2015 at 10:42 AM, Yann Ylavic <ylavic....@gmail.com> wrote: > The key here is using AP_MODE_INIT first (where 0 make sense) before > AP_MODE_SPECULATIVE. > > > On Mon, Oct 12, 2015 at 10:36 AM, Stefan Eissing > <stefan.eiss...@greenbytes.de> wrote: >> Yeah, I tried a 0 len read. Unfortunately, the core input filter is not >> happy about that: >> >> core_filters.c, line 231 >> AP_DEBUG_ASSERT(readbytes > 0); >> >>> Am 12.10.2015 um 10:25 schrieb Plüm, Rüdiger, Vodafone Group >>> <ruediger.pl...@vodafone.com>: >>> >>> How about the following patch? >>> >>> This separates the triggering of the SSL Handshake and the possible reading >>> of the 24 bytes: >>> >>> Index: h2_h2.c >>> =================================================================== >>> --- h2_h2.c (revision 1707437) >>> +++ h2_h2.c (working copy) >>> @@ -154,7 +154,7 @@ >>> >>> temp = apr_brigade_create(c->pool, c->bucket_alloc); >>> status = ap_get_brigade(c->input_filters, temp, >>> - AP_MODE_SPECULATIVE, APR_BLOCK_READ, 24); >>> + AP_MODE_INIT, APR_BLOCK_READ, 0); >>> >>> if (status == APR_SUCCESS) { >>> if (h2_ctx_protocol_get(c) >>> @@ -171,23 +171,31 @@ >>> char *s = NULL; >>> apr_size_t slen; >>> >>> - apr_brigade_pflatten(temp, &s, &slen, c->pool); >>> - if ((slen >= 24) && !memcmp(H2_MAGIC_TOKEN, s, 24)) { >>> - ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c, >>> - "h2_h2, direct mode detected"); >>> - h2_ctx_protocol_set(ctx, is_tls? "h2" : "h2c"); >>> + status = ap_get_brigade(c->input_filters, temp, >>> + AP_MODE_SPECULATIVE, >>> APR_BLOCK_READ, 24); >>> + if (status == APR_SUCCESS) { >>> + apr_brigade_pflatten(temp, &s, &slen, c->pool); >>> + if ((slen >= 24) && !memcmp(H2_MAGIC_TOKEN, s, >>> 24)) { >>> + ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c, >>> + "h2_h2, direct mode detected"); >>> + h2_ctx_protocol_set(ctx, is_tls? "h2" : "h2c"); >>> + } >>> + else { >>> + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c, >>> + "h2_h2, not detected in %d >>> bytes: %s", >>> + (int)slen, s); >>> + } >>> } >>> - else { >>> - ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c, >>> - "h2_h2, not detected in %d bytes: >>> %s", >>> - (int)slen, s); >>> - } >>> } >>> + else { >>> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, c, >>> + "h2_h2, error reading 24 bytes >>> speculative"); >>> + } >>> } >>> } >>> else { >>> ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, c, >>> - "h2_h2, error reading 24 bytes speculative"); >>> + "h2_h2, Failed to init connection"); >>> } >>> apr_brigade_destroy(temp); >>> } >>> >>> This would still block in the non ssl case if directmode is not set to off >>> explicitly. I would propose to change the default behaviour of directmode >>> here to off as directmode seems to be something very special to me that >>> should be explicitly enabled. >>> >>> Regards >>> >>> Rüdiger >>> >>>> -----Original Message----- >>>> From: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de] >>>> Sent: Sonntag, 11. Oktober 2015 19:54 >>>> To: dev@httpd.apache.org >>>> Subject: Re: 2.4.17 test failure for mod_nntp_like_ssl when mod_http2 is >>>> loaded >>>> >>>> Ok, in ssl_engine_io.c, lines 1426+ I see a hint: >>>> >>>> /* XXX: we could actually move ssl_io_filter_handshake to an >>>> * ap_hook_process_connection but would still need to call it for >>>> * AP_MODE_INIT for protocols that may upgrade the connection >>>> * rather than have SSLEngine On configured. >>>> */ >>>> if ((status = ssl_io_filter_handshake(inctx->filter_ctx)) != >>>> APR_SUCCESS) { >>>> return ssl_io_filter_error(f, bb, status); >>>> } >>>> >>>> The handshake handling gets triggered on a filter, so it all happens on >>>> READs (or WRITEs), which explains why connection hooks do not see it. This >>>> is quite some code which looks like we do not want to touch for a "quick >>>> fix". Also, there is no quick fix inside mod_http2 as before handshake, >>>> the SNI might also not be there yet, thus the vhost cannot be determined. >>>> etc. >>>> >>>> For the current release, I think we should leave it as it is and advise >>>> that current mod_http2 is incompatible with things like nntp. >>>> >>>> For the next release, I'd like the server to perform the handshake at a >>>> defined time, so other modules can rely on protocols being selected and >>>> correct vhost being known before the first read/write. >>>> >>>> //Stefan >>>> >>