You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Justin Erenkrantz <je...@apache.org> on 2002/06/03 01:40:41 UTC
[PATCH] ap_discard_request_body() can't be called more than once
This patch combined with the last few patches I've posted today allow
chunked trailer support again and now passes all httpd-test cases.
What we try to do is to ensure that ap_discard_request_body() is not
called before the handler "accepts" the request and begins generating
the output. It is still possible for a bad module to call discard
more than once or improperly.
This effectively reverts Ryan's patch to http_protocol.c, so
I'd appreciate it gets some review (preferably from Ryan
himself!). -- justin
Index: modules/dav/main/mod_dav.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/dav/main/mod_dav.c,v
retrieving revision 1.79
diff -u -r1.79 mod_dav.c
--- modules/dav/main/mod_dav.c 17 May 2002 11:33:08 -0000 1.79
+++ modules/dav/main/mod_dav.c 2 Jun 2002 22:50:08 -0000
@@ -1084,11 +1097,6 @@
int result;
int depth;
- /* We don't use the request body right now, so torch it. */
- if ((result = ap_discard_request_body(r)) != OK) {
- return result;
- }
-
/* Ask repository module to resolve the resource */
err = dav_get_resource(r, 0 /* label_allowed */, 0 /* use_checked_in */,
&resource);
@@ -2680,11 +2646,6 @@
"and Overwrite has been specified.");
}
- /* ### for now, we don't need anything in the body */
- if ((result = ap_discard_request_body(r)) != OK) {
- return result;
- }
-
if ((err = dav_open_lockdb(r, 0, &lockdb)) != NULL) {
/* ### add a higher-level description? */
return dav_handle_err(r, err, NULL);
@@ -3461,10 +3422,6 @@
if (vsn_hooks == NULL)
return DECLINED;
- if ((result = ap_discard_request_body(r)) != OK) {
- return result;
- }
-
/* Ask repository module to resolve the resource */
err = dav_get_resource(r, 0 /* label_allowed */, 0 /* use_checked_in */,
&resource);
@@ -3506,6 +3463,11 @@
return dav_handle_err(r, err, NULL);
}
+ /* Discard the body now. */
+ if ((result = ap_discard_request_body(r)) != OK) {
+ return result;
+ }
+
/* no body */
ap_set_content_length(r, 0);
@@ -4056,11 +4018,6 @@
&resource);
if (err != NULL)
return dav_handle_err(r, err, NULL);
-
- /* MKACTIVITY does not have a defined request body. */
- if ((result = ap_discard_request_body(r)) != OK) {
- return result;
- }
/* Check request preconditions */
Index: modules/http/http_protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.432
diff -u -r1.432 http_protocol.c
--- modules/http/http_protocol.c 31 May 2002 20:41:06 -0000 1.432
+++ modules/http/http_protocol.c 2 Jun 2002 22:50:14 -0000
@@ -903,11 +903,6 @@
if (!ctx->remaining) {
switch (ctx->state) {
case BODY_NONE:
- if (f->r->proxyreq != PROXYREQ_RESPONSE) {
- e = apr_bucket_eos_create(f->c->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(b, e);
- return APR_SUCCESS;
- }
break;
case BODY_LENGTH:
e = apr_bucket_eos_create(f->c->bucket_alloc);
@@ -2285,14 +2280,8 @@
const char *title = status_lines[idx];
const char *h1;
- /* XXX This is a major hack that should be fixed cleanly. The
- * problem is that we have the information we need in a previous
- * request, but the text of the page must be sent down the last
- * request_rec's filter stack. rbb
- */
- request_rec *rlast = r;
- while (rlast->next) {
- rlast = rlast->next;
+ if (ap_discard_request_body(r) == AP_FILTER_ERROR) {
+ return;
}
/* Accept a status_line set by a module, but only if it begins
@@ -2315,24 +2304,24 @@
* so do ebcdic->ascii translation explicitly (if needed)
*/
- ap_rvputs_proto_in_ascii(rlast,
+ ap_rvputs_proto_in_ascii(r,
DOCTYPE_HTML_2_0
"<html><head>\n<title>", title,
"</title>\n</head><body>\n<h1>", h1, "</h1>\n",
NULL);
- ap_rvputs_proto_in_ascii(rlast,
+ ap_rvputs_proto_in_ascii(r,
get_canned_error_string(status, r, location),
NULL);
if (recursive_error) {
- ap_rvputs_proto_in_ascii(rlast, "<p>Additionally, a ",
+ ap_rvputs_proto_in_ascii(r, "<p>Additionally, a ",
status_lines[ap_index_of_response(recursive_error)],
"\nerror was encountered while trying to use an "
"ErrorDocument to handle the request.</p>\n", NULL);
}
- ap_rvputs_proto_in_ascii(rlast, ap_psignature("<hr />\n", r), NULL);
- ap_rvputs_proto_in_ascii(rlast, "</body></html>\n", NULL);
+ ap_rvputs_proto_in_ascii(r, ap_psignature("<hr />\n", r), NULL);
+ ap_rvputs_proto_in_ascii(r, "</body></html>\n", NULL);
}
ap_finalize_request_protocol(r);
}
Index: modules/http/http_request.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_request.c,v
retrieving revision 1.145
diff -u -r1.145 http_request.c
--- modules/http/http_request.c 31 May 2002 07:37:19 -0000 1.145
+++ modules/http/http_request.c 2 Jun 2002 22:50:15 -0000
@@ -145,17 +145,6 @@
if (ap_status_drops_connection(r->status)) {
r->connection->keepalive = 0;
}
- else if ((r->status != HTTP_NOT_MODIFIED) &&
- (r->status != HTTP_NO_CONTENT) &&
- r->connection && (r->connection->keepalive != -1)) {
- /* If the discard returns AP_FILTER_ERROR, it means that we went
- * recursive on ourselves and we should abort.
- */
- int errstatus = ap_discard_request_body(r);
- if (errstatus == AP_FILTER_ERROR) {
- return;
- }
- }
/*
* Two types of custom redirects --- plain text, and URLs. Plain text has
Index: modules/mappers/mod_negotiation.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/mappers/mod_negotiation.c,v
retrieving revision 1.102
diff -u -r1.102 mod_negotiation.c
--- modules/mappers/mod_negotiation.c 17 May 2002 11:24:16 -0000 1.102
+++ modules/mappers/mod_negotiation.c 2 Jun 2002 22:50:18 -0000
@@ -2818,9 +2818,6 @@
apr_bucket *e;
ap_allow_standard_methods(r, REPLACE_ALLOW, M_GET, M_OPTIONS, M_POST, -1);
- if ((res = ap_discard_request_body(r)) != OK) {
- return res;
- }
/*if (r->method_number == M_OPTIONS) {
* return ap_send_http_options(r);
*}
@@ -2841,6 +2838,9 @@
return res;
}
+ if ((res = ap_discard_request_body(r)) != OK) {
+ return res;
+ }
bb = apr_brigade_create(r->pool, c->bucket_alloc);
e = apr_bucket_file_create(map, best->body,
(apr_size_t)best->bytes, r->pool,
Index: server/core.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/core.c,v
retrieving revision 1.182
diff -u -r1.182 core.c
--- server/core.c 31 May 2002 20:52:28 -0000 1.182
+++ server/core.c 2 Jun 2002 22:50:23 -0000
@@ -3196,15 +3196,6 @@
ap_allow_standard_methods(r, MERGE_ALLOW, M_GET, M_OPTIONS, M_POST, -1);
- /* If filters intend to consume the request body, they must
- * register an InputFilter to slurp the contents of the POST
- * data from the POST input stream. It no longer exists when
- * the output filters are invoked by the default handler.
- */
- if ((errstatus = ap_discard_request_body(r)) != OK) {
- return errstatus;
- }
-
if (r->method_number == M_GET || r->method_number == M_POST) {
if (r->finfo.filetype == 0) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
@@ -3242,6 +3233,11 @@
if (bld_content_md5) {
apr_table_setn(r->headers_out, "Content-MD5",
ap_md5digest(r->pool, fd));
+ }
+
+ /* We know we are okay to respond to this, so discard the body. */
+ if ((errstatus = ap_discard_request_body(r)) != OK) {
+ return errstatus;
}
bb = apr_brigade_create(r->pool, c->bucket_alloc);
Index: server/protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/protocol.c,v
retrieving revision 1.104
diff -u -r1.104 protocol.c
--- server/protocol.c 17 May 2002 11:11:37 -0000 1.104
+++ server/protocol.c 2 Jun 2002 22:50:25 -0000
@@ -968,8 +968,9 @@
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
"client sent an unrecognized expectation value of "
"Expect: %s", expect);
- ap_send_error_response(r, 0);
- (void) ap_discard_request_body(r);
+ if (ap_discard_request_body(r) != AP_FILTER_ERROR) {
+ ap_send_error_response(r, 0);
+ }
ap_run_log_transaction(r);
return r;
}
Re: [PATCH] ap_discard_request_body() can't be called more than once
Posted by Justin Erenkrantz <je...@apache.org>.
On Mon, Jun 03, 2002 at 05:29:14PM -0700, Greg Stein wrote:
> This is all getting *way* too complicated.
I agree, but I think this is something that we've let slide for a
while now. Only now are we trying to get this code correct.
We currently call ap_discard_request_body without understanding
what it means to discard the request body. Lots of handlers are
calling discard in the wrong places without understanding the
ramifications of doing so.
> I recall seeing an email where somebody suggested putting a flag in the
> request_rec to determine whether HTTP_IN had seen an EOS or not. Bleh. At a
> minimum, that would go into the context for HTTP_IN.
Here's my core question: "Can HTTP_IN return EOS more than once?"
So, if two places in the request path try to read the request
entity, what do we return to the second guy since the first caller
read until EOS? (This is highly related to my question about
the double #exec cgi mod_include request.)
> I think that the right answer is that when the request_rec is about to go
> away, that any unread body content should be read by the framework. It would
> be really nice to have filter logic to say "call me when I'm done" so that
> HTTP_IN could go ahead and read the rest of the request right then.
Could we get into any "deadlock" situations at the network layer if
both input and output socket buffers were full? Imagine if we didn't
discard a large request body until the very end and we had a large
response - could the TCP stack run out of window space since the
client won't send anything until we read (and ACK) the input?
> If that were done, then you wouldn't have to worry about a bunch of rules
> all over the place, about when to call it, to avoid double-calls, etc. In
> fact, all of the calls could just go away...
>
> [ note that double-calling ap_discard_request_body() is quite fine. ]
Because ap_discard_request_body() becomes a no-op, right?
> My vote would be to put something into ap_finalize_request_protocol(). (the
> problem, of course, is recovering the HTTP_IN context; short of that,
> putting the flag into the request_rec or maybe the 'core_request_config'
> structure (hmm; the latter would be better).
Isn't HTTP_IN's context is still present in the request chain? Why
would it have been destroyed? -- justin
Re: [PATCH] ap_discard_request_body() can't be called more than once
Posted by Greg Stein <gs...@lyra.org>.
On Sun, Jun 02, 2002 at 04:40:41PM -0700, Justin Erenkrantz wrote:
> This patch combined with the last few patches I've posted today allow
> chunked trailer support again and now passes all httpd-test cases.
> What we try to do is to ensure that ap_discard_request_body() is not
> called before the handler "accepts" the request and begins generating
> the output. It is still possible for a bad module to call discard
> more than once or improperly.
>
> This effectively reverts Ryan's patch to http_protocol.c, so
> I'd appreciate it gets some review (preferably from Ryan
> himself!). -- justin
This is all getting *way* too complicated.
I recall seeing an email where somebody suggested putting a flag in the
request_rec to determine whether HTTP_IN had seen an EOS or not. Bleh. At a
minimum, that would go into the context for HTTP_IN.
I think that the right answer is that when the request_rec is about to go
away, that any unread body content should be read by the framework. It would
be really nice to have filter logic to say "call me when I'm done" so that
HTTP_IN could go ahead and read the rest of the request right then.
If that were done, then you wouldn't have to worry about a bunch of rules
all over the place, about when to call it, to avoid double-calls, etc. In
fact, all of the calls could just go away...
[ note that double-calling ap_discard_request_body() is quite fine. ]
My vote would be to put something into ap_finalize_request_protocol(). (the
problem, of course, is recovering the HTTP_IN context; short of that,
putting the flag into the request_rec or maybe the 'core_request_config'
structure (hmm; the latter would be better).
Note that mod_dav calls it a ton because it generates complete error
responses. It needs to suck up any body, then generate the error, then
return DONE. By returning DONE, we prevent an attempt by ap_die() to
generate messages, but it also means that ap_die() won't discard the body.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/