httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: svn commit: r1482522 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ include/ modules/dav/main/ modules/generators/ modules/http/ modules/proxy/ server/
Date Fri, 17 May 2013 17:27:07 GMT
I am currently working in frontporting some patches I use for a while
in the 2.2.x branch and I'd like to propose for trunk (at least, my
goal is to upgrade to 2.4 of course).

The reason I talk about this in this thread is that 2 of these patches
may collide with your current work/review on ap_http_filter() and
maybe it worth taking my suggestions into account. If I should open a
different thread let me know...

My 2 patches serve the following purposes :
1. make mod_proxy_http handle a truncated response body (closed
connection with remaining data) like any other streaming error,
2. make LimitRequestBody also apply to mod_proxy_http forwarded requests,
3. make mod_proxy_http respond the HTTP error mapping the
ap_http_filter() status.

1. When ap_http_filter() returns APR_EOF, currently mod_proxy simply
stops forwarding the response, no log, no error bucket, like a normal
end of sream.
For an output filter there is no way to be aware of the problem, and
mod_cache (for example) will cache an incomplete entity.

The following patch addresses this issue :
<patch1>
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c    (revision 1483799)
+++ modules/http/http_filters.c    (working copy)
@@ -520,8 +524,12 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
         ctx->remaining -= totalread;
         if (ctx->remaining > 0) {
             e = APR_BRIGADE_LAST(b);
-            if (APR_BUCKET_IS_EOS(e))
-                return APR_EOF;
+            if (APR_BUCKET_IS_EOS(e)) {
+                apr_bucket_delete(e);
+                if (APR_BRIGADE_EMPTY(b)) {
+                    return APR_EOF;
+                }
+            }
         }
     }

Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c    (revision 1483799)
+++ modules/proxy/mod_proxy_http.c    (working copy)
@@ -1679,9 +1679,6 @@ int ap_proxy_http_process_response(apr_pool_t * p,
                         mode = APR_BLOCK_READ;
                         continue;
                     }
-                    else if (rv == APR_EOF) {
-                        break;
-                    }
                     else if (rv != APR_SUCCESS) {
                         if (rv == APR_ENOSPC) {
                             ap_log_rerror(APLOG_MARK, APLOG_ERR, rv,
r, APLOGNO(02475)
@@ -1691,6 +1688,10 @@ int ap_proxy_http_process_response(apr_pool_t * p,
                             ap_log_rerror(APLOG_MARK, APLOG_ERR, rv,
r, APLOGNO(02476)
                                           "Response Transfer-Encoding
was not recognised");
                         }
+                        else if (rv == APR_EOF) {
+                            ap_log_rerror(APLOG_MARK, APLOG_ERR, rv,
r, APLOGNO()
+                                          "Response content is incomplete");
+                        }
                         else {
                             ap_log_rerror(APLOG_MARK, APLOG_ERR, rv,
r, APLOGNO(01110)
                                           "Network error reading response");
</patch1>

In the 2.2 version of the patch I used APR_INCOMPLETE instead of
APR_EOF and change APR_EOF to APR_INCOMPLETE in ap_http_filter()
whenever the content is incomplete. I didn't want to change the
meaning of APR_EOF in mod_proxy, but now I think mod_proxy has to be
fixed and other modules should not be impacted.

Note the patch will not trigger APR_EOF on the first call if some data
is available with the EOS, IMHO anything received should be forwarded
to the client/output filters before closing the stream.


2. LimitRequestBody handling is disabled when mod_proxy is on stage,
not only for PROXYREQ_RESPONSE (although the associated comment says
"does not apply to proxied responses").

Maybe there should be a ProxyLimitRequestBody (and
ProxyLimitResponseBody?) for the mod_proxy case, not to break some
existing configurations (for example with a weird global
LimitRequestBody that currently only apply to non-proxy vhosts), but
that would duplicate the ap_http_filter() code somewhere else...

<patch3>
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c    (revision 1483799)
+++ modules/http/http_filters.c    (working copy)
@@ -223,7 +223,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
          * Would adding a directive to limit the size of proxied
          * responses be useful?
          */
-        if (!f->r->proxyreq) {
+        if (f->r->proxyreq != PROXYREQ_RESPONSE) {
             ctx->limit = ap_get_limit_req_body(f->r);
         }
         else {
@@ -387,6 +387,10 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
             }
         }
     }
+    else if (ctx->limit && ctx->limit < ctx->limit_used) {
+        /* The limit is already reached, don't read any further. */
+        return APR_ENOSPC;
+    }
     else {
         bb = ctx->bb;
     }
@@ -544,15 +548,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
                           "Read content-length of %" APR_OFF_T_FMT
                           " is larger than the configured limit"
                           " of %" APR_OFF_T_FMT, ctx->limit_used, ctx->limit);
-            apr_brigade_cleanup(bb);
-            e = ap_bucket_error_create(HTTP_REQUEST_ENTITY_TOO_LARGE, NULL,
-                                       f->r->pool,
-                                       f->c->bucket_alloc);
-            APR_BRIGADE_INSERT_TAIL(bb, e);
-            e = apr_bucket_eos_create(f->c->bucket_alloc);
-            APR_BRIGADE_INSERT_TAIL(bb, e);
-            ctx->eos_sent = 1;
-            return ap_pass_brigade(f->r->output_filters, bb);
+            return APR_ENOSPC;
         }
     }

</patch3>


3. For the ap_http_filter() status mapping to HTTP error you have done
all the work, and the patch is now as simple as :
<patch3>
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c    (revision 1483799)
+++ modules/http/http_filters.c    (working copy)
@@ -1399,7 +1399,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
  * Map specific APR codes returned by the filter stack to HTTP error
  * codes, or the default status code provided. Use it as follows:
  *
- * return ap_map_http_response(rv, HTTP_BAD_REQUEST);
+ * return ap_map_http_request_error(rv, HTTP_BAD_REQUEST);
  *
  * If the filter has already handled the error, AP_FILTER_ERROR will
  * be returned, which is cleanly passed through.
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c    (revision 1483799)
+++ modules/proxy/mod_proxy_http.c    (working copy)
@@ -319,7 +319,7 @@ static int stream_reqbody_chunked(apr_pool_t *p,
                                 HUGE_STRING_LEN);

         if (status != APR_SUCCESS) {
-            return HTTP_BAD_REQUEST;
+            return ap_map_http_request_error(status, HTTP_BAD_REQUEST);
         }
     }

@@ -464,7 +464,7 @@ static int stream_reqbody_cl(apr_pool_t *p,
                                 HUGE_STRING_LEN);

         if (status != APR_SUCCESS) {
-            return HTTP_BAD_REQUEST;
+            return ap_map_http_request_error(status, HTTP_BAD_REQUEST);
         }
     }

@@ -607,7 +607,7 @@ static int spool_reqbody_cl(apr_pool_t *p,
                                 HUGE_STRING_LEN);

         if (status != APR_SUCCESS) {
-            return HTTP_BAD_REQUEST;
+            return ap_map_http_request_error(status, HTTP_BAD_REQUEST);
         }
     }

@@ -791,7 +791,7 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
                           " from %s (%s)",
                           p_conn->addr, p_conn->hostname ?
p_conn->hostname: "",
                           c->client_ip, c->remote_host ? c->remote_host: "");
-            return HTTP_BAD_REQUEST;
+            return ap_map_http_request_error(status, HTTP_BAD_REQUEST);
         }

         apr_brigade_length(temp_brigade, 1, &bytes);
</patch3>


Regards,
Yann.

Mime
View raw message