Here are the 3 patches attached as files (without spurious line breakage). Regards, Yann.
On Fri, May 17, 2013 at 7:27 PM, Yann Ylavic <[email protected]> wrote: > 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. >
httpd-trunk-mod_proxy-response_EOF.patch1
Description: Binary data
httpd-trunk-mod_proxy-http_filter_limit.patch2
Description: Binary data
httpd-trunk-mod_proxy-request_error_mapping.patch3
Description: Binary data
