You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by co...@apache.org on 2010/04/25 21:10:45 UTC
svn commit: r937858 - in /httpd/httpd/trunk: CHANGES
modules/http/http_filters.c
Author: covener
Date: Sun Apr 25 19:10:45 2010
New Revision: 937858
URL: http://svn.apache.org/viewvc?rev=937858&view=rev
Log:
PR49167, unexpected 413 and double-errordoc during a timeout reading a
chunk-size.
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/http/http_filters.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=937858&r1=937857&r2=937858&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Sun Apr 25 19:10:45 2010
@@ -28,6 +28,11 @@ Changes with Apache 2.3.7
processing is completed, avoiding orphaned callback pointers.
[Brett Gervasoni <brettg senseofsecurity.com>, Jeff Trawick]
+ *) Log an error for failures to read a chunk-size, and return 400 instead
+ 413 when this is due to a read timeout. This change also fixes some cases
+ of two error documents being sent in the response for the same scenario.
+ [Eric Covener] PR49167
+
*) mod_proxy_balancer: Add new directive BalancerNonce to allow admin
to control/set the nonce used in the balancer-manager application.
[Jim Jagielski]
Modified: httpd/httpd/trunk/modules/http/http_filters.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=937858&r1=937857&r2=937858&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http/http_filters.c (original)
+++ httpd/httpd/trunk/modules/http/http_filters.c Sun Apr 25 19:10:45 2010
@@ -383,8 +383,13 @@ apr_status_t ap_http_filter(ap_filter_t
/* Detect chunksize error (such as overflow) */
if (rv != APR_SUCCESS || ctx->remaining < 0) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, "Error reading first chunk %s ",
+ (ctx->remaining < 0) ? "(overflow)" : "");
ctx->remaining = 0; /* Reset it in case we have to
* come back here later */
+ if (APR_STATUS_IS_TIMEUP(rv)) {
+ http_error = HTTP_BAD_REQUEST;
+ }
return bail_out_on_error(ctx, f, http_error);
}
@@ -484,10 +489,14 @@ apr_status_t ap_http_filter(ap_filter_t
/* Detect chunksize error (such as overflow) */
if (rv != APR_SUCCESS || ctx->remaining < 0) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, "Error reading chunk %s ",
+ (ctx->remaining < 0) ? "(overflow)" : "");
ctx->remaining = 0; /* Reset it in case we have to
* come back here later */
- bail_out_on_error(ctx, f, http_error);
- return rv;
+ if (APR_STATUS_IS_TIMEUP(rv)) {
+ http_error = HTTP_BAD_REQUEST;
+ }
+ return bail_out_on_error(ctx, f, http_error);
}
if (!ctx->remaining) {
Re: svn commit: r937858 - in /httpd/httpd/trunk: CHANGES modules/http/http_filters.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 26.04.2010 08:04, Roy T. Fielding wrote:
> I am confused. Why is this a good idea? Why is it unexpected to
> encounter a 413 response after a timeout due to a read of chunked
> body, and how does changing it to a 400 somehow prevent a double
> errordoc? Why not just fix the double errordoc bug instead of
> the case that triggers it?
Changing to 400 does not fix the double error message problem.
In fact this patch addresses two things in one and it might
have been worth doing two commits for it. I am partly to blame
for this as I encouraged Eric in a Bugzilla entry to commit this
patch.
The part that fixes the double error messages is:
>> - bail_out_on_error(ctx, f, http_error);
>> - return rv;
>> + return bail_out_on_error(ctx, f, http_error);
Regards
Rüdiger
>
> ....Roy
>
> On Apr 25, 2010, at 12:10 PM, covener@apache.org wrote:
>
>> Author: covener
>> Date: Sun Apr 25 19:10:45 2010
>> New Revision: 937858
>>
>> URL: http://svn.apache.org/viewvc?rev=937858&view=rev
>> Log:
>> PR49167, unexpected 413 and double-errordoc during a timeout reading a
>> chunk-size.
>>
>>
>> Modified:
>> httpd/httpd/trunk/CHANGES
>> httpd/httpd/trunk/modules/http/http_filters.c
>>
>> Modified: httpd/httpd/trunk/CHANGES
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=937858&r1=937857&r2=937858&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>> +++ httpd/httpd/trunk/CHANGES [utf-8] Sun Apr 25 19:10:45 2010
>> @@ -28,6 +28,11 @@ Changes with Apache 2.3.7
>> processing is completed, avoiding orphaned callback pointers.
>> [Brett Gervasoni <brettg senseofsecurity.com>, Jeff Trawick]
>>
>> + *) Log an error for failures to read a chunk-size, and return 400 instead
>> + 413 when this is due to a read timeout. This change also fixes some cases
>> + of two error documents being sent in the response for the same scenario.
>> + [Eric Covener] PR49167
>> +
>> *) mod_proxy_balancer: Add new directive BalancerNonce to allow admin
>> to control/set the nonce used in the balancer-manager application.
>> [Jim Jagielski]
>>
>> Modified: httpd/httpd/trunk/modules/http/http_filters.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=937858&r1=937857&r2=937858&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
>> +++ httpd/httpd/trunk/modules/http/http_filters.c Sun Apr 25 19:10:45 2010
>> @@ -383,8 +383,13 @@ apr_status_t ap_http_filter(ap_filter_t
>>
>> /* Detect chunksize error (such as overflow) */
>> if (rv != APR_SUCCESS || ctx->remaining < 0) {
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, "Error reading first chunk %s ",
>> + (ctx->remaining < 0) ? "(overflow)" : "");
>> ctx->remaining = 0; /* Reset it in case we have to
>> * come back here later */
>> + if (APR_STATUS_IS_TIMEUP(rv)) {
>> + http_error = HTTP_BAD_REQUEST;
>> + }
>> return bail_out_on_error(ctx, f, http_error);
>> }
>>
>> @@ -484,10 +489,14 @@ apr_status_t ap_http_filter(ap_filter_t
>>
>> /* Detect chunksize error (such as overflow) */
>> if (rv != APR_SUCCESS || ctx->remaining < 0) {
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, "Error reading chunk %s ",
>> + (ctx->remaining < 0) ? "(overflow)" : "");
>> ctx->remaining = 0; /* Reset it in case we have to
>> * come back here later */
>> - bail_out_on_error(ctx, f, http_error);
>> - return rv;
>> + if (APR_STATUS_IS_TIMEUP(rv)) {
>> + http_error = HTTP_BAD_REQUEST;
>> + }
>> + return bail_out_on_error(ctx, f, http_error);
>> }
>>
>> if (!ctx->remaining) {
>>
>>
>
>
Re: svn commit: r937858 - in /httpd/httpd/trunk: CHANGES
modules/http/http_filters.c
Posted by Eric Covener <co...@gmail.com>.
On Mon, Apr 26, 2010 at 6:15 PM, Roy T. Fielding <fi...@gbiv.com> wrote:
> On Apr 26, 2010, at 2:55 AM, Eric Covener wrote:
>
>> On Mon, Apr 26, 2010 at 2:04 AM, Roy T. Fielding <fi...@gbiv.com> wrote:
>>> I am confused. Why is this a good idea? Why is it unexpected to
>>> encounter a 413 response after a timeout due to a read of chunked
>>> body,
>>
>> The body is not too large, and the server is not rejecting the body
>> due to size. Isn't 413 a very misleading status?
>
> Er, yeah, 413 is wrong -- I should have looked it up. It should be
> a 408 (Request Timeout).
400->408 in r938265.
--
Eric Covener
covener@gmail.com
Re: svn commit: r937858 - in /httpd/httpd/trunk: CHANGES modules/http/http_filters.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
On Apr 26, 2010, at 2:55 AM, Eric Covener wrote:
> On Mon, Apr 26, 2010 at 2:04 AM, Roy T. Fielding <fi...@gbiv.com> wrote:
>> I am confused. Why is this a good idea? Why is it unexpected to
>> encounter a 413 response after a timeout due to a read of chunked
>> body,
>
> The body is not too large, and the server is not rejecting the body
> due to size. Isn't 413 a very misleading status?
Er, yeah, 413 is wrong -- I should have looked it up. It should be
a 408 (Request Timeout).
>> and how does changing it to a 400 somehow prevent a double
>> errordoc? Why not just fix the double errordoc bug instead of
>> the case that triggers it?
>
> This was the secondary part to me, but I'll open a bug with an example
> (IIUC, filter that adds an error bucket but then also returns a
> (non-http) error).
Well, if it is fixed as Ruediger explained, then I am fine with it.
I just couldn't see the fix in that patch.
....Roy
Re: svn commit: r937858 - in /httpd/httpd/trunk: CHANGES
modules/http/http_filters.c
Posted by Eric Covener <co...@gmail.com>.
On Mon, Apr 26, 2010 at 2:04 AM, Roy T. Fielding <fi...@gbiv.com> wrote:
> I am confused. Why is this a good idea? Why is it unexpected to
> encounter a 413 response after a timeout due to a read of chunked
> body,
The body is not too large, and the server is not rejecting the body
due to size. Isn't 413 a very misleading status?
> and how does changing it to a 400 somehow prevent a double
> errordoc? Why not just fix the double errordoc bug instead of
> the case that triggers it?
This was the secondary part to me, but I'll open a bug with an example
(IIUC, filter that adds an error bucket but then also returns a
(non-http) error).
--
Eric Covener
covener@gmail.com
Re: svn commit: r937858 - in /httpd/httpd/trunk: CHANGES modules/http/http_filters.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
I am confused. Why is this a good idea? Why is it unexpected to
encounter a 413 response after a timeout due to a read of chunked
body, and how does changing it to a 400 somehow prevent a double
errordoc? Why not just fix the double errordoc bug instead of
the case that triggers it?
....Roy
On Apr 25, 2010, at 12:10 PM, covener@apache.org wrote:
> Author: covener
> Date: Sun Apr 25 19:10:45 2010
> New Revision: 937858
>
> URL: http://svn.apache.org/viewvc?rev=937858&view=rev
> Log:
> PR49167, unexpected 413 and double-errordoc during a timeout reading a
> chunk-size.
>
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/http/http_filters.c
>
> Modified: httpd/httpd/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=937858&r1=937857&r2=937858&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Sun Apr 25 19:10:45 2010
> @@ -28,6 +28,11 @@ Changes with Apache 2.3.7
> processing is completed, avoiding orphaned callback pointers.
> [Brett Gervasoni <brettg senseofsecurity.com>, Jeff Trawick]
>
> + *) Log an error for failures to read a chunk-size, and return 400 instead
> + 413 when this is due to a read timeout. This change also fixes some cases
> + of two error documents being sent in the response for the same scenario.
> + [Eric Covener] PR49167
> +
> *) mod_proxy_balancer: Add new directive BalancerNonce to allow admin
> to control/set the nonce used in the balancer-manager application.
> [Jim Jagielski]
>
> Modified: httpd/httpd/trunk/modules/http/http_filters.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=937858&r1=937857&r2=937858&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
> +++ httpd/httpd/trunk/modules/http/http_filters.c Sun Apr 25 19:10:45 2010
> @@ -383,8 +383,13 @@ apr_status_t ap_http_filter(ap_filter_t
>
> /* Detect chunksize error (such as overflow) */
> if (rv != APR_SUCCESS || ctx->remaining < 0) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, "Error reading first chunk %s ",
> + (ctx->remaining < 0) ? "(overflow)" : "");
> ctx->remaining = 0; /* Reset it in case we have to
> * come back here later */
> + if (APR_STATUS_IS_TIMEUP(rv)) {
> + http_error = HTTP_BAD_REQUEST;
> + }
> return bail_out_on_error(ctx, f, http_error);
> }
>
> @@ -484,10 +489,14 @@ apr_status_t ap_http_filter(ap_filter_t
>
> /* Detect chunksize error (such as overflow) */
> if (rv != APR_SUCCESS || ctx->remaining < 0) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, "Error reading chunk %s ",
> + (ctx->remaining < 0) ? "(overflow)" : "");
> ctx->remaining = 0; /* Reset it in case we have to
> * come back here later */
> - bail_out_on_error(ctx, f, http_error);
> - return rv;
> + if (APR_STATUS_IS_TIMEUP(rv)) {
> + http_error = HTTP_BAD_REQUEST;
> + }
> + return bail_out_on_error(ctx, f, http_error);
> }
>
> if (!ctx->remaining) {
>
>