httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Roy T. Fielding" <field...@gbiv.com>
Subject Re: [Bug 47087] Incorrect request body handling with Expect: 100-continue if the client does not receive a transmitted 300 or 400 response prior to sending its body
Date Mon, 31 Aug 2009 03:20:24 GMT
On Aug 29, 2009, at 11:03 PM, Graham Dumpleton wrote:

> 2009/8/30 Nick Kew <nick@webthing.com>:
>>
>> On 27 Aug 2009, at 17:22, bugzilla@apache.org wrote:
>>
>>> It appears that Apache is violating this paragraph from RFC 2616:
>>>
>>>      - Upon receiving a request which includes an Expect request- 
>>> header
>>>        field with the "100-continue" expectation, an origin  
>>> server MUST
>>>        either respond with 100 (Continue) status and continue to  
>>> read
>>>        from the input stream, or respond with a final status  
>>> code. The
>>>        origin server MUST NOT wait for the request body before  
>>> sending
>>>        the 100 (Continue) response. If it responds with a final  
>>> status
>>>        code, it MAY close the transport connection or it MAY  
>>> continue
>>>        to read and discard the rest of the request.  It MUST NOT
>>>        perform the requested method if it returns a final status  
>>> code.
>>
>> Looks like we have a problem with the sequence:
>> Client asks for 100-continue
>> We reply with a final status - e.g. 3xx
>> [delay somewhere on the wire]
>> Client sends a request body
>> We read body as a new request - oops!
>>
>> It seems to me that keeping the connection open in this
>> instance means inevitable ambiguity over interpretation
>> of subsequent data, and the safe course of action is to
>> close it.  Otherwise we can read subsequent data line-
>> by-line and discard anything that isn't a valid request
>> line, at the risk of encountering a false positive in a
>> request body.
>>
>> +1 for closing the connection.  Any divergent opinions?
>
> FWIW, this area of code was change somewhere between 2.2.6 and 2.2.9
> already. Prior to the change if a handler sent a 20x response and then
> only after sending it did it attempt to read request input, the 100
> was being emitted as part of the response content.
>
> The old code in http_filters.c was:
>
>         /* Since we're about to read data, send 100-Continue if  
> needed.
>          * Only valid on chunked and C-L bodies where the C-L is >  
> 0. */
>         if ((ctx->state == BODY_CHUNK ||
>             (ctx->state == BODY_LENGTH && ctx->remaining > 0)) &&
>             f->r->expecting_100 && f->r->proto_num >= HTTP_VERSION

> (1,1)) {
>             char *tmp;
>             apr_bucket_brigade *bb;
>
>             tmp = apr_pstrcat(f->r->pool, AP_SERVER_PROTOCOL, " ",
>                               ap_get_status_line(100), CRLF CRLF,  
> NULL);
>             bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
>             e = apr_bucket_pool_create(tmp, strlen(tmp), f->r->pool,
>                                        f->c->bucket_alloc);
>             APR_BRIGADE_INSERT_HEAD(bb, e);
>             e = apr_bucket_flush_create(f->c->bucket_alloc);
>             APR_BRIGADE_INSERT_TAIL(bb, e);
>
>             ap_pass_brigade(f->c->output_filters, bb);
>         }
>
> and is now:
>
>         /* Since we're about to read data, send 100-Continue if  
> needed.
>          * Only valid on chunked and C-L bodies where the C-L is >  
> 0. */
>         if ((ctx->state == BODY_CHUNK ||
>             (ctx->state == BODY_LENGTH && ctx->remaining > 0)) &&
>             f->r->expecting_100 && f->r->proto_num >= HTTP_VERSION

> (1,1) &&
>             !(f->r->eos_sent || f->r->bytes_sent)) {
>             if (!ap_is_HTTP_SUCCESS(f->r->status)) {
>                 ctx->state = BODY_NONE;
>                 ctx->eos_sent = 1;
>             } else {
>                 char *tmp;
>
>                 tmp = apr_pstrcat(f->r->pool, AP_SERVER_PROTOCOL, " ",
>                                   ap_get_status_line(100), CRLF  
> CRLF, NULL);
>                 apr_brigade_cleanup(bb);
>                 e = apr_bucket_pool_create(tmp, strlen(tmp), f->r- 
> >pool,
>                                            f->c->bucket_alloc);
>                 APR_BRIGADE_INSERT_HEAD(bb, e);
>                 e = apr_bucket_flush_create(f->c->bucket_alloc);
>                 APR_BRIGADE_INSERT_TAIL(bb, e);
>
>                 ap_pass_brigade(f->c->output_filters, bb);
>             }
>         }
>
> The fix was needed for case where handler needs to start streaming a
> response before it starts processing request content.

Well, that explains why the code doesn't work any more.

> If there is no valid reason why for non 20x response that a handler
> should be able to read request content after having sent a response,
> then closing the connection seems logical thing to do to avoid Apache
> having to read the whole request content and discard it, just to
> handle a potential request over same connection after it.

The problem with that theory is that TCP reset behavior requires
us to read a substantial amount of the request anyway, just to
ensure that the client receives the final response.  However,
it is probably safer to tell the client to use a new connection
for the next request.

In any case, if we failed to read the request then we must mark
the connection as soiled.  What the above does is pretend that
there isn't a body and then leaves the connection in a
half-read state.

The solution should be something like

       if (!ap_is_HTTP_SUCCESS(f->r->status)) {
            ctx->state = BODY_NONE;
            ctx->eos_sent = 1;
            f->c->keepalive = AP_CONN_CLOSE;
       }

but I haven't tested that.

....Roy

Mime
View raw message