On Aug 29, 2009, at 11:03 PM, Graham Dumpleton wrote:
2009/8/30 Nick Kew <[email protected]>:
On 27 Aug 2009, at 17:22, [email protected] 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
