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:[email protected]]
> Sent: Sonntag, 11. Oktober 2015 19:54
> To: [email protected]
> 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
>
