You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2013/12/12 18:45:01 UTC
[PATCH] ap_proxy_http_process_response double lifetime transform
Hi devs,
This was pointed out by Joe Orton's comment at
https://issues.apache.org/bugzilla/show_bug.cgi?id=50335#c40.
Here is a proposal (patch against ap_proxy_http_process_response) to
address the double lifetime transformation of the buckets from the backend
when its connection is released early (on EOS, before the last buckets are
forwarded to the client).
Regards,
Yann.
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1550128)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -1729,11 +1729,9 @@ int ap_proxy_http_process_response(apr_pool_t * p,
break;
}
- /* Switch the allocator lifetime of the buckets */
- proxy_buckets_lifetime_transform(r, bb, pass_bb);
-
/* found the last brigade? */
- if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(pass_bb))) {
+ if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
+ apr_bucket_brigade *swap_bb;
/* signal that we must leave */
finish = TRUE;
@@ -1743,17 +1741,18 @@ int ap_proxy_http_process_response(apr_pool_t * p,
* Force a setaside so these transient buckets
become heap
* buckets that live as long as the request.
*/
- for (e = APR_BRIGADE_FIRST(pass_bb); e
- != APR_BRIGADE_SENTINEL(pass_bb); e
- = APR_BUCKET_NEXT(e)) {
+ for (e = APR_BRIGADE_FIRST(bb);
+ e != APR_BRIGADE_SENTINEL(bb);
+ e = APR_BUCKET_NEXT(e)) {
apr_bucket_setaside(e, r->pool);
}
- /* finally it is safe to clean up the brigade from
the
- * connection pool, as we have forced a setaside
on all
- * buckets.
+ /* swap bb and pass_bb to keep the logic below,
where
+ * pass_bb is the one finally passed to the client.
*/
- apr_brigade_cleanup(bb);
+ swap_bb = bb;
+ bb = pass_bb;
+ pass_bb = swap_bb;
/* make sure we release the backend connection as
soon
* as we know we are done, so that the backend
isn't
@@ -1764,8 +1763,11 @@ int ap_proxy_http_process_response(apr_pool_t * p,
backend, r->server);
/* Ensure that the backend is not reused */
*backend_ptr = NULL;
-
}
+ else {
+ /* Switch the allocator lifetime of the buckets */
+ proxy_buckets_lifetime_transform(r, bb, pass_bb);
+ }
/* try send what we read */
if (ap_pass_brigade(r->output_filters, pass_bb) !=
APR_SUCCESS
[EOS]
Re: [PATCH] ap_proxy_http_process_response double lifetime transform
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Dec 12, 2013 at 7:14 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Dec 12, 2013 at 6:45 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
>> Here is a proposal (patch against ap_proxy_http_process_response) to
>> address the double lifetime transformation of the buckets from the backend
>> when its connection is released early (on EOS, before the last buckets are
>> forwarded to the client).
>>
>
> Maybe this version is more safe should bb be (later) created with a
> different pool/bucket_alloc than pass_bb :
>
> [...]
>
> - for (e = APR_BRIGADE_FIRST(pass_bb); e
> - != APR_BRIGADE_SENTINEL(pass_bb); e
> - = APR_BUCKET_NEXT(e)) {
> - apr_bucket_setaside(e, r->pool);
> + rv = ap_save_brigade(NULL, &pass_bb, &bb,
> r->pool);
> + if (rv != APR_SUCCESS) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
> APLOGNO()
> + "Failed to save final output
> brigade");
> + return HTTP_INTERNAL_SERVER_ERROR;
>
> }
>
This seems not a good idea to return an error here, so this patch is no
good either. Let me try a third...
Since the original code did not check the return value, I assume the patch
does not have to :
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1550532)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -1729,12 +1729,8 @@ int ap_proxy_http_process_response(apr_pool_t * p,
break;
}
- /* Switch the allocator lifetime of the buckets */
- proxy_buckets_lifetime_transform(r, bb, pass_bb);
-
/* found the last brigade? */
- if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(pass_bb))) {
-
+ if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
/* signal that we must leave */
finish = TRUE;
@@ -1742,19 +1738,12 @@ int ap_proxy_http_process_response(apr_pool_t * p,
* data that lives only as long as the backend
connection.
* Force a setaside so these transient buckets
become heap
* buckets that live as long as the request.
+ * Calling ap_save_brigade with NULL as filter is
OK,
+ * because pass_bb brigade already has been
created and
+ * does not need to get created by ap_save_brigade.
*/
- for (e = APR_BRIGADE_FIRST(pass_bb); e
- != APR_BRIGADE_SENTINEL(pass_bb); e
- = APR_BUCKET_NEXT(e)) {
- apr_bucket_setaside(e, r->pool);
- }
+ ap_save_brigade(NULL, &pass_bb, &bb, r->pool);
- /* finally it is safe to clean up the brigade from
the
- * connection pool, as we have forced a setaside
on all
- * buckets.
- */
- apr_brigade_cleanup(bb);
-
/* make sure we release the backend connection as
soon
* as we know we are done, so that the backend
isn't
* left waiting for a slow client to eventually
@@ -1764,8 +1753,11 @@ int ap_proxy_http_process_response(apr_pool_t * p,
backend, r->server);
/* Ensure that the backend is not reused */
*backend_ptr = NULL;
-
}
+ else {
+ /* Switch the allocator lifetime of the buckets */
+ proxy_buckets_lifetime_transform(r, bb, pass_bb);
+ }
/* try send what we read */
if (ap_pass_brigade(r->output_filters, pass_bb) !=
APR_SUCCESS
[EOS]
Re: [PATCH] ap_proxy_http_process_response double lifetime transform
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Dec 12, 2013 at 6:45 PM, Yann Ylavic <yl...@gmail.com> wrote:
> Here is a proposal (patch against ap_proxy_http_process_response) to
> address the double lifetime transformation of the buckets from the backend
> when its connection is released early (on EOS, before the last buckets are
> forwarded to the client).
>
Maybe this version is more safe should bb be (later) created with a
different pool/bucket_alloc than pass_bb :
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1550128)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -1729,12 +1729,8 @@ int ap_proxy_http_process_response(apr_pool_t * p,
break;
}
- /* Switch the allocator lifetime of the buckets */
- proxy_buckets_lifetime_transform(r, bb, pass_bb);
-
/* found the last brigade? */
- if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(pass_bb))) {
-
+ if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
/* signal that we must leave */
finish = TRUE;
@@ -1742,19 +1738,17 @@ int ap_proxy_http_process_response(apr_pool_t * p,
* data that lives only as long as the backend
connection.
* Force a setaside so these transient buckets
become heap
* buckets that live as long as the request.
+ * Calling ap_save_brigade with NULL as filter is
OK,
+ * because pass_bb brigade already has been
created and
+ * does not need to get created by ap_save_brigade.
*/
- for (e = APR_BRIGADE_FIRST(pass_bb); e
- != APR_BRIGADE_SENTINEL(pass_bb); e
- = APR_BUCKET_NEXT(e)) {
- apr_bucket_setaside(e, r->pool);
+ rv = ap_save_brigade(NULL, &pass_bb, &bb, r->pool);
+ if (rv != APR_SUCCESS) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
APLOGNO()
+ "Failed to save final output
brigade");
+ return HTTP_INTERNAL_SERVER_ERROR;
}
- /* finally it is safe to clean up the brigade from
the
- * connection pool, as we have forced a setaside
on all
- * buckets.
- */
- apr_brigade_cleanup(bb);
-
/* make sure we release the backend connection as
soon
* as we know we are done, so that the backend
isn't
* left waiting for a slow client to eventually
@@ -1764,8 +1758,11 @@ int ap_proxy_http_process_response(apr_pool_t * p,
backend, r->server);
/* Ensure that the backend is not reused */
*backend_ptr = NULL;
-
}
+ else {
+ /* Switch the allocator lifetime of the buckets */
+ proxy_buckets_lifetime_transform(r, bb, pass_bb);
+ }
/* try send what we read */
if (ap_pass_brigade(r->output_filters, pass_bb) !=
APR_SUCCESS
[EOS]
Re: [PATCH] ap_proxy_http_process_response double lifetime transform
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Dec 13, 2013, at 4:33 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> Yann Ylavic wrote:
>> Hi devs,
>>
>> This was pointed out by Joe Orton's comment at https://issues.apache.org/bugzilla/show_bug.cgi?id=50335#c40.
>>
>> Here is a proposal (patch against ap_proxy_http_process_response) to address the double lifetime transformation of the
>> buckets from the backend when its connection is released early (on EOS, before the last buckets are forwarded to the
>> client).
>
> In this case the buckets that would be sent to the output filters would have been allocated from the wrong allocator
> which can lead to crashes. IMHO the code is fine as is. It might be possible to optimize here, but the patch below just
> sents down buckets with the wrong lifetime down the chain.
>
+1.
Re: [PATCH] ap_proxy_http_process_response double lifetime transform
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Dec 13, 2013 at 10:33 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> Yann Ylavic wrote:
> > Hi devs,
> >
> > This was pointed out by Joe Orton's comment at
> https://issues.apache.org/bugzilla/show_bug.cgi?id=50335#c40.
> >
> > Here is a proposal (patch against ap_proxy_http_process_response) to
> address the double lifetime transformation of the
> > buckets from the backend when its connection is released early (on EOS,
> before the last buckets are forwarded to the
> > client).
>
> In this case the buckets that would be sent to the output filters would
> have been allocated from the wrong allocator
> which can lead to crashes. IMHO the code is fine as is. It might be
> possible to optimize here, but the patch below just
> sents down buckets with the wrong lifetime down the chain.
>
Yes you are right, the patch only cares about the pool...
I guess the optimisation should be in proxy_buckets_lifetime_transform(),
but an efficient APR way of copying/setting aside buckets (including the
bucket_alloc, ie. refcounting), depending on the bucket type, would be
welcome...
>
> Regards
>
> Rüdiger
>
>
Re: [PATCH] ap_proxy_http_process_response double lifetime transform
Posted by Ruediger Pluem <rp...@apache.org>.
Yann Ylavic wrote:
> Hi devs,
>
> This was pointed out by Joe Orton's comment at https://issues.apache.org/bugzilla/show_bug.cgi?id=50335#c40.
>
> Here is a proposal (patch against ap_proxy_http_process_response) to address the double lifetime transformation of the
> buckets from the backend when its connection is released early (on EOS, before the last buckets are forwarded to the
> client).
In this case the buckets that would be sent to the output filters would have been allocated from the wrong allocator
which can lead to crashes. IMHO the code is fine as is. It might be possible to optimize here, but the patch below just
sents down buckets with the wrong lifetime down the chain.
Regards
Rüdiger