Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (nike.apache.org: domain of ylavic.dev@gmail.com designates
 209.85.213.173 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CAL7zx_e=K-suj1by7DL4Fi+gaLixmpumAPn+Q+7SVY9HQ3JxeA@mail.gmail.com>
References: 
 <CALK=YjOF=gKLgO6R5E=3683vTg7498_Op_q819KFE50pM4hamQ@mail.gmail.com>
	<CAKQ1sVO74ub2k53QmT9Hq+g=ELuC+7wf0LKj_1P8bn=G+focqQ@mail.gmail.com>
	<CALK=YjNQyOssbkYjVxjNCmj4fq4P9KY0vtMLo0Sk4gv-uZDM3A@mail.gmail.com>
	<CAKQ1sVMMmbBmNKvctNsDGiVK-j_GWdL6pLzEz3m2JvmRGBHT6g@mail.gmail.com>
	<CAKQ1sVNLVXr0VgtwccPNwcDCcGGhVyGRp72SBhsfjk-x_Ons1g@mail.gmail.com>
	<CALK=YjPfrtvZ47ryDzyi3E2_jkAkHht7MZovfsPXujLNjcPtGg@mail.gmail.com>
	<CAKQ1sVPRUkQagV+588u=bqPAtOiynm5wBe3jMusdX0Lcatk=VA@mail.gmail.com>
	<CAKQ1sVOA-WDV=vfYn1gpgGYfmmDdTYoEPVQ58Rrwtqm0Bf0OLw@mail.gmail.com>
	<CALK=YjMSr5oNQ4Ej5r7F2B7cpssEXTpD=Xp7Mougj8nDXWR=PQ@mail.gmail.com>
	<CAKQ1sVOPcPyiV6u-iS8CLC2cxbmTTH5hGdkGg7==Ywnoa75OnQ@mail.gmail.com>
	<CAKQ1sVNOR32cELU6HhcfKfQvpTyXdFVzVZDsFqW-gr_GmCXrOA@mail.gmail.com>
	<CALK=YjMx9NT1u3JwmNXpkiLs6g-hPRk1xPdG-3-j2tKTbro2-Q@mail.gmail.com>
	<CAKQ1sVNJHxDFQwB1raRcp4FQPpg7U5wdDAe5DsKxruPRKgt6_A@mail.gmail.com>
	<CALK=YjNEn6PSAmxfPzVL-nDFh8A5+9bmLFP-MfFKS7MWyypODA@mail.gmail.com>
	<CAL7zx_e=K-suj1by7DL4Fi+gaLixmpumAPn+Q+7SVY9HQ3JxeA@mail.gmail.com>
Date: Sun, 11 May 2014 18:35:02 +0200
Message-ID: 
 <CAKQ1sVNNj49PBvhdeLtXq=TNqHBaHS-WaO-bf44KzVMGx0nwXQ@mail.gmail.com>
Subject: Re: stop copying footers to r->headers_in?
From: Yann Ylavic <ylavic.dev@gmail.com>
To: httpd <dev@httpd.apache.org>
Content-Type: text/plain; charset=UTF-8

On Fri, May 9, 2014 at 5:01 PM, Edward Lu <chaosed0@gmail.com> wrote:
> Here's a 2.2.x backport of Yann's patch with a directive, MergeTrailers,
> added in to opt-in to the old behavior. By default, it doesn't merge the
> trailers. I'm working on adding the capability to mod_log_config to log the
> trailers, but I figured I'd post this patch first to get some review.
>

(paste from your patch with comments below)

>
> Index: modules/http/http_request.c
> ===================================================================
> --- modules/http/http_request.c    (revision 1593540)
> +++ modules/http/http_request.c    (working copy)
> @@ -384,8 +384,10 @@
>      new->main            = r->main;
>
>      new->headers_in      = r->headers_in;
> +    new->trailers_in     = r->trailers_in;
>      new->headers_out     = apr_table_make(r->pool, 12);
>      new->err_headers_out = r->err_headers_out;
> +    new->trailers_out    = r->trailers_out;

Here my patch used :
  new->trailers_out = apr_table_make(r->pool, 5);
so to start internal redirect(s) with fresh new outgoing trailers.
I think the previous body (if any) is likely to be consumed when
ap_internal_redirect[_handler] is called.
I don't know what's the correct thing to do, though, my understanding
is that err_headers_out are preserved here because they may contain
(set-)cookies or alike, but the final response will be the one of the
redirect (headers and body, if any).

>      new->subprocess_env  = rename_original_env(r->pool, r->subprocess_env);
>      new->notes           = apr_table_make(r->pool, 5);
>
> @@ -495,6 +497,8 @@
>                                         r->headers_out);
>      r->err_headers_out = apr_table_overlay(r->pool, rr->err_headers_out,
>                                             r->err_headers_out);
> +    r->err_headers_out = apr_table_overlay(r->pool, rr->trailers_out,
> +                                           r->trailers_out);

