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.
