Jim didn't tag yet AFAICT, did he? If not, since it's mod_proxy_http2 scope only, +1 for me.
On Tue, Jun 28, 2016 at 3:02 PM, Stefan Eissing <[email protected]> wrote: > Thanks. I had to change from r-> to ctx->rbase, but otherwise works fine. > > Shall I commit this and potentially break Jim's tagging again? > > -Stefan > > > >> Am 28.06.2016 um 13:49 schrieb Yann Ylavic <[email protected]>: >> >> 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? >>>>>>>>>> >>>>>>>> >>>>>>> >>>>> >> <httpd-2.4.x-mod_proxy_http2-ssl_reuse_cleanup.patch> > >