Probably a typo here, should be r->trailers_out = ....


Otherwise, I agree with Eric that ap_http_filter() should save and
restore the request's status/headers/notes by default, and only merge
the trailers if configured to.
Restoring the status and notes (set before the body is read) seems
unconditional to me.

Something like the following patch (against 2.2.x), which also
fixes/improves (compared to my previous patch) the unsetting of the
"error-notes" before/after the call to ap_get_mime_headers().

Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c    (revision 1593786)
+++ modules/http/http_filters.c    (working copy)
@@ -403,13 +407,61 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
             }

             if (!ctx->remaining) {
-                /* Handle trailers by calling ap_get_mime_headers again! */
+                /* Handle trailers by calling ap_get_mime_headers again,
+                 * but preserve original status and incoming headers (unless
+                 * MergeTrailers in on) by saving/restoring them before/after.
+                 */
+                const char *error_notes = NULL;
+                int saved_status = f->r->status;
+                apr_table_t *saved_headers_in = f->r->headers_in;
+                const char *saved_error_notes = apr_table_get(f->r->notes,
+                                                              "error-notes");
+
+                f->r->status = HTTP_OK;
+                f->r->headers_in = f->r->trailers_in;
+                apr_table_clear(f->r->headers_in);
+                if (saved_error_notes) {
+                    apr_table_unset(f->r->notes, "error-notes");
+                }
+
                 ctx->state = BODY_NONE;
                 ap_get_mime_headers(f->r);
-                e = apr_bucket_eos_create(f->c->bucket_alloc);
-                APR_BRIGADE_INSERT_TAIL(b, e);
-                ctx->eos_sent = 1;
-                return APR_SUCCESS;
+                if (f->r->status == HTTP_OK) {
+                    e = apr_bucket_eos_create(f->c->bucket_alloc);
+                    APR_BRIGADE_INSERT_TAIL(b, e);
+                    ctx->eos_sent = 1;
+                    rv = APR_SUCCESS;
+                }
+                else {
+                    error_notes = apr_table_get(f->r->notes, "error-notes");
+                    ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r,
+                                  "Error while reading HTTP trailer: %i%s%s",
+                                  f->r->status, error_notes ? ": " : "",
+                                  error_notes ? error_notes : "");
+                    rv = APR_EINVAL;
+                }
+
+                /* By default, don't merge in the trailers
+                 * (treat UNSET as DISABLE) */
+                if (conf->merge_trailers != AP_MERGE_TRAILERS_ENABLE) {
+                    f->r->headers_in = saved_headers_in;
+                }
+                else {
+                    f->r->headers_in = apr_table_overlay(f->r->pool,
+                                                         saved_headers_in,
+                                                         f->r->headers_in);
+                    apr_table_clear(f->r->trailers_in);
+                }
+                if (saved_error_notes) {
+                    apr_table_setn(f->r->notes, "error-notes",
+                                   saved_error_notes);
+                }
+                else if (error_notes) {
+                    apr_table_unset(f->r->notes, "error-notes");
+                }
+                f->r->status = saved_status;
+
+                return rv;
             }
         }
     }
[END]

Regards,
Yann.