Merge r1657881, r1665643 from trunk:

http: Make ap_die() robust against any HTTP error code and not modify
response status (finally logged) when nothing is to be done.

ap_die(): follow up to r1657881.

Use log level DEBUG for AP_FILTER_ERROR => HTTP_INTERNAL_SERVER_ERROR.
Submitted by: ylavic
Reviewed/backported by: jim

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

diff --git a/CHANGES b/CHANGES
index dea2424d3ad1b2ab6202b840e0a725ab146d7f0a..79c085824eb3e14f452bad2abc3dea063e0d5cc7 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -134,6 +134,9 @@ Changes with Apache 2.4.13
      a ProxyRemote forward-proxy.  PR 55892.  [Hendrik Harms <hendrik.harms
      gmail com>, William Rowe, Yann Ylavic]
 
+  *) http: Make ap_die() robust against any HTTP error code and not modify
+     response status (finally logged) when nothing is to be done. [Yann Ylavic]
+
   *) mod_proxy_connect/wstunnel: If both client and backend sides get readable
      at the same time, don't lose errors occuring while forwarding on the first
      side when none occurs next on the other side, and abort.  [Yann Ylavic]

diff --git a/STATUS b/STATUS
index a0c97a775d2495c6ea68af4ceef437c892bb3e2e..07a5c94d6fb56a6a50235e4d14510f0b6a7554d5 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -105,12 +105,6 @@ RELEASE SHOWSTOPPERS:
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
-  *) http: Make ap_die() robust against any HTTP error code and not modify
-     response status (finally logged) when nothing is to be done. PR 56035.
-     trunk patch: http://svn.apache.org/r1657881
-                  http://svn.apache.org/r1665643
-     2.4.x patch: trunk works (modulo CHANGES)
-     +1: ylavic, minfrin, jim
 
 
 PATCHES PROPOSED TO BACKPORT FROM TRUNK:

diff --git a/modules/http/http_request.c b/modules/http/http_request.c
index 9f23a9ddec05f9d3a6e6ca58b799540a1960eb6e..7b06def9405573da04ff7712b038c24ac67f7bc0 100644 (file)
--- a/modules/http/http_request.c
+++ b/modules/http/http_request.c
@@ -73,19 +73,22 @@ static void update_r_in_filters(ap_filter_t *f,
     }
 }
 
-AP_DECLARE(void) ap_die(int type, request_rec *r)
+static void ap_die_r(int type, request_rec *r, int recursive_error)
 {
-    int error_index = ap_index_of_response(type);
-    char *custom_response = ap_response_code_string(r, error_index);
-    int recursive_error = 0;
+    char *custom_response;
     request_rec *r_1st_err = r;
 
-    if (type == AP_FILTER_ERROR) {
+    if (type == OK || type == DONE) {
+        ap_finalize_request_protocol(r);
+        return;
+    }
+
+    if (!ap_is_HTTP_VALID_RESPONSE(type)) {
         ap_filter_t *next;
 
         /*
          * Check if we still have the ap_http_header_filter in place. If
-         * this is the case we should not ignore AP_FILTER_ERROR here because
+         * this is the case we should not ignore the error here because
          * it means that we have not sent any response at all and never
          * will. This is bad. Sent an internal server error instead.
          */
@@ -99,8 +102,14 @@ AP_DECLARE(void) ap_die(int type, request_rec *r)
          * next->frec == ap_http_header_filter
          */
         if (next) {
-            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01579)
-                          "Custom error page caused AP_FILTER_ERROR");
+            if (type != AP_FILTER_ERROR) {
+                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01579)
+                              "Invalid response status %i", type);
+            }
+            else {
+                ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02831)
+                              "Response from AP_FILTER_ERROR");
+            }
             type = HTTP_INTERNAL_SERVER_ERROR;
         }
         else {
@@ -108,20 +117,13 @@ AP_DECLARE(void) ap_die(int type, request_rec *r)
         }
     }
 
-    if (type == DONE) {
-        ap_finalize_request_protocol(r);
-        return;
-    }
-
     /*
      * The following takes care of Apache redirects to custom response URLs
      * Note that if we are already dealing with the response to some other
      * error condition, we just report on the original error, and give up on
      * any attempt to handle the other thing "intelligently"...
      */
-    if (r->status != HTTP_OK) {
-        recursive_error = type;
-
+    if (recursive_error != HTTP_OK) {
         while (r_1st_err->prev && (r_1st_err->prev->status != HTTP_OK))
             r_1st_err = r_1st_err->prev;  /* Get back to original error */
 
@@ -141,6 +143,11 @@ AP_DECLARE(void) ap_die(int type, request_rec *r)
 
         custom_response = NULL; /* Do NOT retry the custom thing! */
     }
+    else {
+        int error_index = ap_index_of_response(type);
+        custom_response = ap_response_code_string(r, error_index);
+        recursive_error = 0;
+    }
 
     r->status = type;
 
@@ -216,6 +223,11 @@ AP_DECLARE(void) ap_die(int type, request_rec *r)
     ap_send_error_response(r_1st_err, recursive_error);
 }
 
+AP_DECLARE(void) ap_die(int type, request_rec *r)
+{
+    ap_die_r(type, r, r->status);
+}
+
 static void check_pipeline(conn_rec *c, apr_bucket_brigade *bb)
 {
     if (c->keepalive != AP_CONN_CLOSE && !c->aborted) {
@@ -346,18 +358,7 @@ void ap_process_async_request(request_rec *r)
     apr_thread_mutex_unlock(r->invoke_mtx);
 #endif
 
-    if (access_status == DONE) {
-        /* e.g., something not in storage like TRACE */
-        access_status = OK;
-    }
-
-    if (access_status == OK) {
-        ap_finalize_request_protocol(r);
-    }
-    else {
-        r->status = HTTP_OK;
-        ap_die(access_status, r);
-    }
+    ap_die_r(access_status, r, HTTP_OK);
 
     ap_process_request_after_handler(r);
 }
@@ -640,8 +641,8 @@ AP_DECLARE(void) ap_internal_fast_redirect(request_rec *rr, request_rec *r)
 
 AP_DECLARE(void) ap_internal_redirect(const char *new_uri, request_rec *r)
 {
-    request_rec *new = internal_internal_redirect(new_uri, r);
     int access_status;
+    request_rec *new = internal_internal_redirect(new_uri, r);
 
     AP_INTERNAL_REDIRECT(r->uri, new_uri);
 
@@ -657,12 +658,7 @@ AP_DECLARE(void) ap_internal_redirect(const char *new_uri, request_rec *r)
             access_status = ap_invoke_handler(new);
         }
     }
-    if (access_status == OK) {
-        ap_finalize_request_protocol(new);
-    }
-    else {
-        ap_die(access_status, new);
-    }
+    ap_die(access_status, new);
 }
 
 /* This function is designed for things like actions or CGI scripts, when
@@ -683,15 +679,9 @@ AP_DECLARE(void) ap_internal_redirect_handler(const char *new_uri, request_rec *
         ap_set_content_type(new, r->content_type);
     access_status = ap_process_request_internal(new);
     if (access_status == OK) {
-        if ((access_status = ap_invoke_handler(new)) != 0) {
-            ap_die(access_status, new);
-            return;
-        }
-        ap_finalize_request_protocol(new);
-    }
-    else {
-        ap_die(access_status, new);
+        access_status = ap_invoke_handler(new);
     }
+    ap_die(access_status, new);
 }
 
 AP_DECLARE(void) ap_allow_methods(request_rec *r, int reset, ...)