httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Shouldn't ap_die() do nothing -inconditionally- when a response was already generated?
Date Wed, 11 Mar 2015 09:32:15 GMT
That is, not only when a hook/handler returns AP_FILTER_ERROR.

Graham (initially) and I made some changes to trunk (backport proposed
in r1665737) so that no internal module should return anything but
AP_FILTER_ERROR in this case, but we can't do much for third-party
modules.

That way ap_die() could become a safe gard against unintentional
double responses (hence HRS) for the same request, in any case since
it is always called at the end.

Something like:

Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c    (revision 1665647)
+++ modules/http/http_request.c    (working copy)
@@ -75,6 +75,7 @@ static void update_r_in_filters(ap_filter_t *f,

 static void ap_die_r(int type, request_rec *r, int recursive_error)
 {
+    ap_filter_t *next;
     char *custom_response;
     request_rec *r_1st_err = r;

@@ -83,38 +84,34 @@ static void ap_die_r(int type, request_rec *r, int
         return;
     }

+    /*
+     * Check if we still have the ap_http_header_filter in place. If
+     * this is the case we should not ignore the error here because
+     * it means that we have not sent any response at all. Otherwise,
+     * don't send two responses for the same request.
+     */
+    next = r->output_filters;
+    while (next && (next->frec != ap_http_header_filter_handle)) {
+        next = next->next;
+    }
+    if (!next) {
+        return;
+    }
+
+    /*
+     * AP_FILTER_ERROR or any invalid status, send an internal server
+     * error instead.
+     */
     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 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.
-         */
-        next = r->output_filters;
-        while (next && (next->frec != ap_http_header_filter_handle)) {
-               next = next->next;
-        }
-
-        /*
-         * If next != NULL then we left the while above because of
-         * next->frec == ap_http_header_filter
-         */
-        if (next) {
-            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 {
-            return;
-        }
+        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;
     }

     /*
--

This is not a great cycles loss since this path is not a fast one
already (!OK && !DONE => ap_send_error_response() or
ap_internal_redirect()), and the inconditional loop is not really
heavy...

Thoughts?

Regards,
Yann.

Mime
View raw message