You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by mi...@apache.org on 2013/05/15 17:46:01 UTC
svn commit: r1482918 - in /httpd/httpd/trunk: modules/http/http_filters.c
server/protocol.c
Author: minfrin
Date: Wed May 15 15:46:01 2013
New Revision: 1482918
URL: http://svn.apache.org/r1482918
Log:
core: Stop ap_finalize_request_protocol() and ap_get_client_block() from silently
swallowing errors from the filter stack, create error buckets and return them
appropriately.
Modified:
httpd/httpd/trunk/modules/http/http_filters.c
httpd/httpd/trunk/server/protocol.c
Modified: httpd/httpd/trunk/modules/http/http_filters.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1482918&r1=1482917&r2=1482918&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http/http_filters.c (original)
+++ httpd/httpd/trunk/modules/http/http_filters.c Wed May 15 15:46:01 2013
@@ -203,7 +203,6 @@ apr_status_t ap_http_filter(ap_filter_t
http_ctx_t *ctx = f->ctx;
apr_status_t rv;
apr_off_t totalread;
- int http_error = HTTP_REQUEST_ENTITY_TOO_LARGE;
apr_bucket_brigade *bb;
/* just get out of the way of things we don't want. */
@@ -360,7 +359,6 @@ apr_status_t ap_http_filter(ap_filter_t
ctx->remaining = get_chunk_size(ctx->chunk_ln);
if (ctx->remaining == INVALID_CHAR) {
rv = APR_EGENERAL;
- http_error = HTTP_BAD_REQUEST;
}
}
}
@@ -370,6 +368,9 @@ apr_status_t ap_http_filter(ap_filter_t
if (rv != APR_SUCCESS || ctx->remaining < 0) {
ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, f->r, APLOGNO(01589) "Error reading first chunk %s ",
(ctx->remaining < 0) ? "(overflow)" : "");
+ if (ctx->remaining < 0) {
+ rv = APR_ENOSPC;
+ }
ctx->remaining = 0; /* Reset it in case we have to
* come back here later */
return rv;
@@ -462,7 +463,6 @@ apr_status_t ap_http_filter(ap_filter_t
ctx->remaining = get_chunk_size(ctx->chunk_ln);
if (ctx->remaining == INVALID_CHAR) {
rv = APR_EGENERAL;
- http_error = HTTP_BAD_REQUEST;
}
}
}
@@ -473,6 +473,9 @@ apr_status_t ap_http_filter(ap_filter_t
if (rv != APR_SUCCESS || ctx->remaining < 0) {
ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, f->r, APLOGNO(01590) "Error reading chunk %s ",
(ctx->remaining < 0) ? "(overflow)" : "");
+ if (ctx->remaining < 0) {
+ rv = APR_ENOSPC;
+ }
ctx->remaining = 0; /* Reset it in case we have to
* come back here later */
return rv;
@@ -1410,6 +1413,9 @@ AP_DECLARE(int) ap_map_http_request_erro
case AP_FILTER_ERROR: {
return AP_FILTER_ERROR;
}
+ case APR_EGENERAL: {
+ return HTTP_BAD_REQUEST;
+ }
case APR_ENOSPC: {
return HTTP_REQUEST_ENTITY_TOO_LARGE;
}
@@ -1637,6 +1643,28 @@ AP_DECLARE(long) ap_get_client_block(req
* not be used.
*/
if (rv != APR_SUCCESS) {
+ apr_bucket *e;
+
+ /* work around our silent swallowing of error messages by mapping
+ * error codes at this point, and sending an error bucket back
+ * upstream.
+ */
+ apr_brigade_cleanup(bb);
+
+ e = ap_bucket_error_create(
+ ap_map_http_request_error(rv, HTTP_BAD_REQUEST), NULL, r->pool,
+ r->connection->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(bb, e);
+
+ e = apr_bucket_eos_create(r->connection->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(bb, e);
+
+ rv = ap_pass_brigade(r->output_filters, bb);
+ if (APR_SUCCESS != rv) {
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, r, APLOGNO()
+ "Error while writing error response");
+ }
+
/* if we actually fail here, we want to just return and
* stop trying to read data from the client.
*/
Modified: httpd/httpd/trunk/server/protocol.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1482918&r1=1482917&r2=1482918&view=diff
==============================================================================
--- httpd/httpd/trunk/server/protocol.c (original)
+++ httpd/httpd/trunk/server/protocol.c Wed May 15 15:46:01 2013
@@ -1282,6 +1282,21 @@ AP_DECLARE(void) ap_set_sub_req_protocol
rnew->main = (request_rec *) r;
}
+static void error_output_stream(request_rec *r, int status)
+{
+ conn_rec *c = r->connection;
+ apr_bucket_brigade *bb;
+ apr_bucket *b;
+
+ bb = apr_brigade_create(r->pool, c->bucket_alloc);
+ b = ap_bucket_error_create(status, NULL, r->pool,
+ r->connection->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(bb, b);
+ b = apr_bucket_eos_create(c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(bb, b);
+ ap_pass_brigade(r->output_filters, bb);
+}
+
static void end_output_stream(request_rec *r)
{
conn_rec *c = r->connection;
@@ -1309,9 +1324,12 @@ AP_DECLARE(void) ap_finalize_sub_req_pro
*/
AP_DECLARE(void) ap_finalize_request_protocol(request_rec *r)
{
- (void) ap_discard_request_body(r);
+ int status = ap_discard_request_body(r);
/* tell the filter chain there is no more content coming */
+ if (status) {
+ error_output_stream(r, status);
+ }
if (!r->eos_sent) {
end_output_stream(r);
}
Re: svn commit: r1482918 - in /httpd/httpd/trunk: modules/http/http_filters.c server/protocol.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 25 Jul 2013, at 10:16 PM, Rainer Jung <ra...@kippdata.de> wrote:
> I have a question about this part which turned up when debugging a
> mod_deflate test failure. Bear with me and my very incomplete
> understanding of bucket brigades:
>
> Is this error bucket insertion safe? In the mod_deflate test we added
> spurious data to the end of a compressed request body. When processing
> the request, mod_deflate inflated the request body and mod_echo_post
> returned it. If the body was big enough the echoed data appeared at the
> client. Now the spurious data was seen by mod_deflate and the above
> error bucket inserted. This error did not show up on the client side.
>
> Now the test case received a connection close header directly at the
> beginning of the response, but I wonder what would have happened if this
> had been a keep-alive connection. Would the error bucket have been
> queued and returned in front of the next response? Or would the
> connection have been aborted by the server in any case? I'm a little bit
> nervous here because of the potential for response mixup.
>
> If the posted body was small enough that the spurious data was found
> before the first part of the response as sent out, then the server
> correctly returns the 400 error page instead of the echo response.
(Coming back to this).
The error buckets are metadata buckets, and they are inserted before the EOS in the stream. Buckets are processed in order, for there to be a response splitting problem the error bucket will need to appear after the EOS.
In the case of mod_deflate’s input filter, it does not seem to take any notice of an error bucket (or any meta buckets), and so is likely to silently consume the error bucket (which is of zero length).
I suspect previously the core was silently swallowing errors, and now that this is fixed mod_deflate is now silently swallowing errors. It looks like mod_deflate needs to be fixed, but it doesn’t affect the underlying patch (from what I can see).
Regards,
Graham
—
Re: svn commit: r1482918 - in /httpd/httpd/trunk: modules/http/http_filters.c
server/protocol.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 15.05.2013 17:46, minfrin@apache.org wrote:
> Author: minfrin
> Date: Wed May 15 15:46:01 2013
> New Revision: 1482918
>
> URL: http://svn.apache.org/r1482918
> Log:
> core: Stop ap_finalize_request_protocol() and ap_get_client_block() from silently
> swallowing errors from the filter stack, create error buckets and return them
> appropriately.
>
> Modified:
> httpd/httpd/trunk/modules/http/http_filters.c
> httpd/httpd/trunk/server/protocol.c
>
> Modified: httpd/httpd/trunk/modules/http/http_filters.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1482918&r1=1482917&r2=1482918&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
> +++ httpd/httpd/trunk/modules/http/http_filters.c Wed May 15 15:46:01 2013
...
> @@ -1637,6 +1643,28 @@ AP_DECLARE(long) ap_get_client_block(req
> * not be used.
> */
> if (rv != APR_SUCCESS) {
> + apr_bucket *e;
> +
> + /* work around our silent swallowing of error messages by mapping
> + * error codes at this point, and sending an error bucket back
> + * upstream.
> + */
> + apr_brigade_cleanup(bb);
> +
> + e = ap_bucket_error_create(
> + ap_map_http_request_error(rv, HTTP_BAD_REQUEST), NULL, r->pool,
> + r->connection->bucket_alloc);
> + APR_BRIGADE_INSERT_TAIL(bb, e);
> +
> + e = apr_bucket_eos_create(r->connection->bucket_alloc);
> + APR_BRIGADE_INSERT_TAIL(bb, e);
> +
> + rv = ap_pass_brigade(r->output_filters, bb);
> + if (APR_SUCCESS != rv) {
> + ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, r, APLOGNO()
> + "Error while writing error response");
> + }
> +
> /* if we actually fail here, we want to just return and
> * stop trying to read data from the client.
> */
>
I have a question about this part which turned up when debugging a
mod_deflate test failure. Bear with me and my very incomplete
understanding of bucket brigades:
Is this error bucket insertion safe? In the mod_deflate test we added
spurious data to the end of a compressed request body. When processing
the request, mod_deflate inflated the request body and mod_echo_post
returned it. If the body was big enough the echoed data appeared at the
client. Now the spurious data was seen by mod_deflate and the above
error bucket inserted. This error did not show up on the client side.
Now the test case received a connection close header directly at the
beginning of the response, but I wonder what would have happened if this
had been a keep-alive connection. Would the error bucket have been
queued and returned in front of the next response? Or would the
connection have been aborted by the server in any case? I'm a little bit
nervous here because of the potential for response mixup.
If the posted body was small enough that the spurious data was found
before the first part of the response as sent out, then the server
correctly returns the 400 error page instead of the echo response.
Regards,
Rainer
Re: svn commit: r1482918 - in /httpd/httpd/trunk: modules/http/http_filters.c
server/protocol.c
Posted by Guenter Knauf <fu...@apache.org>.
Hi Graham,
seems you forgot to add a log number at line 1541:
On 15.05.2013 17:46, minfrin@apache.org wrote:
> Author: minfrin
> Date: Wed May 15 15:46:01 2013
> New Revision: 1482918
>
> URL: http://svn.apache.org/r1482918
> Log:
> core: Stop ap_finalize_request_protocol() and ap_get_client_block() from silently
> swallowing errors from the filter stack, create error buckets and return them
> appropriately.
>
> Modified:
> httpd/httpd/trunk/modules/http/http_filters.c
> httpd/httpd/trunk/server/protocol.c
>
> Modified: httpd/httpd/trunk/modules/http/http_filters.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1482918&r1=1482917&r2=1482918&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
> +++ httpd/httpd/trunk/modules/http/http_filters.c Wed May 15 15:46:01 2013
...
> + if (APR_SUCCESS != rv) {
> + ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, r, APLOGNO()
> + "Error while writing error response");
> + }
> +
> /* if we actually fail here, we want to just return and
> * stop trying to read data from the client.
> */
>
Gün.