httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Querna <p...@querna.org>
Subject Re: svn commit: r821477 - in /httpd/httpd/trunk: CHANGES modules/http/byterange_filter.c modules/http/http_filters.c server/core_filters.c
Date Sun, 04 Oct 2009 08:14:47 GMT
On Sun, Oct 4, 2009 at 1:08 AM,  <sf@apache.org> wrote:
> Author: sf
> Date: Sun Oct  4 08:08:50 2009
> New Revision: 821477
>
> URL: http://svn.apache.org/viewvc?rev=821477&view=rev
> Log:
> Make sure to not destroy bucket brigades that have been created by earlier
> filters. Otherwise the pool cleanups would be removed causing potential memory
> leaks later on.
>

I am not sure these changes make sense.  The 'traditional' API view
says that brigades passed down the output fitler chain should be
destroyed, not cleared -- please see the thread started with this
message:
<http://mail-archives.apache.org/mod_mbox/httpd-dev/200504.mbox/%3C9ccfb16df2220fc31c836bb7cd640554@ricilake.net%3E>

<>
> Modified:
>    httpd/httpd/trunk/CHANGES
>    httpd/httpd/trunk/modules/http/byterange_filter.c
>    httpd/httpd/trunk/modules/http/http_filters.c
>    httpd/httpd/trunk/server/core_filters.c
>
> Modified: httpd/httpd/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=821477&r1=821476&r2=821477&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Sun Oct  4 08:08:50 2009
> @@ -10,6 +10,10 @@
>      mod_proxy_ftp: NULL pointer dereference on error paths.
>      [Stefan Fritsch <sf fritsch.de>, Joe Orton]
>
> +  *) core: Fix potential memory leaks by making sure to not destroy
> +     bucket brigades that have been created by earlier filters.
> +     [Stefan Fritsch]
> +
>   *) core, mod_deflate, mod_sed: Reduce memory usage by reusing bucket
>      brigades in several places. [Stefan Fritsch]
>
>
> Modified: httpd/httpd/trunk/modules/http/byterange_filter.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/byterange_filter.c?rev=821477&r1=821476&r2=821477&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/byterange_filter.c (original)
> +++ httpd/httpd/trunk/modules/http/byterange_filter.c Sun Oct  4 08:08:50 2009
> @@ -307,7 +307,7 @@
>     APR_BRIGADE_INSERT_TAIL(bsend, e);
>
>     /* we're done with the original content - all of our data is in bsend. */
> -    apr_brigade_destroy(bb);
> +    apr_brigade_cleanup(bb);
>
>     /* send our multipart output */
>     return ap_pass_brigade(f->next, bsend);
>
> Modified: httpd/httpd/trunk/modules/http/http_filters.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=821477&r1=821476&r2=821477&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
> +++ httpd/httpd/trunk/modules/http/http_filters.c Sun Oct  4 08:08:50 2009
> @@ -1112,7 +1112,7 @@
>             ctx = f->ctx = apr_pcalloc(r->pool, sizeof(header_filter_ctx));
>         }
>         else if (ctx->headers_sent) {
> -            apr_brigade_destroy(b);
> +            apr_brigade_cleanup(b);
>             return OK;
>         }
>     }
> @@ -1283,7 +1283,7 @@
>     ap_pass_brigade(f->next, b2);
>
>     if (r->header_only) {
> -        apr_brigade_destroy(b);
> +        apr_brigade_cleanup(b);
>         ctx->headers_sent = 1;
>         return OK;
>     }
>
> Modified: httpd/httpd/trunk/server/core_filters.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core_filters.c?rev=821477&r1=821476&r2=821477&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/core_filters.c (original)
> +++ httpd/httpd/trunk/server/core_filters.c Sun Oct  4 08:08:50 2009
> @@ -316,7 +316,7 @@
>  static void setaside_remaining_output(ap_filter_t *f,
>                                       core_output_filter_ctx_t *ctx,
>                                       apr_bucket_brigade *bb,
> -                                      int make_a_copy, conn_rec *c);
> +                                      conn_rec *c);
>
>  static apr_status_t send_brigade_nonblocking(apr_socket_t *s,
>                                              apr_bucket_brigade
*bb,
> @@ -392,19 +392,21 @@
>         }
>     }
>
> +    if (new_bb != NULL) {
> +        bb = new_bb;
> +    }
> +
>     if ((ctx->buffered_bb != NULL) &&
>         !APR_BRIGADE_EMPTY(ctx->buffered_bb)) {
> -        bb = ctx->buffered_bb;
> -        ctx->buffered_bb = NULL;
>         if (new_bb != NULL) {
> -            APR_BRIGADE_CONCAT(bb, new_bb);
> +            APR_BRIGADE_PREPEND(bb, ctx->buffered_bb);
> +        }
> +        else {
> +            bb = ctx->buffered_bb;
>         }
>         c->data_in_output_filters = 0;
>     }
> -    else if (new_bb != NULL) {
> -        bb = new_bb;
> -    }
> -    else {
> +    else if (new_bb == NULL) {
>         return APR_SUCCESS;
>     }
>
> @@ -444,7 +446,7 @@
>             /* The client has aborted the connection */
>             c->aborted = 1;
>         }
> -        setaside_remaining_output(f, ctx, bb, 0, c);
> +        setaside_remaining_output(f, ctx, bb, c);
>         return rv;
>     }
>
> @@ -508,14 +510,14 @@
>         }
>     }
>
> -    setaside_remaining_output(f, ctx, bb, 1, c);
> +    setaside_remaining_output(f, ctx, bb, c);
>     return APR_SUCCESS;
>  }
>
>  static void setaside_remaining_output(ap_filter_t *f,
>                                       core_output_filter_ctx_t *ctx,
>                                       apr_bucket_brigade *bb,
> -                                      int make_a_copy, conn_rec *c)
> +                                      conn_rec *c)
>  {
>     if (bb == NULL) {
>         return;
> @@ -523,20 +525,14 @@
>     remove_empty_buckets(bb);
>     if (!APR_BRIGADE_EMPTY(bb)) {
>         c->data_in_output_filters = 1;
> -        if (make_a_copy) {
> +        if (bb != ctx->buffered_bb) {
>             /* XXX should this use a separate deferred write pool, like
>              * the original ap_core_output_filter?
>              */
>             ap_save_brigade(f, &(ctx->buffered_bb), &bb, c->pool);
> -            apr_brigade_destroy(bb);
> -        }
> -        else {
> -            ctx->buffered_bb = bb;
> +            apr_brigade_cleanup(bb);
>         }
>     }
> -    else {
> -        apr_brigade_destroy(bb);
> -    }
>  }
>
>  #ifndef APR_MAX_IOVEC_SIZE
>
>
>
Mime
View raw message