Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
Commited in r1750505.
> Am 28.06.2016 um 15:13 schrieb Yann Ylavic :
>
> 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
> 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 :
>>>
>>> Patch as a file attached.
>>>
>>> On Tue, Jun 28, 2016 at 1:48 PM, Yann Ylavic 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 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
> 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 :
>>>
>>> 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
>>> wrote:
We are talking about adding this to trunk first, right? ^^
> Am 28.06.2016 um 12:34 schrieb Stefan Eissing
> :
>
> I believe so. Highly experimental and all such...
>
>> Am 28.06.2016 um 12:23 schrieb Yann Ylavic :
>>
>> 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
>> 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 :
On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic
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?
>>>
>
>>
>>>
>>
>>
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
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
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 :
>>
>> Patch as a file attached.
>>
>> On Tue, Jun 28, 2016 at 1:48 PM, Yann Ylavic 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 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
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 :
>>
>> 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
>> wrote:
>>> We are talking about adding this to trunk first, right? ^^
>>>
Am 28.06.2016 um 12:34 schrieb Stefan Eissing
:
I believe so. Highly experimental and all such...
> Am 28.06.2016 um 12:23 schrieb Yann Ylavic :
>
> 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
> 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 :
>>>
>>> On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic
>>> 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?
>>
>>>
>
>>
>
>
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
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
httpd-2.4.x-mod_proxy_http2-ssl_reuse_cleanup-v2.diff
Description: Binary data
> Am 28.06.2016 um 13:49 schrieb Yann Ylavic :
>
> Patch as a file attached.
>
> On Tue, Jun 28, 2016 at 1:48 PM, Yann Ylavic 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 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
>>> 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 :
>
> 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
> wrote:
>> We are talking about adding this to trunk first, right? ^^
>>
>>> Am 28.06.2016 um 12:34 schrieb Stefan Eissing
>>> :
>>>
>>> I believe so. Highly experimental and all such...
>>>
Am 28.06.2016 um 12:23 schrieb Yann Ylavic :
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
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 :
>>
>> On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic
>> 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?
>
>>>
>>
>
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
Patch as a file attached.
On Tue, Jun 28, 2016 at 1:48 PM, Yann Ylavic 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 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
>> 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 :
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
wrote:
> We are talking about adding this to trunk first, right? ^^
>
>> Am 28.06.2016 um 12:34 schrieb Stefan Eissing
>> :
>>
>> I believe so. Highly experimental and all such...
>>
>>> Am 28.06.2016 um 12:23 schrieb Yann Ylavic :
>>>
>>> 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
>>> 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 :
>
> On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic
> 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_briga
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
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 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
> 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 :
>>>
>>> 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
>>> wrote:
We are talking about adding this to trunk first, right? ^^
> Am 28.06.2016 um 12:34 schrieb Stefan Eissing
> :
>
> I believe so. Highly experimental and all such...
>
>> Am 28.06.2016 um 12:23 schrieb Yann Ylavic :
>>
>> 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
>> 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 :
On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic
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?
>>>
>
>>
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
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
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 :
>>
>> 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
>> wrote:
>>> We are talking about adding this to trunk first, right? ^^
>>>
Am 28.06.2016 um 12:34 schrieb Stefan Eissing
:
I believe so. Highly experimental and all such...
> Am 28.06.2016 um 12:23 schrieb Yann Ylavic :
>
> 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
> 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 :
>>>
>>> On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic
>>> 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?
>>
>>>
>
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
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 :
>
> 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
> wrote:
>> We are talking about adding this to trunk first, right? ^^
>>
>>> Am 28.06.2016 um 12:34 schrieb Stefan Eissing
>>> :
>>>
>>> I believe so. Highly experimental and all such...
>>>
Am 28.06.2016 um 12:23 schrieb Yann Ylavic :
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
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 :
>>
>> On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic
>> 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?
>
>>>
>>
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
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
wrote:
> We are talking about adding this to trunk first, right? ^^
>
>> Am 28.06.2016 um 12:34 schrieb Stefan Eissing :
>>
>> I believe so. Highly experimental and all such...
>>
>>> Am 28.06.2016 um 12:23 schrieb Yann Ylavic :
>>>
>>> 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
>>> 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 :
>
> On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic
> 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?
>>
>
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
We are talking about adding this to trunk first, right? ^^
> Am 28.06.2016 um 12:34 schrieb Stefan Eissing :
>
> I believe so. Highly experimental and all such...
>
>> Am 28.06.2016 um 12:23 schrieb Yann Ylavic :
>>
>> 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
>> 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 :
On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic 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?
>>>
>
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
I believe so. Highly experimental and all such...
> Am 28.06.2016 um 12:23 schrieb Yann Ylavic :
>
> 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
> 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 :
>>>
>>> On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic 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?
>>
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
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
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 :
>>
>> On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic 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?
>
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
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 :
>
> On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic 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?
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
On Tue, Jun 28, 2016 at 12:23 AM, Yann Ylavic 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?
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
On Tue, Jun 28, 2016 at 12:11 AM, Yann Ylavic wrote: > On Mon, Jun 27, 2016 at 11:26 PM, Yann Ylavic wrote: >> On Mon, Jun 27, 2016 at 10:48 AM, Stefan Eissing >> wrote: >>> Am 27.06.2016 um 10:41 schrieb Yann Ylavic : On Mon, Jun 27, 2016 at 10:23 AM, Stefan Eissing wrote: > This looks nice for HTTP/1.1, but what about other protocols? Do I read > it correctly that any pending data downstream will reopen the connection? Hmm, I did not think about mod_proxy_h2, but correct (I'd rather say upstream though, backend side). Are there cases where some backend data are available before the request is sent? >>> >>> In HTTP/2, yes. For example a PING frame might be waiting, or a >>> SETTINGS change. Most backends will have short timeouts for answers >>> to these (SETTINGS is expected to be ACKed soonish), but races may >>> happen. Also, HTTP/2 extensions with new frame types that need to be >>> ignored when not known may get received here. >> >> OK, so we probably should get rid of the call to >> ap_proxy_ssl_connection_cleanup() in proxy_http2_handler (and also in >> proxy_http_handler with my latest changes since the same check is now >> available in ap_proxy_check_connection). > > I went ahead and committed r1750414 (resp. r1750416). > 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).
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
On Mon, Jun 27, 2016 at 11:26 PM, Yann Ylavic wrote: > On Mon, Jun 27, 2016 at 10:48 AM, Stefan Eissing > wrote: >> >>> Am 27.06.2016 um 10:41 schrieb Yann Ylavic : >>> >>> On Mon, Jun 27, 2016 at 10:23 AM, Stefan Eissing wrote: This looks nice for HTTP/1.1, but what about other protocols? Do I read it correctly that any pending data downstream will reopen the connection? >>> >>> Hmm, I did not think about mod_proxy_h2, but correct (I'd rather say >>> upstream though, backend side). >>> Are there cases where some backend data are available before the >>> request is sent? >> >> In HTTP/2, yes. For example a PING frame might be waiting, or a >> SETTINGS change. Most backends will have short timeouts for answers >> to these (SETTINGS is expected to be ACKed soonish), but races may >> happen. Also, HTTP/2 extensions with new frame types that need to be >> ignored when not known may get received here. > > OK, so we probably should get rid of the call to > ap_proxy_ssl_connection_cleanup() in proxy_http2_handler (and also in > proxy_http_handler with my latest changes since the same check is now > available in ap_proxy_check_connection). I went ahead and committed r1750414 (resp. r1750416). 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...
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
On Mon, Jun 27, 2016 at 10:48 AM, Stefan Eissing wrote: > >> Am 27.06.2016 um 10:41 schrieb Yann Ylavic : >> >> On Mon, Jun 27, 2016 at 10:23 AM, Stefan Eissing wrote: >>> This looks nice for HTTP/1.1, but what about other protocols? Do I read it >>> correctly that any pending data downstream will reopen the connection? >> >> Hmm, I did not think about mod_proxy_h2, but correct (I'd rather say >> upstream though, backend side). >> Are there cases where some backend data are available before the >> request is sent? > > In HTTP/2, yes. For example a PING frame might be waiting, or a > SETTINGS change. Most backends will have short timeouts for answers > to these (SETTINGS is expected to be ACKed soonish), but races may > happen. Also, HTTP/2 extensions with new frame types that need to be > ignored when not known may get received here. OK, so we probably should get rid of the call to ap_proxy_ssl_connection_cleanup() in proxy_http2_handler (and also in proxy_http_handler with my latest changes since the same check is now available in ap_proxy_check_connection). Regards, Yann.
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
On Mon, Jun 27, 2016 at 6:00 PM, Yann Ylavic wrote: > > PR 57832 does not depend on this (race) condition, hence this change > (r1750301, or actually its successor since I'll revert it for a less > intrusive version) could be applied to 2.4.x without r1656259. New commit is r1750392. I'll let it linger in trunk (for review) before any backport proposal...
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
On Mon, Jun 27, 2016 at 3:17 PM, Jim Jagielski wrote: > Doesn't this depend on: > > trunk patch: http://svn.apache.org/r1656259 > http://svn.apache.org/r1656359 (CHANGES entry) > > which, in STATUS, is tagged as still being worked? I don't think so, r1656259 is more about reducing the time frame between the backend connection check and its effective reuse (for requests with a body which may be long to read or retained by some input filter). PR 57832 does not depend on this (race) condition, hence this change (r1750301, or actually its successor since I'll revert it for a less intrusive version) could be applied to 2.4.x without r1656259. No hurry though, this is mainly to mitigate a (potential) defect on the backend side only, it could be included in > 2.4.23 IMHO. Regards, Yann.
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
Doesn't this depend on:
trunk patch: http://svn.apache.org/r1656259
http://svn.apache.org/r1656359 (CHANGES entry)
which, in STATUS, is tagged as still being worked?
> On Jun 27, 2016, at 4:23 AM, Stefan Eissing wrote:
>
> This looks nice for HTTP/1.1, but what about other protocols? Do I read it
> correctly that any pending data downstream will reopen the connection?
>
>> Am 27.06.2016 um 10:00 schrieb yla...@apache.org:
>>
>> Author: ylavic
>> Date: Mon Jun 27 08:00:30 2016
>> New Revision: 1750301
>>
>> URL: http://svn.apache.org/viewvc?rev=1750301&view=rev
>> Log:
>> mod_proxy: don't reuse backend connections with data available before the
>> request is sent. PR 57832.
>>
>> Modified:
>> httpd/httpd/trunk/modules/proxy/mod_proxy.h
>> httpd/httpd/trunk/modules/proxy/proxy_util.c
>>
>> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.h?rev=1750301&r1=1750300&r2=1750301&view=diff
>> ==
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Mon Jun 27 08:00:30 2016
>> @@ -271,6 +271,7 @@ typedef struct {
>>unsigned int inreslist:1; /* connection in apr_reslist? */
>>const char *uds_path;/* Unix domain socket path */
>>const char *ssl_hostname;/* Hostname (SNI) in use by SSL connection */
>> +apr_bucket_brigade *tmp_bb;
>> } proxy_conn_rec;
>>
>> typedef struct {
>>
>> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1750301&r1=1750300&r2=1750301&view=diff
>> ==
>> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Jun 27 08:00:30 2016
>> @@ -2487,7 +2487,7 @@ ap_proxy_determine_connection(apr_pool_t
>> #endif
>>
>> #if USE_ALTERNATE_IS_CONNECTED && defined(APR_MSG_PEEK)
>> -PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket)
>> +static int get_socket_connected(apr_socket_t *socket)
>> {
>>apr_pollfd_t pfds[1];
>>apr_status_t status;
>> @@ -2514,7 +2514,7 @@ PROXY_DECLARE(int) ap_proxy_is_socket_co
>>status = apr_socket_recvfrom(&unused, socket, APR_MSG_PEEK,
>> &buf[0], &len);
>>if (status == APR_SUCCESS && len)
>> -return 1;
>> +return 2;
>>else
>>return 0;
>>}
>> @@ -2525,7 +2525,7 @@ PROXY_DECLARE(int) ap_proxy_is_socket_co
>>
>> }
>> #else
>> -PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket)
>> +static int is_socket_connnected(apr_socket_t *socket)
>>
>> {
>>apr_size_t buffer_len = 1;
>> @@ -2544,12 +2544,19 @@ PROXY_DECLARE(int) ap_proxy_is_socket_co
>>|| APR_STATUS_IS_ECONNRESET(socket_status)) {
>>return 0;
>>}
>> +else if (status == APR_SUCCESS && buffer_len) {
>> +return 2;
>> +}
>>else {
>>return 1;
>>}
>> }
>> #endif /* USE_ALTERNATE_IS_CONNECTED */
>>
>> +PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket)
>> +{
>> +return get_socket_connected(socket) != 0;
>> +}
>>
>> /*
>> * Send a HTTP CONNECT request to a forward proxy.
>> @@ -2716,7 +2723,35 @@ PROXY_DECLARE(int) ap_proxy_connect_back
>>(proxy_server_conf *) ap_get_module_config(sconf, &proxy_module);
>>
>>if (conn->sock) {
>> -if (!(connected = ap_proxy_is_socket_connected(conn->sock))) {
>> +conn_rec *c = conn->connection;
>> +if (!c) {
>> +connected = get_socket_connected(conn->sock);
>> +}
>> +else {
>> +if (conn->tmp_bb == NULL) {
>> +conn->tmp_bb = apr_brigade_create(c->pool, c->bucket_alloc);
>> +}
>> +rv = ap_get_brigade(c->input_filters, conn->tmp_bb,
>> +AP_MODE_SPECULATIVE, APR_NONBLOCK_READ, 1);
>> +if (rv == APR_SUCCESS) {
>> +apr_off_t len = 0;
>> +apr_brigade_length(conn->tmp_bb, 0, &len);
>> +if (len) {
>> +connected = 2;
>> +}
>> +else {
>> +connected = 1;
>> +}
>> +}
>> +else if (APR_STATUS_IS_EAGAIN(rv)) {
>> +connected = 1;
>> +}
>> +else {
>> +connected = 0;
>> +}
>> +apr_brigade_cleanup(conn->tmp_bb);
>> +}
>> +if (connected != 1) {
>>/* This clears conn->scpool (and associated data), so backup and
>> * restore any ssl_hostname for this connection set earlier by
>> * ap_proxy_determine_connection().
>> @@ -2728,9 +2763,17 @@ PROXY_D
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
> Am 27.06.2016 um 10:41 schrieb Yann Ylavic : > > On Mon, Jun 27, 2016 at 10:23 AM, Stefan Eissing wrote: >> This looks nice for HTTP/1.1, but what about other protocols? Do I read it >> correctly that any pending data downstream will reopen the connection? > > Hmm, I did not think about mod_proxy_h2, but correct (I'd rather say > upstream though, backend side). > Are there cases where some backend data are available before the > request is sent? In HTTP/2, yes. For example a PING frame might be waiting, or a SETTINGS change. Most backends will have short timeouts for answers to these (SETTINGS is expected to be ACKed soonish), but races may happen. Also, HTTP/2 extensions with new frame types that need to be ignored when not known may get received here. Again, the added check is very good for HTTP/1, but not for protocols that already have request/response protection and delimiters. Can we see which proxy module is involved on the connection? Do we see the scheme there? That might be a way to whitelist the check. -Stefan
RE: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
> -Original Message- > From: Stefan Eissing [mailto:ste...@eissing.org] > Sent: Montag, 27. Juni 2016 10:24 > To: dev@httpd.apache.org > Subject: Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: > mod_proxy.h proxy_util.c > > This looks nice for HTTP/1.1, but what about other protocols? Do I read it > correctly that any pending data downstream will reopen the connection? Agreed. This is a severe change in behaviour and does not let other protocols handle this in their own way. For them pending data might be fine. So especially I wouldn't backport it to 2.4.x in this state. Regards Rüdiger
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
On Mon, Jun 27, 2016 at 10:23 AM, Stefan Eissing wrote: > This looks nice for HTTP/1.1, but what about other protocols? Do I read it > correctly that any pending data downstream will reopen the connection? Hmm, I did not think about mod_proxy_h2, but correct (I'd rather say upstream though, backend side). Are there cases where some backend data are available before the request is sent?
Re: svn commit: r1750301 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h proxy_util.c
This looks nice for HTTP/1.1, but what about other protocols? Do I read it
correctly that any pending data downstream will reopen the connection?
> Am 27.06.2016 um 10:00 schrieb yla...@apache.org:
>
> Author: ylavic
> Date: Mon Jun 27 08:00:30 2016
> New Revision: 1750301
>
> URL: http://svn.apache.org/viewvc?rev=1750301&view=rev
> Log:
> mod_proxy: don't reuse backend connections with data available before the
> request is sent. PR 57832.
>
> Modified:
>httpd/httpd/trunk/modules/proxy/mod_proxy.h
>httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.h?rev=1750301&r1=1750300&r2=1750301&view=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Mon Jun 27 08:00:30 2016
> @@ -271,6 +271,7 @@ typedef struct {
> unsigned int inreslist:1; /* connection in apr_reslist? */
> const char *uds_path;/* Unix domain socket path */
> const char *ssl_hostname;/* Hostname (SNI) in use by SSL connection */
> +apr_bucket_brigade *tmp_bb;
> } proxy_conn_rec;
>
> typedef struct {
>
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1750301&r1=1750300&r2=1750301&view=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Jun 27 08:00:30 2016
> @@ -2487,7 +2487,7 @@ ap_proxy_determine_connection(apr_pool_t
> #endif
>
> #if USE_ALTERNATE_IS_CONNECTED && defined(APR_MSG_PEEK)
> -PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket)
> +static int get_socket_connected(apr_socket_t *socket)
> {
> apr_pollfd_t pfds[1];
> apr_status_t status;
> @@ -2514,7 +2514,7 @@ PROXY_DECLARE(int) ap_proxy_is_socket_co
> status = apr_socket_recvfrom(&unused, socket, APR_MSG_PEEK,
> &buf[0], &len);
> if (status == APR_SUCCESS && len)
> -return 1;
> +return 2;
> else
> return 0;
> }
> @@ -2525,7 +2525,7 @@ PROXY_DECLARE(int) ap_proxy_is_socket_co
>
> }
> #else
> -PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket)
> +static int is_socket_connnected(apr_socket_t *socket)
>
> {
> apr_size_t buffer_len = 1;
> @@ -2544,12 +2544,19 @@ PROXY_DECLARE(int) ap_proxy_is_socket_co
> || APR_STATUS_IS_ECONNRESET(socket_status)) {
> return 0;
> }
> +else if (status == APR_SUCCESS && buffer_len) {
> +return 2;
> +}
> else {
> return 1;
> }
> }
> #endif /* USE_ALTERNATE_IS_CONNECTED */
>
> +PROXY_DECLARE(int) ap_proxy_is_socket_connected(apr_socket_t *socket)
> +{
> +return get_socket_connected(socket) != 0;
> +}
>
> /*
> * Send a HTTP CONNECT request to a forward proxy.
> @@ -2716,7 +2723,35 @@ PROXY_DECLARE(int) ap_proxy_connect_back
> (proxy_server_conf *) ap_get_module_config(sconf, &proxy_module);
>
> if (conn->sock) {
> -if (!(connected = ap_proxy_is_socket_connected(conn->sock))) {
> +conn_rec *c = conn->connection;
> +if (!c) {
> +connected = get_socket_connected(conn->sock);
> +}
> +else {
> +if (conn->tmp_bb == NULL) {
> +conn->tmp_bb = apr_brigade_create(c->pool, c->bucket_alloc);
> +}
> +rv = ap_get_brigade(c->input_filters, conn->tmp_bb,
> +AP_MODE_SPECULATIVE, APR_NONBLOCK_READ, 1);
> +if (rv == APR_SUCCESS) {
> +apr_off_t len = 0;
> +apr_brigade_length(conn->tmp_bb, 0, &len);
> +if (len) {
> +connected = 2;
> +}
> +else {
> +connected = 1;
> +}
> +}
> +else if (APR_STATUS_IS_EAGAIN(rv)) {
> +connected = 1;
> +}
> +else {
> +connected = 0;
> +}
> +apr_brigade_cleanup(conn->tmp_bb);
> +}
> +if (connected != 1) {
> /* This clears conn->scpool (and associated data), so backup and
> * restore any ssl_hostname for this connection set earlier by
> * ap_proxy_determine_connection().
> @@ -2728,9 +2763,17 @@ PROXY_DECLARE(int) ap_proxy_connect_back
> }
>
> socket_cleanup(conn);
> -ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(00951)
> - "%s: backend socket is disconnected.",
> - proxy_function);
> +if (!connected) {
> +ap_log_error(APLOG_MARK,
