Patch as a file attached.
On Tue, Jun 28, 2016 at 1:48 PM, Yann Ylavic <[email protected]> wrote:
> Maybe if you can test current 2.4.x with this patch and it works as
> expected it could be backported...
>
> On Tue, Jun 28, 2016 at 1:46 PM, Yann Ylavic <[email protected]> wrote:
>> Dunno, the issue is that reused TLS connections where data are
>> immediately available from the backend may be missing some bytes...
>>
>> On Tue, Jun 28, 2016 at 1:43 PM, Stefan Eissing
>> <[email protected]> wrote:
>>> Ah, understood. Do you want to squeeze it into 2.4.23 or can it wait?
>>>
>>>> Am 28.06.2016 um 13:42 schrieb Yann Ylavic <[email protected]>:
>>>>
>>>> I don't think trunk needs it because ap_proxy_connect_backend() is
>>>> already doing this work (via ap_proxy_check_backend).
>>>>
>>>> That's why I proposed a 2.4.x only patch, but I can commit it to trunk
>>>> temporarily if that helps (and until) backport...
>>>>
>>>>
>>>> On Tue, Jun 28, 2016 at 12:36 PM, Stefan Eissing
>>>> <[email protected]> wrote:
>>>>> We are talking about adding this to trunk first, right? ^^
>>>>>
>>>>>> Am 28.06.2016 um 12:34 schrieb Stefan Eissing
>>>>>> <[email protected]>:
>>>>>>
>>>>>> I believe so. Highly experimental and all such...
>>>>>>
>>>>>>> Am 28.06.2016 um 12:23 schrieb Yann Ylavic <[email protected]>:
>>>>>>>
>>>>>>> I can, but is mod_proxy_h2 CTR (Commit Then Review) like
>>>>>>> mod_h2 ?
>>>>>>>
>>>>>>> On Tue, Jun 28, 2016 at 12:15 PM, Stefan Eissing
>>>>>>> <[email protected]> wrote:
>>>>>>>> Looks good to me. Can you commit this, then I quickly run my tests
>>>>>>>> with it...
>>>>>>>>
>>>>>>>>> Am 28.06.2016 um 09:50 schrieb Yann Ylavic <[email protected]>:
>>>>>>>>>
>>>>>>>>> On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic <[email protected]>
>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> The possible issue if r1750414 were backported, is that without
>>>>>>>>>>> r1750392 mod_proxy_http2 may not detect a TLS close notify before
>>>>>>>>>>> reusing a backend connection.
>>>>>>>>>>> If it's not backported, it may close a legitimate backend connection
>>>>>>>>>>> with (pre-)available data...
>>>>>>>>>>
>>>>>>>>>> I meant: it may discard (pre-)available data (not closing the
>>>>>>>>>> connection).
>>>>>>>>>
>>>>>>>>> A possible solution for 2.4.x (needed only there AFAICT), could be:
>>>>>>>>>
>>>>>>>>> Index: modules/http2/mod_proxy_http2.c
>>>>>>>>> ===================================================================
>>>>>>>>> --- modules/http2/mod_proxy_http2.c (revision 1750453)
>>>>>>>>> +++ modules/http2/mod_proxy_http2.c (working copy)
>>>>>>>>> @@ -520,11 +520,19 @@ run_connect:
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> ctx->p_conn->is_ssl = ctx->is_ssl;
>>>>>>>>> - if (ctx->is_ssl) {
>>>>>>>>> - /* If there is still some data on an existing ssl
>>>>>>>>> connection, now
>>>>>>>>> - * would be a good timne to get rid of it. */
>>>>>>>>> - ap_proxy_ssl_connection_cleanup(ctx->p_conn, ctx->rbase);
>>>>>>>>> - }
>>>>>>>>> + if (ctx->is_ssl && ctx->p_conn->connection) {
>>>>>>>>> + /* If there are some metadata on the connection (e.g. TLS
>>>>>>>>> alert),
>>>>>>>>> + * let mod_ssl detect them, and create a new connection
>>>>>>>>> below.
>>>>>>>>> + */
>>>>>>>>> + apr_bucket_brigade *tmp_bb;
>>>>>>>>> + tmp_bb = apr_brigade_create(r->pool,
>>>>>>>>> r->connection->bucket_alloc);
>>>>>>>>> + status =
>>>>>>>>> ap_get_brigade(ctx->p_conn->connection->input_filters, tmp_bb,
>>>>>>>>> + AP_MODE_SPECULATIVE,
>>>>>>>>> APR_NONBLOCK_READ, 1);
>>>>>>>>> + if (status != APR_SUCCESS && !APR_STATUS_IS_EAGAIN(status)) {
>>>>>>>>> + ctx->p_conn->close = 1;
>>>>>>>>> + }
>>>>>>>>> + apr_brigade_cleanup(tmp_bb);
>>>>>>>>> + }
>>>>>>>>>
>>>>>>>>> /* Step One: Determine the URL to connect to (might be a proxy),
>>>>>>>>> * initialize the backend accordingly and determine the server
>>>>>>>>> _
>>>>>>>>>
>>>>>>>>> Stefan?
>>>>>>>>
>>>>>>
>>>>>
>>>
Index: modules/http2/mod_proxy_http2.c
===================================================================
--- modules/http2/mod_proxy_http2.c (revision 1750453)
+++ modules/http2/mod_proxy_http2.c (working copy)
@@ -520,11 +520,19 @@ run_connect:
}
ctx->p_conn->is_ssl = ctx->is_ssl;
- if (ctx->is_ssl) {
- /* If there is still some data on an existing ssl connection, now
- * would be a good timne to get rid of it. */
- ap_proxy_ssl_connection_cleanup(ctx->p_conn, ctx->rbase);
- }
+ if (ctx->is_ssl && ctx->p_conn->connection) {
+ /* If there are some metadata on the connection (e.g. TLS alert),
+ * let mod_ssl detect them, and create a new connection below.
+ */
+ apr_bucket_brigade *tmp_bb;
+ tmp_bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
+ status = ap_get_brigade(ctx->p_conn->connection->input_filters, tmp_bb,
+ AP_MODE_SPECULATIVE, APR_NONBLOCK_READ, 1);
+ if (status != APR_SUCCESS && !APR_STATUS_IS_EAGAIN(status)) {
+ ctx->p_conn->close = 1;
+ }
+ apr_brigade_cleanup(tmp_bb);
+ }
/* Step One: Determine the URL to connect to (might be a proxy),
* initialize the backend accordingly and determine the server
