You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by rp...@apache.org on 2007/02/05 21:46:01 UTC
svn commit: r503863 - /httpd/httpd/trunk/CHANGES
Author: rpluem
Date: Mon Feb 5 12:46:01 2007
New Revision: 503863
URL: http://svn.apache.org/viewvc?view=rev&rev=503863
Log:
* Add missing Changelog entry for PR41056 / PR 19954. This was fixed in r480135.
PR: 41056 / 19954
Submitted by: jfclere, jim
Reviewed by: jim
Modified:
httpd/httpd/trunk/CHANGES
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?view=diff&rev=503863&r1=503862&r2=503863
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Mon Feb 5 12:46:01 2007
@@ -2,6 +2,9 @@
Changes with Apache 2.3.0
[Remove entries to the current 2.0 and 2.2 section below, when backported]
+ *) core: Fix broken chunk filtering that causes all non blocking reads to be
+ converted into blocking reads. PR 41056. [Jean-Frederic Clere, Jim Jagielski]
+
*) apxs: Enhance -q flag to print all known variables and their values
when invoked without variable name(s).
[William Rowe, Sander Temme]
Re: svn commit: r503863 - /httpd/httpd/trunk/CHANGES
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Feb 06, 2007 at 05:35:00PM +0100, Ruediger Pluem wrote:
> On 02/06/2007 04:49 PM, Jim Jagielski wrote:
> > Joe, can you see if the below fixes it:
> >
> > Index: http_filters.c
> > ===================================================================
> > --- http_filters.c (revision 504180)
> > +++ http_filters.c (working copy)
> > @@ -299,7 +299,8 @@
> > rv = APR_SUCCESS;
> > }
> > - if (rv == APR_SUCCESS) {
> > + if (rv == APR_SUCCESS ||
> > + (ctx->state == BODY_CHUNK &&
> > (APR_STATUS_IS_EAGAIN(rv))) ) {
> > /* Read the real chunk line. */
> > rv = ap_get_brigade(f->next, bb, AP_MODE_GETLINE,
> > block, 0);
> >
>
> Wouldn't that do the wrong thing?
Yes.
...
> Joe could you please refresh my mind what was wrong in returning an
> APR_EAGAIN? We actually do this explicity in line 311. If this is
> wrong I guess this needs to be fixed there as well.
Returning an EAGAIN directly after line 295 would be fine but that's not
what the code does: it falls through to the line 331 conditional and
sends an error response.
joe
Re: svn commit: r503863 - /httpd/httpd/trunk/CHANGES
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Feb 6, 2007, at 3:07 PM, Ruediger Pluem wrote:
>
>
> On 02/06/2007 06:26 PM, Jim Jagielski wrote:
>> I don't know what's happening with my emails, but they
>> appear to be getting dropped left and right.
>>
>> I had responded to Joe's email, saying that I must be
>> misunderstanding his concern, but I haven't seen that
>> make it's way through yet.
>>
>> If I'm understanding it correctly, what we should be
>> doing is honoring the EAGAIN (if any) at line 295 and
>> returning that (w/o the state change). But it seems
>> that Joe doesn't think that'll work... I'm confused.
>> The previous patch wasn't correct, that's for sure.
>>
>
>> From my understanding the following needs to be done:
>
> Index: http_filters.c
> ===================================================================
> --- http_filters.c (Revision 504183)
> +++ http_filters.c (Arbeitskopie)
> @@ -294,6 +294,12 @@
> if (ctx->state == BODY_CHUNK) {
> rv = ap_get_brigade(f->next, bb, AP_MODE_GETLINE,
> block, 0);
> + if (block == APR_NONBLOCK_READ &&
> + (((rv == APR_SUCCESS) && (APR_BRIGADE_EMPTY
> (bb))) ||
> + (APR_STATUS_IS_EAGAIN(rv)))) {
> + apr_brigade_cleanup(bb);
> + return APR_EAGAIN;
> + }
> apr_brigade_cleanup(bb);
> } else {
> rv = APR_SUCCESS;
>
>
> This is slightly different than your patch Jim, as it also checks
> for an empty brigade with a APR_SUCESSS return code as we do
> a few lines later. Put possibly this could be also wrong here if the
> *successfull* read of an empty line in AP_MODE_GET_LINE returns an
> empty brigade. In this case the patch you sent to Joe and me
> should be correct:
>
That was my concern... And since we always do the cleanup,
duplicating that call doesn't seem to make sense... ;)
(here's hoping this email makes it through :) )
> Index: modules/http/http_filters.c
> ===================================================================
> --- modules/http/http_filters.c (revision 504222)
> +++ modules/http/http_filters.c (working copy)
> @@ -295,6 +295,10 @@
> rv = ap_get_brigade(f->next, bb, AP_MODE_GETLINE,
> block, 0);
> apr_brigade_cleanup(bb);
> + if (block == APR_NONBLOCK_READ &&
> + (APR_STATUS_IS_EAGAIN(rv))) {
> + return APR_EAGAIN;
> + }
> } else {
> rv = APR_SUCCESS;
> }
>
>
>
>
> Regards
>
> Rüdiger
>
Re: svn commit: r503863 - /httpd/httpd/trunk/CHANGES
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/06/2007 06:26 PM, Jim Jagielski wrote:
> I don't know what's happening with my emails, but they
> appear to be getting dropped left and right.
>
> I had responded to Joe's email, saying that I must be
> misunderstanding his concern, but I haven't seen that
> make it's way through yet.
>
> If I'm understanding it correctly, what we should be
> doing is honoring the EAGAIN (if any) at line 295 and
> returning that (w/o the state change). But it seems
> that Joe doesn't think that'll work... I'm confused.
> The previous patch wasn't correct, that's for sure.
>
>From my understanding the following needs to be done:
Index: http_filters.c
===================================================================
--- http_filters.c (Revision 504183)
+++ http_filters.c (Arbeitskopie)
@@ -294,6 +294,12 @@
if (ctx->state == BODY_CHUNK) {
rv = ap_get_brigade(f->next, bb, AP_MODE_GETLINE,
block, 0);
+ if (block == APR_NONBLOCK_READ &&
+ (((rv == APR_SUCCESS) && (APR_BRIGADE_EMPTY(bb))) ||
+ (APR_STATUS_IS_EAGAIN(rv)))) {
+ apr_brigade_cleanup(bb);
+ return APR_EAGAIN;
+ }
apr_brigade_cleanup(bb);
} else {
rv = APR_SUCCESS;
This is slightly different than your patch Jim, as it also checks
for an empty brigade with a APR_SUCESSS return code as we do
a few lines later. Put possibly this could be also wrong here if the
*successfull* read of an empty line in AP_MODE_GET_LINE returns an
empty brigade. In this case the patch you sent to Joe and me should be correct:
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c (revision 504222)
+++ modules/http/http_filters.c (working copy)
@@ -295,6 +295,10 @@
rv = ap_get_brigade(f->next, bb, AP_MODE_GETLINE,
block, 0);
apr_brigade_cleanup(bb);
+ if (block == APR_NONBLOCK_READ &&
+ (APR_STATUS_IS_EAGAIN(rv))) {
+ return APR_EAGAIN;
+ }
} else {
rv = APR_SUCCESS;
}
Regards
Rüdiger
Re: svn commit: r503863 - /httpd/httpd/trunk/CHANGES
Posted by Jim Jagielski <ji...@jaguNET.com>.
I don't know what's happening with my emails, but they
appear to be getting dropped left and right.
I had responded to Joe's email, saying that I must be
misunderstanding his concern, but I haven't seen that
make it's way through yet.
If I'm understanding it correctly, what we should be
doing is honoring the EAGAIN (if any) at line 295 and
returning that (w/o the state change). But it seems
that Joe doesn't think that'll work... I'm confused.
The previous patch wasn't correct, that's for sure.
Re: svn commit: r503863 - /httpd/httpd/trunk/CHANGES
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/06/2007 04:49 PM, Jim Jagielski wrote:
>
> On Feb 6, 2007, at 8:10 AM, Joe Orton wrote:
>
>> On Mon, Feb 05, 2007 at 08:46:01PM -0000, rpluem@apache.org wrote:
>>
>>> Author: rpluem
>>> Date: Mon Feb 5 12:46:01 2007
>>> New Revision: 503863
>>>
>>> URL: http://svn.apache.org/viewvc?view=rev&rev=503863
>>> Log:
>>> * Add missing Changelog entry for PR41056 / PR 19954. This was fixed
>>> in r480135.
>>
>>
>> It looks like the regression is still present in this code, an EAGAIN
>> from the ap_get_brigade() to read the post-chunk CRLF is not handled and
>> will be treated as an error, line 295 of http_filters.c
>>
>> (AFAIK the test suite does not exercise the code paths for a NONBLOCKing
>> read of a chunked request body, testing this reliably requires a client
>> which drip-feeds bytes to trigger the EAGAIN on the server side on every
>> possible read call)
>>
>
> Joe, can you see if the below fixes it:
>
> Index: http_filters.c
> ===================================================================
> --- http_filters.c (revision 504180)
> +++ http_filters.c (working copy)
> @@ -299,7 +299,8 @@
> rv = APR_SUCCESS;
> }
> - if (rv == APR_SUCCESS) {
> + if (rv == APR_SUCCESS ||
> + (ctx->state == BODY_CHUNK &&
> (APR_STATUS_IS_EAGAIN(rv))) ) {
> /* Read the real chunk line. */
> rv = ap_get_brigade(f->next, bb, AP_MODE_GETLINE,
> block, 0);
>
Wouldn't that do the wrong thing? Suppose we miss the the CRLF in line 295 because
it is not available on the wire, but available later during our call to
ap_get_brigade in line 304. In this case we would read an empty line where we expect
to read the chunk size and we would bail out in line 319 with an error.
Joe could you please refresh my mind what was wrong in returning an APR_EAGAIN?
We actually do this explicity in line 311. If this is wrong I guess this needs to be
fixed there as well.
Regards
Rüdiger
Re: svn commit: r503863 - /httpd/httpd/trunk/CHANGES
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Feb 6, 2007, at 8:10 AM, Joe Orton wrote:
> On Mon, Feb 05, 2007 at 08:46:01PM -0000, rpluem@apache.org wrote:
>> Author: rpluem
>> Date: Mon Feb 5 12:46:01 2007
>> New Revision: 503863
>>
>> URL: http://svn.apache.org/viewvc?view=rev&rev=503863
>> Log:
>> * Add missing Changelog entry for PR41056 / PR 19954. This was
>> fixed in r480135.
>
> It looks like the regression is still present in this code, an EAGAIN
> from the ap_get_brigade() to read the post-chunk CRLF is not
> handled and
> will be treated as an error, line 295 of http_filters.c
>
> (AFAIK the test suite does not exercise the code paths for a
> NONBLOCKing
> read of a chunked request body, testing this reliably requires a
> client
> which drip-feeds bytes to trigger the EAGAIN on the server side on
> every
> possible read call)
>
Joe, can you see if the below fixes it:
Index: http_filters.c
===================================================================
--- http_filters.c (revision 504180)
+++ http_filters.c (working copy)
@@ -299,7 +299,8 @@
rv = APR_SUCCESS;
}
- if (rv == APR_SUCCESS) {
+ if (rv == APR_SUCCESS ||
+ (ctx->state == BODY_CHUNK &&
(APR_STATUS_IS_EAGAIN(rv))) ) {
/* Read the real chunk line. */
rv = ap_get_brigade(f->next, bb, AP_MODE_GETLINE,
block, 0);
Re: svn commit: r503863 - /httpd/httpd/trunk/CHANGES
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Feb 05, 2007 at 08:46:01PM -0000, rpluem@apache.org wrote:
> Author: rpluem
> Date: Mon Feb 5 12:46:01 2007
> New Revision: 503863
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=503863
> Log:
> * Add missing Changelog entry for PR41056 / PR 19954. This was fixed in r480135.
It looks like the regression is still present in this code, an EAGAIN
from the ap_get_brigade() to read the post-chunk CRLF is not handled and
will be treated as an error, line 295 of http_filters.c
(AFAIK the test suite does not exercise the code paths for a NONBLOCKing
read of a chunked request body, testing this reliably requires a client
which drip-feeds bytes to trigger the EAGAIN on the server side on every
possible read call)
joe