httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: 2.4.17 test failure for mod_nntp_like_ssl when mod_http2 is loaded
Date Mon, 12 Oct 2015 08:54:51 GMT
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.dev@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.eissing@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.pluem@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.eissing@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
>>>>
>>

Mime
View raw message