Merge r1775195 from trunk:

http_header_filter: on check_headers() failure, use AP_FILTER_ERROR and EOC
semantics to respectively warn the caller and cleanly terminate the connection
afterwards.

Suggested by: rpluem

Submitted by: ylavic
Reviewed/backported by: jim

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1775828 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/STATUS b/STATUS
index 0aa7ea6d3a7441af22ab1531b19e15a1e7785fcb..bc7e1629f079b61ade74e074135fdf5209ee5ce2 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -120,13 +120,6 @@ PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
 
-  *) mod_http: Optimize bad headers handling workflow
-     Trunk version of patch:
-      http://svn.apache.org/r1775195
-     Backport version for 2.4.x of patch:
-      Trunk version of patch works
-     +1: rpluem, jim, ylavic
-
 
 PATCHES PROPOSED TO BACKPORT FROM TRUNK:
   [ New proposals should be added at the end of the list ]

diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c
index f6a19aef75d930e20b3c4e18abd657743993a8f4..595d3c4a806008ddf4a0772f3e41812dc9cfd941 100644 (file)
--- a/modules/http/http_filters.c
+++ b/modules/http/http_filters.c
@@ -1243,7 +1243,6 @@ AP_DECLARE_NONSTD(int) ap_send_http_trace(request_rec *r)
 
 typedef struct header_filter_ctx {
     int headers_sent;
-    int headers_error;
 } header_filter_ctx;
 
 AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
@@ -1259,23 +1258,22 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
     header_filter_ctx *ctx = f->ctx;
     const char *ctype;
     ap_bucket_error *eb = NULL;
-    apr_bucket *eos = NULL;
+    apr_status_t rv = APR_SUCCESS;
+    int recursive_error = 0;
 
     AP_DEBUG_ASSERT(!r->main);
 
     if (!ctx) {
         ctx = f->ctx = apr_pcalloc(r->pool, sizeof(header_filter_ctx));
     }
-    if (ctx->headers_sent) {
+    else if (ctx->headers_sent) {
         /* Eat body if response must not have one. */
         if (r->header_only || r->status == HTTP_NO_CONTENT) {
             apr_brigade_cleanup(b);
             return APR_SUCCESS;
         }
     }
-    else if (!ctx->headers_error && !check_headers(r)) {
-        ctx->headers_error = 1;
-    }
+
     for (e = APR_BRIGADE_FIRST(b);
          e != APR_BRIGADE_SENTINEL(b);
          e = APR_BUCKET_NEXT(e))
@@ -1292,23 +1290,15 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
             ap_remove_output_filter(f);
             return ap_pass_brigade(f->next, b);
         }
-        if (ctx->headers_error && APR_BUCKET_IS_EOS(e)) {
-            eos = e;
-        }
     }
-    if (ctx->headers_error) {
-        if (!eos) {
-            /* Eat body until EOS */
-            apr_brigade_cleanup(b);
-            return APR_SUCCESS;
-        }
 
+    if (!ctx->headers_sent && !check_headers(r)) {
         /* We may come back here from ap_die() below,
          * so clear anything from this response.
          */
-        ctx->headers_error = 0;
         apr_table_clear(r->headers_out);
         apr_table_clear(r->err_headers_out);
+        apr_brigade_cleanup(b);
 
         /* Don't recall ap_die() if we come back here (from its own internal
          * redirect or error response), otherwise we can end up in infinite
@@ -1316,17 +1306,18 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
          * empty body (EOS only).
          */
         if (!check_headers_recursion(r)) {
-            apr_brigade_cleanup(b);
             ap_die(HTTP_INTERNAL_SERVER_ERROR, r);
             return AP_FILTER_ERROR;
         }
-        APR_BUCKET_REMOVE(eos);
-        apr_brigade_cleanup(b);
-        APR_BRIGADE_INSERT_TAIL(b, eos);
         r->status = HTTP_INTERNAL_SERVER_ERROR;
+        e = ap_bucket_eoc_create(c->bucket_alloc);
+        APR_BRIGADE_INSERT_TAIL(b, e);
+        e = apr_bucket_eos_create(c->bucket_alloc);
+        APR_BRIGADE_INSERT_TAIL(b, e);
         r->content_type = r->content_encoding = NULL;
         r->content_languages = NULL;
         ap_set_content_length(r, 0);
+        recursive_error = 1;
     }
     else if (eb) {
         int status;
@@ -1339,7 +1330,8 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
     if (r->assbackwards) {
         r->sent_bodyct = 1;
         ap_remove_output_filter(f);
-        return ap_pass_brigade(f->next, b);
+        rv = ap_pass_brigade(f->next, b);
+        goto out;
     }
 
     /*
@@ -1477,12 +1469,15 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
 
     terminate_header(b2);
 
-    ap_pass_brigade(f->next, b2);
+    rv = ap_pass_brigade(f->next, b2);
+    if (rv != APR_SUCCESS) {
+        goto out;
+    }
     ctx->headers_sent = 1;
 
     if (r->header_only || r->status == HTTP_NO_CONTENT) {
         apr_brigade_cleanup(b);
-        return APR_SUCCESS;
+        goto out;
     }
 
     r->sent_bodyct = 1;         /* Whatever follows is real body stuff... */
@@ -1500,7 +1495,12 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
      * brigade won't be chunked properly.
      */
     ap_remove_output_filter(f);
-    return ap_pass_brigade(f->next, b);
+    rv = ap_pass_brigade(f->next, b);
+out:
+    if (recursive_error) {
+        return AP_FILTER_ERROR;
+    }
+    return rv;
 }
 
 /*