Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Message-ID: <45C8AE34.4040301@apache.org>
Date: Tue, 06 Feb 2007 17:35:00 +0100
From: Ruediger Pluem <rpluem@apache.org>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.13) Gecko/20060417
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Re: svn commit: r503863 - /httpd/httpd/trunk/CHANGES
References: <20070205204602.0A0651A981A@eris.apache.org>
 <20070206131015.GA2881@redhat.com>
 <A38E9882-3CF5-41F5-989D-769E29794278@jaguNET.com>
In-Reply-To: <A38E9882-3CF5-41F5-989D-769E29794278@jaguNET.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


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