httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: [PATCH] ap_proxy_http_process_response double lifetime transform
Date Thu, 12 Dec 2013 21:56:59 GMT
On Thu, Dec 12, 2013 at 7:14 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

> On Thu, Dec 12, 2013 at 6:45 PM, Yann Ylavic <ylavic.dev@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]

Mime
View raw message