You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@bellsouth.net> on 2000/10/14 07:43:09 UTC
[PATCH] dechunking, body filtering
Okay, I didn't test body filtering but I think all the ducks are in a
row :) An eos bucket marks the end of the request body. Filters
between dechunk_filter() and ap_get_client_block() don't have to watch
out for lengths, so it doesn't seem any trickier than filtering the
body of a response.
Note that this is the product of a sleep-deprived person, but it seems
to work. A few tweaks were made here and there to create one possible
implementation of dechunking and what I think is a simpler interface
for request body filters than worrying about length parameters.
--------------------------------------
A few notes, with varying levels of completeness...
X means still-to-do
Data structure changes
http_filter's ctx:
Add remaining field. ap_setup_client_block() and dechunk_filter() know how to
find this ctx and set this field.
r
Add "void *filtered_input" field; ap_get_client_block() holds onto
leftover body in this field. Ugly that we clutter the r... Ugly that
I made it "void *" instead of "ap_bucket_brigade *" (avoids an additional
#include in "httpd.h" for (probably) no good reason).
Code changes
ap_setup_client_block()
remember that the read_policy parameter is copied to r->read_body;
if read_policy is REQUEST_CHUNKED_PASS and the transport encoding is chunked...
Disable filtering of the request body, because chunk headers would be invalid
if the filter changed the length.
r->input_filters = r->connection->input_filters;
dechunk_filter() will look at the read policy to know whether or not to leave the
chunk headers.
if transport encoding is chunked, add dechunk_filter() to r->input_filters; we'll
play games with the registered filter type so that it gets added at the right spot
(not *my* problem :) )
if transport encoding is not chunked (have content-length), set http_filter's
ctx->remaining to the content length
ap_get_client_block()
lots of logic goes away as we don't care how the body is presented
recap:
using content-length: ap_setup_client_block() told http_filter() everything
it needs to know
using chunked encoding: ap_setup_client_block() inserted the dechunk filter
on entry, if (!r->filtered_input) then allocate r->filtered_input (a brigade)
eos will be present at the end of the body; that is the only way we know we've
seen everything; if the filter below us gives us too much, we have to squirrel it
away in the brigade r->filtered_input
http_filter()
header logic is unchanged
when ctx->remaining > 0...
decrement ctx->remaining as we gather buckets to deliver to our caller;
when ctx->remaining reaches zero, split as needed to avoid delivering part of
the next request (or chunk header) as body; also, append an eos bucket to
signify the end of the request body (or of the current chunk)
as soon as we have data to deliver to the caller, do so... we won't try
to gather any certain amount before returning;
core's register_hooks()
register the dechunk filter; play games with filter type so that it is
added "properly"
dechunk_filter()
This is in the r->input_filters list, not in the c->input_filters list.
This is added by ap_setup_client_block() when the transport encoding is chunked.
Ordinarily, it will remove the chunk headers/trailers from the stream of data.
However, if the module wants to see them (REQUEST_CHUNKED_PASS), we'll leave them
in.
This decides what to do based on the state...
waiting-for-chunk-header
ensure that http_filter's ctx->remaining is zero, which tells http_filter()
to read a header, then call ap_get_brigade()
when we read the header, get the length and store it in http_filter's
ctx->remaining;
if the length is zero (final chunk), add an eos bucket to the brigade we
pass to the caller; otherwise, set state to processing-body
processing-body
we just call ap_get_brigade(); http_filter() returns us body bytes; it
will add an eos bucket after the last byte of the chunk; we always remove
that eos bucket; once we see the eos bucket, we change he state to
waiting-for-chunk-trailer; http_filter's ctx->remaining should be zero
at this point
waiting-for-trailer
http_filter() will return an empty line for this
joe_input_filter()
X call ap_get_next_brigade() to get data, of course... we place no limitations
X on how much we get back and we feel free to return whatever is convenient to
X our caller
X
X we watch out for the eos bucket which signifies end of body
Not implemented yet:
X getting rid of the length parameters on ap_get_brigade() and the input filters (a
trivial change)
X limiting the request body size; dechunk() needs to be part of this...
X CORE_IN shouldn't deliver the whole request body if it is big... one I/O
buffer worth of request body is plenty... otherwise we eat up virtual storage
Temporary hacks:
ap_setup_client_block() allows chunked encoding on POST
Not-so-temporary hacks (i.e., I don't know how to fix it):
There must be a nice way to avoid r->filtered_input (other than imposing
length games on all input filters).
Known bugs:
X post 10000 bytes -- CGI sees 10002 bytes; the extra two are CR+LF at the end of
the body; depends on whether or not I'm debugging it :(
Index: include/http_protocol.h
===================================================================
RCS file: /home/cvspublic/apache-2.0/src/include/http_protocol.h,v
retrieving revision 1.28
diff -u -r1.28 http_protocol.h
--- include/http_protocol.h 2000/10/12 16:35:37 1.28
+++ include/http_protocol.h 2000/10/14 05:25:06
@@ -530,6 +530,7 @@
API_EXPORT(const char *) ap_method_name_of(int methnum);
int http_filter(ap_filter_t *f, ap_bucket_brigade *b, apr_ssize_t length);
+apr_status_t dechunk_filter(ap_filter_t *f, ap_bucket_brigade *b, apr_ssize_t length);
/* Hooks */
/*
Index: include/httpd.h
===================================================================
RCS file: /home/cvspublic/apache-2.0/src/include/httpd.h,v
retrieving revision 1.101
diff -u -r1.101 httpd.h
--- include/httpd.h 2000/10/13 18:39:16 1.101
+++ include/httpd.h 2000/10/14 05:25:08
@@ -836,6 +836,9 @@
/** A flag to determine if the eos bucket has been sent yet
* @defvar int eos_sent */
int eos_sent;
+ /** ap_get_client_block() holds onto additional data here
+ * @defvar voide *filtered_input */
+ void *filtered_input;
/* Things placed at the end of the record to avoid breaking binary
* compatibility. It would be nice to remember to reorder the entire
Index: main/http_core.c
===================================================================
RCS file: /home/cvspublic/apache-2.0/src/main/http_core.c,v
retrieving revision 1.168
diff -u -r1.168 http_core.c
--- main/http_core.c 2000/10/13 17:36:51 1.168
+++ main/http_core.c 2000/10/14 05:25:52
@@ -3582,6 +3582,7 @@
*/
ap_hook_insert_filter(core_insert_filter, NULL, NULL, AP_HOOK_MIDDLE);
ap_register_input_filter("HTTP_IN", http_filter, AP_FTYPE_CONNECTION);
+ ap_register_input_filter("DECHUNK", dechunk_filter, AP_FTYPE_CONNECTION + 1);
ap_register_input_filter("CORE_IN", core_input_filter, AP_FTYPE_CONNECTION);
ap_register_output_filter("CORE", core_output_filter, AP_FTYPE_CONNECTION + 1);
ap_register_output_filter("CHUNK", chunk_filter, AP_FTYPE_CONNECTION);
Index: main/http_protocol.c
===================================================================
RCS file: /home/cvspublic/apache-2.0/src/main/http_protocol.c,v
retrieving revision 1.169
diff -u -r1.169 http_protocol.c
--- main/http_protocol.c 2000/10/13 18:39:18 1.169
+++ main/http_protocol.c 2000/10/14 05:25:58
@@ -856,10 +856,147 @@
return AP_HTTP_METHODS[methnum];
}
+static apr_status_t brigade_bgets(ap_bucket_brigade *bb,
+ int remove_buckets,
+ char *line,
+ apr_ssize_t max_line_size)
+{
+ const char *buf;
+ apr_ssize_t len;
+ ap_bucket *e = AP_BRIGADE_FIRST(bb), *old;
+
+ while (e != AP_BRIGADE_SENTINEL(bb)) {
+ ap_bucket_read(e, &buf, &len, 0);
+ if (len + 2 >= max_line_size) { /* overflow */
+ return APR_EINVAL; /* what a stupid choice */
+ }
+ memcpy(line, buf, len);
+ line += len;
+ max_line_size -= len;
+ old = e;
+ e = AP_BUCKET_NEXT(e);
+ if (remove_buckets) {
+ AP_BUCKET_REMOVE(old);
+ ap_bucket_destroy(old);
+ }
+ }
+ --line; /* point at '\n' */
+ ap_debug_assert(*line == '\n');
+ if (*(line - 1) == '\r') {
+ --line;
+ }
+ *line = '\0';
+ return APR_SUCCESS;
+}
+
typedef struct http_filter_ctx {
ap_bucket_brigade *b;
+ apr_ssize_t remaining;
} http_ctx_t;
+static http_ctx_t *find_http_ctx(conn_rec *c)
+{
+ ap_debug_assert(!strcmp(c->input_filters->frec->name, "HTTP_IN"));
+ return c->input_filters->ctx;
+}
+
+struct dechunk_ctx {
+ apr_ssize_t chunk_size;
+ apr_ssize_t bytes_delivered;
+ enum {WANT_HDR /* must have value zero */, WANT_BODY, WANT_TRL} state;
+ int remove_chunk_proto;
+};
+
+static long get_chunk_size(char *);
+
+apr_status_t dechunk_filter(ap_filter_t *f, ap_bucket_brigade *bb,
+ apr_ssize_t length)
+{
+ apr_status_t rv;
+ struct dechunk_ctx *ctx = f->ctx;
+ ap_bucket *b;
+ const char *buf;
+ apr_ssize_t len;
+
+ if (!ctx) {
+ f->ctx = ctx = apr_pcalloc(f->r->pool, sizeof(struct dechunk_ctx));
+ ctx->remove_chunk_proto = f->r->read_body != REQUEST_CHUNKED_PASS;
+ }
+
+ do {
+ if (ctx->chunk_size == ctx->bytes_delivered) {
+ /* Time to read another chunk header or trailer... http_filter() is
+ * the next filter in line and it knows how to return a brigade with
+ * one line.
+ */
+ char line[30];
+
+ ap_debug_assert(!strcmp(f->next->frec->name, "HTTP_IN"));
+ if ((rv = ap_get_brigade(f->next, bb, AP_GET_LINE)) != APR_SUCCESS) {
+ return rv;
+ }
+ if ((rv = brigade_bgets(bb, ctx->remove_chunk_proto,
+ line, sizeof(line))) != APR_SUCCESS) {
+ return rv;
+ }
+ switch(ctx->state) {
+ case WANT_HDR:
+ ctx->chunk_size = get_chunk_size(line);
+ ctx->bytes_delivered = 0;
+ if (ctx->chunk_size == 0) {
+ ctx->state = WANT_TRL;
+ }
+ else {
+ ctx->state = WANT_BODY;
+ }
+ break;
+ case WANT_TRL:
+ /* XXX sanity check end chunk here */
+ if (strlen(line)) {
+ /* bad trailer */
+ }
+ if (ctx->chunk_size == 0) { /* we just finished the last chunk? */
+ /* append eos bucket and get out */
+ b = ap_bucket_create_eos();
+ AP_BRIGADE_INSERT_TAIL(bb, b);
+ return APR_SUCCESS;
+ }
+ ctx->state = WANT_HDR;
+ break;
+ default:
+ ap_assert(ctx->state == WANT_HDR || ctx->state == WANT_TRL);
+ }
+ }
+ } while (ctx->state != WANT_BODY);
+
+ if (ctx->state == WANT_BODY) {
+ /* Tell http_filter() how many bytes to deliver. */
+ find_http_ctx(f->c)->remaining = ctx->chunk_size - ctx->bytes_delivered;
+ if ((rv = ap_get_brigade(f->next, bb, 999)) != APR_SUCCESS) {
+ return rv;
+ }
+ /* Walk through the body, accounting for bytes, and removing an eos bucket if
+ * http_filter() delivered the entire chunk.
+ */
+ b = AP_BRIGADE_FIRST(bb);
+ while (b != AP_BRIGADE_SENTINEL(bb) &&
+ b->type != ap_eos_type()) {
+ ap_bucket_read(b, &buf, &len, 1);
+ ap_debug_assert(len <= ctx->chunk_size - ctx->bytes_delivered);
+ ctx->bytes_delivered += len;
+ b = AP_BUCKET_NEXT(b);
+ }
+ if (ctx->bytes_delivered == ctx->chunk_size) {
+ ap_debug_assert(b->type == ap_eos_type());
+ AP_BUCKET_REMOVE(b);
+ ap_bucket_destroy(b);
+ ctx->state = WANT_TRL;
+ }
+ }
+
+ return APR_SUCCESS;
+}
+
apr_status_t http_filter(ap_filter_t *f, ap_bucket_brigade *b, apr_ssize_t length)
{
#define ASCII_LF '\012'
@@ -889,32 +1026,35 @@
}
}
- if (length > 0) {
- int remain = length;
+ if (ctx->remaining > 0) {
const char *ignore;
e = AP_BRIGADE_FIRST(b);
while (e != AP_BRIGADE_SENTINEL(b)) {
ap_bucket_read(e, &ignore, &len, 0);
- if (remain <= len) {
+ if (ctx->remaining < len) {
break;
}
- remain -= len;
+ ctx->remaining -= len;
e = AP_BUCKET_NEXT(e);
}
if (e != AP_BRIGADE_SENTINEL(b)) {
- if (remain <= len) {
- ap_bucket_split(e, remain);
- remain = 0;
+ if (ctx->remaining < len) {
+ ap_bucket_split(e, ctx->remaining);
+ ctx->remaining = 0;
}
bb = ap_brigade_split(b, AP_BUCKET_NEXT(e));
ctx->b = bb;
- return APR_SUCCESS;
}
else {
ctx->b = NULL;
- return APR_SUCCESS;
}
+ if (ctx->remaining == 0) {
+ ap_bucket *eos = ap_bucket_create_eos();
+
+ AP_BRIGADE_INSERT_TAIL(b, eos);
+ }
+ return APR_SUCCESS;
}
AP_BRIGADE_FOREACH(e, b) {
@@ -2269,6 +2409,11 @@
"Unknown Transfer-Encoding %s", tenc);
return HTTP_NOT_IMPLEMENTED;
}
+/*XXX hack to use mod_cgi[d] for testing chunked request bodies XXX */
+ if (r->read_body == REQUEST_CHUNKED_ERROR) {
+ r->read_body = REQUEST_CHUNKED_DECHUNK;
+ }
+/*XXX end of this particular hack :) */
if (r->read_body == REQUEST_CHUNKED_ERROR) {
ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
"chunked Transfer-Encoding forbidden: %s", r->uri);
@@ -2276,6 +2421,11 @@
}
r->read_chunked = 1;
+ if (r->read_body == REQUEST_CHUNKED_PASS) {
+ /* can't filter the body *and* preserve the chunk headers */
+ r->input_filters = r->connection->input_filters;
+ }
+ ap_add_input_filter("DECHUNK", NULL, r, r->connection);
}
else if (lenp) {
const char *pos = lenp;
@@ -2290,6 +2440,7 @@
}
r->remaining = atol(lenp);
+ find_http_ctx(r->connection)->remaining = r->remaining;
}
if ((r->read_body == REQUEST_NO_BODY) &&
@@ -2388,208 +2539,73 @@
*/
API_EXPORT(long) ap_get_client_block(request_rec *r, char *buffer, int bufsiz)
{
- int c;
- apr_size_t len_to_read;
apr_ssize_t len_read, total;
- long chunk_start = 0;
- long max_body;
apr_status_t rv;
apr_int32_t timeout;
ap_bucket *b, *old;
ap_bucket_brigade *bb;
-
- if (!r->read_chunked) { /* Content-length read */
- const char *tempbuf;
+ const char *tempbuf;
- len_to_read = (r->remaining > bufsiz) ? bufsiz : r->remaining;
-
- if (len_to_read == 0) {
- return 0;
- }
- bb = ap_brigade_create(r->pool);
- do {
- if (AP_BRIGADE_EMPTY(bb)) {
- apr_getsocketopt(r->connection->client->bsock, APR_SO_TIMEOUT, &timeout);
- apr_setsocketopt(r->connection->client->bsock, APR_SO_TIMEOUT, 0);
- if (ap_get_brigade(r->input_filters, bb, len_to_read) != APR_SUCCESS) {
- /* if we actually fail here, we want to just return and
- * stop trying to read data from the client.
- */
- apr_setsocketopt(r->connection->client->bsock, APR_SO_TIMEOUT, timeout);
- r->connection->keepalive = -1;
- ap_brigade_destroy(bb);
- return -1;
- }
- apr_setsocketopt(r->connection->client->bsock, APR_SO_TIMEOUT, timeout);
- }
- b = AP_BRIGADE_FIRST(bb);
-
- while (b->length == 0 && b != AP_BRIGADE_SENTINEL(bb)) {
- ap_bucket *e = b;
- b = AP_BUCKET_NEXT(e);
- AP_BUCKET_REMOVE(e);
- ap_bucket_destroy(e);
- }
- } while (AP_BRIGADE_EMPTY(bb));
-
- total = 0;
- do {
- rv = ap_bucket_read(b, &tempbuf, &len_read, 0);
- ap_debug_assert(total + len_read <= bufsiz); /* because we told the filter
- * below us not to give us too much */
- ap_debug_assert(r->remaining >= len_read);
- memcpy(buffer, tempbuf, len_read);
- buffer += len_read;
- r->read_length += len_read;
- total += len_read;
- r->remaining -= len_read;
- old = b;
- b = AP_BUCKET_NEXT(b);
- AP_BUCKET_REMOVE(old);
- ap_bucket_destroy(old);
- } while (b != AP_BRIGADE_SENTINEL(bb));
- ap_brigade_destroy(bb);
- return total;
+ if (!r->filtered_input) {
+ r->filtered_input = ap_brigade_create(r->pool);
}
+ bb = (ap_bucket_brigade *)r->filtered_input;
- /*
- * Handle chunked reading Note: we are careful to shorten the input
- * bufsiz so that there will always be enough space for us to add a CRLF
- * (if necessary).
- */
- if (r->read_body == REQUEST_CHUNKED_PASS)
- bufsiz -= 2;
- if (bufsiz <= 0)
- return -1; /* Cannot read chunked with a small buffer */
-
- /* Check to see if we have already read too much request data.
- * For efficiency reasons, we only check this at the top of each
- * caller read pass, since the limit exists just to stop infinite
- * length requests and nobody cares if it goes over by one buffer.
- */
- max_body = ap_get_limit_req_body(r);
- if (max_body && (r->read_length > max_body)) {
- ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
- "Chunked request body is larger than the "
- "configured limit of %lu", max_body);
- r->connection->keepalive = -1;
- return -1;
- }
-
- if (r->remaining == 0) { /* Start of new chunk */
-
- chunk_start = getline(buffer, bufsiz, r->connection, 0);
- if ((chunk_start <= 0) || (chunk_start >= (bufsiz - 1))
- || !apr_isxdigit(*buffer)) {
- r->connection->keepalive = -1;
- return -1;
- }
-
- len_to_read = get_chunk_size(buffer);
-
- if (len_to_read == 0) { /* Last chunk indicated, get footers */
- if (r->read_body == REQUEST_CHUNKED_DECHUNK) {
- get_mime_headers(r);
- apr_snprintf(buffer, bufsiz, "%ld", r->read_length);
- apr_table_unset(r->headers_in, "Transfer-Encoding");
- apr_table_setn(r->headers_in, "Content-Length",
- apr_pstrdup(r->pool, buffer));
- return 0;
- }
- r->remaining = -1; /* Indicate footers in-progress */
- }
- else {
- r->remaining = len_to_read;
- }
- if (r->read_body == REQUEST_CHUNKED_PASS) {
- buffer[chunk_start++] = CR; /* Restore chunk-size line end */
- buffer[chunk_start++] = LF;
- buffer += chunk_start; /* and pass line on to caller */
- bufsiz -= chunk_start;
- }
- else {
- /* REQUEST_CHUNKED_DECHUNK -- do not include the length of the
- * header in the return value
- */
- chunk_start = 0;
- }
- }
- /* When REQUEST_CHUNKED_PASS, we are */
- if (r->remaining == -1) { /* reading footers until empty line */
- len_read = chunk_start;
-
- while ((bufsiz > 1)
- && ((len_read = getline(buffer, bufsiz, r->connection,
- 1)) > 0)) {
-
- if (len_read != (bufsiz - 1)) {
- buffer[len_read++] = CR; /* Restore footer line end */
- buffer[len_read++] = LF;
+ do {
+ if (AP_BRIGADE_EMPTY(bb)) {
+ apr_getsocketopt(r->connection->client->bsock, APR_SO_TIMEOUT, &timeout);
+ apr_setsocketopt(r->connection->client->bsock, APR_SO_TIMEOUT, 0);
+ if (ap_get_brigade(r->input_filters, bb, 9999) != APR_SUCCESS) {
+ /* if we actually fail here, we want to just return and
+ * stop trying to read data from the client.
+ */
+ apr_setsocketopt(r->connection->client->bsock, APR_SO_TIMEOUT, timeout);
+ r->connection->keepalive = -1;
+ ap_brigade_destroy(bb);
+ return -1;
}
- chunk_start += len_read;
- buffer += len_read;
- bufsiz -= len_read;
+ apr_setsocketopt(r->connection->client->bsock, APR_SO_TIMEOUT, timeout);
}
- if (len_read < 0) {
- r->connection->keepalive = -1;
- return -1;
+ b = AP_BRIGADE_FIRST(bb);
+ while (b->type != ap_eos_type() &&
+ b->length == 0 && b != AP_BRIGADE_SENTINEL(bb)) {
+ ap_bucket *e = b;
+ b = AP_BUCKET_NEXT(e);
+ AP_BUCKET_REMOVE(e);
+ ap_bucket_destroy(e);
}
+ } while (AP_BRIGADE_EMPTY(bb));
- if (len_read == 0) { /* Indicates an empty line */
- buffer[0] = CR;
- buffer[1] = LF;
- chunk_start += 2;
- r->remaining = -2;
- }
- r->read_length += chunk_start;
- return chunk_start;
- }
- /* When REQUEST_CHUNKED_PASS, we */
- if (r->remaining == -2) { /* finished footers when last called */
- r->remaining = 0; /* so now we must signal EOF */
+ if (b->type == ap_eos_type()) { /* reached eos on previous invocation */
+ AP_BUCKET_REMOVE(b);
+ ap_bucket_destroy(b);
return 0;
}
-
- /* Otherwise, we are in the midst of reading a chunk of data */
-
- len_to_read = (r->remaining > bufsiz) ? bufsiz : r->remaining;
-
- (void) ap_bread(r->connection->client, buffer, len_to_read, &len_read);
- if (len_read == 0) { /* error or eof */
- r->connection->keepalive = -1;
- return -1;
- }
-
- r->remaining -= len_read;
-
- if (r->remaining == 0) { /* End of chunk, get trailing CRLF */
-#ifdef APACHE_XLATE
- /* Chunk end is Protocol stuff! Set conversion = 1 to read CR LF: */
- AP_PUSH_INPUTCONVERSION_STATE(r->connection->client,
- ap_hdrs_from_ascii);
-#endif /*APACHE_XLATE*/
-
- if ((c = ap_bgetc(r->connection->client)) == CR) {
- c = ap_bgetc(r->connection->client);
- }
-#ifdef APACHE_XLATE
- /* restore previous input translation handle */
- AP_POP_INPUTCONVERSION_STATE(r->connection->client);
-#endif /*APACHE_XLATE*/
-
- if (c != LF) {
- r->connection->keepalive = -1;
+ total = 0;
+ while (total < bufsiz && b != AP_BRIGADE_SENTINEL(bb) && b->type != ap_eos_type()) {
+ if ((rv = ap_bucket_read(b, &tempbuf, &len_read, 0)) != APR_SUCCESS) {
return -1;
}
- if (r->read_body == REQUEST_CHUNKED_PASS) {
- buffer[len_read++] = CR;
- buffer[len_read++] = LF;
- }
+ if (total + len_read > bufsiz) {
+ ap_bucket_split(b, bufsiz - total);
+ len_read = bufsiz - total;
+ }
+ memcpy(buffer, tempbuf, len_read);
+ buffer += len_read;
+ total += len_read;
+ /* XXX the next two fields shouldn't be mucked with here, as they are in terms
+ * of bytes in the unfiltered body
+ */
+ r->read_length += len_read; /* XXX are these fields important? */
+ r->remaining -= len_read; /* XXX if so, tell http_filter() where the r is? */
+ old = b;
+ b = AP_BUCKET_NEXT(b);
+ AP_BUCKET_REMOVE(old);
+ ap_bucket_destroy(old);
}
- r->read_length += (chunk_start + len_read);
- return (chunk_start + len_read);
+ return total;
}
/* In HTTP/1.1, any method can have a body. However, most GET handlers
Off to catch some zzzs...
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] dechunking, body filtering
Posted by Jeff Trawick <tr...@bellsouth.net>.
Oops... A few hours ago I mistakenly responded to Ryan directly
instead of to the list, and I didn't have gnus set up to archive
outgoing messages :( I'll attempt to reconstruct my comments to Ryan
(and hopefully prove that gnus is now configured in a reasonable
way)... I'm sorry if this leads to confusion if Ryan responds to my
orginal note...
rbb@covalent.net writes:
> > Data structure changes
> >
> > http_filter's ctx:
> >
> > Add remaining field. ap_setup_client_block() and dechunk_filter() know how to
> > find this ctx and set this field.
>
> You can't do this. You can't even garauntee that http_filter will be in
> the server. I have every intention of writing an input filter that takes
> ftp requests and converts them to http. When I do that, there will be no
> http_filter in the request, and this will break. No filter can EVER
> depend on another filter being in the stack.
So put your ftp->http converter filter *below* HTTP_IN.
If Apache's http infrastructure is being used then it needs to know
that HTTP_IN is there. HTTP_IN is just a cog in the HTTP support
provided by Apache, interdependent with other parts. We don't let
modules replace just any part of Apache's HTTP support. It is o.k. to
have some interdependent pieces which can't be replaced individually.
An example of another case where we'll want to know the state of
HTTP_IN *outside* of HTTP_IN:
In some response buffering logic, we'll want to know if we've received
a pipelined request yet. If so, we won't flush to the network... If
not, we'll flush to the network.
How will we know if another request has arrived without querying the
state of HTTP_IN, since if the request arrived HTTP_IN hid the
knowledge of it from the filters above?
> > r
> >
> > Add "void *filtered_input" field; ap_get_client_block() holds onto
> > leftover body in this field. Ugly that we clutter the r... Ugly that
> > I made it "void *" instead of "ap_bucket_brigade *" (avoids an additional
> > #include in "httpd.h" for (probably) no good reason).
>
> This was discussed yesterday and people really disliked it. If filters
> are told the maximum amount of data they can return, this is
> unnecessary.
Some people disliked it; some people didn't ;) I don't have any
problem at all with ap_get_client_block() saving state information
between invocations. I agree that directly in the r isn't a nice
place, but we can perhaps find a better place rather than imposing the
length parameter. What about r->notes? (If there is no other place,
I find putting it in the r preferable to using the length parameters.)
Some issues with the length parameter(s):
. HTTP_IN deals with byte counts on unfiltered input...
ap_get_client_block() deals with byte counts on filtered input...
How does the discrepancy get resolved?
. what if a filter below ap_get_client_block() wants to return a pipe
bucket because the request body was filtered on the other side of a
pipe? how do we govern the length there?
. I don't see the length parameter(s) as being helpful overall to most
filters... sure, the filter gets the opportunity to place
restrictions on the filter below it, possibly simplifying some
logic, but on the other hand, the filter has to satisfy the
restrictions of the filter (or ap_get_client_block() or whatever)
above it... we just move the problem somewhere else...
it seems less error prone and simpler to let filters deliver data as
they see fit (like output filters); this length checking is extra
code in every input filter; I'm sure it isn't rocket science, but I
don't see the benefit
If the patch is bad because
. HTTP support in one place in Apache (ap_get_client_block()) has to
be aware of HTTP support in another place in Apache (HTTP_IN)
. ap_get_client_block() has to save state between invocations
then I can live with that :)
Thanks for your comments,
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] dechunking, body filtering
Posted by rb...@covalent.net.
I have reviewed the patch in some detail and some parts of it are very
good. Others I just can't agree with. Some sketchy comments below. I'll
make more comments tomorrow morning.
> Data structure changes
>
> http_filter's ctx:
>
> Add remaining field. ap_setup_client_block() and dechunk_filter() know how to
> find this ctx and set this field.
You can't do this. You can't even garauntee that http_filter will be in
the server. I have every intention of writing an input filter that takes
ftp requests and converts them to http. When I do that, there will be no
http_filter in the request, and this will break. No filter can EVER
depend on another filter being in the stack.
> r
>
> Add "void *filtered_input" field; ap_get_client_block() holds onto
> leftover body in this field. Ugly that we clutter the r... Ugly that
> I made it "void *" instead of "ap_bucket_brigade *" (avoids an additional
> #include in "httpd.h" for (probably) no good reason).
This was discussed yesterday and people really disliked it. If filters
are told the maximum amount of data they can return, this is unnecessary.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] dechunking, body filtering
Posted by rb...@covalent.net.
Cool. I saw the commit. Ignore my message about fixing the things I
disliked, I assume you'll do that during the cleanup. I really think that
http_filter was basically correct before your last patch. I will wait
until you send e-mail saying you are done to really review things again.
Ryan
On 16 Oct 2000, Jeff Trawick wrote:
> rbb@covalent.net writes:
>
> > Well, I promised to do a more thorough review a few days ago, but I
> > forgot, here it is.
>
> Ryan... I committed a few minutes ago. I am first going to see if I
> lost anything from your changes from Friday night. We started from a
> common source but diverged wildly. After that, I will go through
> this note. I can see already that some of the issues are taken care
> of but others aren't.
>
> --
> Jeff Trawick | trawick@ibm.net | PGP public key at web site:
> http://www.geocities.com/SiliconValley/Park/9289/
> Born in Roswell... married an alien...
>
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] dechunking, body filtering
Posted by Jeff Trawick <tr...@bellsouth.net>.
rbb@covalent.net writes:
> Well, I promised to do a more thorough review a few days ago, but I
> forgot, here it is.
Ryan... I committed a few minutes ago. I am first going to see if I
lost anything from your changes from Friday night. We started from a
common source but diverged wildly. After that, I will go through
this note. I can see already that some of the issues are taken care
of but others aren't.
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] dechunking, body filtering
Posted by rb...@covalent.net.
> > > if read_policy is REQUEST_CHUNKED_PASS and the transport encoding is
> > > chunked...
> > >
> > > Disable filtering of the request body, because chunk headers would
> > > be invalid
> > > if the filter changed the length.
> >
> > I'm not sure I like this, but I dislike the other options more. The
> > problem here, is that we are changing how a request is received based on
> > information in the filter. Then again, we only use REQUEST_CHUNKED_PASS
> > when we are discarding the whole body anyway. I have no good answer, but
> > the solution I hate more than disabling filtering, is for the chunk filter
> > to fix the chunk headers. The solution I like a bit more, is to remove
> > this option altogether, although that too is a bit bogus. Just some food
> > for thought on this.
>
> I was wondering out loud some weeks ago on what to do with this
> issue... the only response I got was a "shrug" from Roy :)
>
> I've tried googling for REQUEST_CHUNKED_PASS but I didn't come up with
> anything outside of our code. I doubt that there is anything.
I agree 100%, I am shrugging in my comment basically, because I don't see
a good answer. Your solution is as good as any of the others. :-)
> > > X CORE_IN shouldn't deliver the whole request body if it is big... one I/O
> > > buffer worth of request body is plenty... otherwise we eat up virtual storage
> >
> > Core in just passes the brigade up to http_filter, or whatever filter is
> > there. http_filter should use a length of MAX_STRING_LENGTH when doing a
> > read.
>
> Yeah, I got my filter names wrong. Anyway... When http_filter() does
> a bucket-read to get data to deliver, it doesn't get to choose how
> much to read. In the normal case where it reads a socket bucket, it
> will get up to IOBUFSIZE bytes. A simple change for now would be to
> return once it does a read which delivers >0 bytes. When the block
> parameter to bucket read actually does something for a socket this
> code needs to be revisited anyway.
Yes.
> > > +static apr_status_t brigade_bgets(ap_bucket_brigade *bb,
> > > + int remove_buckets,
> > > + char *line,
> > > + apr_ssize_t max_line_size)
> >
> > <brigade_bgets snipped for brevity>
> >
> > Why isn't this just getline? This does a bit less than getline, but
> > getline is already there, and it is one less place for things to go
> > wrong.
>
> Tonight or tomorrow I'll look at using getline()...
BTW, I took a real quick look at getline. It is always by-passing the
request-based input headers, so this should just work.
> I went to the version of this code in cvs which I replaced today but
> can't find much that I should pick up. Part of it is that I report
> eos a little sooner... another difference is that the length parm is
> unused in my version... another issue is that some of the replaced
> code seemed to have the intention of telling the bucket read routine
> how much data to read. I've got some minor tweaks which make it look
> more like what I replaced, but there isn't anything major.
Yeah. After I saw the code completely, with the length parameter removed,
this looks good.
> > > + if (r->read_body == REQUEST_CHUNKED_PASS) {
> > > + /* can't filter the body *and* preserve the chunk headers */
> > > + r->input_filters = r->connection->input_filters;
> > > + }
> > > + ap_add_input_filter("DECHUNK", NULL, r, r->connection);
> >
> > This doesn't actually disable filtering. I'm not really sure of the
> > correct way to disable filtering, but I have the feeling this will
> > actually need to happen in ap_get_client_block, not
> > ap_setup_client_block. The problem is that it is perfectly possible for a
> > module to insert a filter between the two function calls.
> > Hmmmm.....
>
> no obvious solution...
The only other thing I can think of is to just remove all filters from the
request_rec. Of course, this requires ap_filter_remove be written.
This looks really good now. It was painful to get here, but I think we
finally got it right. :-)
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] dechunking, body filtering
Posted by Jeff Trawick <tr...@bellsouth.net>.
rbb@covalent.net writes:
> > r
> >
> > Add "void *filtered_input" field; ap_get_client_block() holds onto
> > leftover body in this field. Ugly that we clutter the r... Ugly that
> > I made it "void *" instead of "ap_bucket_brigade *" (avoids an additional
> > #include in "httpd.h" for (probably) no good reason).
>
> Mentioned in another note, but repeating it here for completeness. This
> belongs in core_dir_conf, not the request_rec. This is information only
> for a core function, so it shouldn't be available to everything. Also,
> this really should be a bucket_brigade *, but that's REALLY minor.
done, committed...
>
> > remember that the read_policy parameter is copied to r->read_body;
> >
> > if read_policy is REQUEST_CHUNKED_PASS and the transport encoding is
> > chunked...
> >
> > Disable filtering of the request body, because chunk headers would
> > be invalid
> > if the filter changed the length.
>
> I'm not sure I like this, but I dislike the other options more. The
> problem here, is that we are changing how a request is received based on
> information in the filter. Then again, we only use REQUEST_CHUNKED_PASS
> when we are discarding the whole body anyway. I have no good answer, but
> the solution I hate more than disabling filtering, is for the chunk filter
> to fix the chunk headers. The solution I like a bit more, is to remove
> this option altogether, although that too is a bit bogus. Just some food
> for thought on this.
I was wondering out loud some weeks ago on what to do with this
issue... the only response I got was a "shrug" from Roy :)
I've tried googling for REQUEST_CHUNKED_PASS but I didn't come up with
anything outside of our code. I doubt that there is anything.
> > r->input_filters = r->connection->input_filters;
> >
> > dechunk_filter() will look at the read policy to know whether or not to leave the
> > chunk headers.
> >
> > core's register_hooks()
> >
> > register the dechunk filter; play games with filter type so that it is
> > added "properly"
>
> BTW, I hate this, but that is a problem with our filter registration that
> we need to fix, and it has nothing to do with this patch.
yep...
> > X CORE_IN shouldn't deliver the whole request body if it is big... one I/O
> > buffer worth of request body is plenty... otherwise we eat up virtual storage
>
> Core in just passes the brigade up to http_filter, or whatever filter is
> there. http_filter should use a length of MAX_STRING_LENGTH when doing a
> read.
Yeah, I got my filter names wrong. Anyway... When http_filter() does
a bucket-read to get data to deliver, it doesn't get to choose how
much to read. In the normal case where it reads a socket bucket, it
will get up to IOBUFSIZE bytes. A simple change for now would be to
return once it does a read which delivers >0 bytes. When the block
parameter to bucket read actually does something for a socket this
code needs to be revisited anyway.
> > Not-so-temporary hacks (i.e., I don't know how to fix it):
> >
> > There must be a nice way to avoid r->filtered_input (other than imposing
> > length games on all input filters).
>
> Not that I can see either. I hate both solutions almost as much as you
> do. We disagree on the cleaner method, but I'll live with the
> r->filtered_input, as long as the brigade lives in core_dir_conf, not
> request_rec.
>
> > Known bugs:
> >
> > X post 10000 bytes -- CGI sees 10002 bytes; the extra two are CR+LF at the end of
> > the body; depends on whether or not I'm debugging it :(
>
> That's kind of cute.
I just remembered what the last sentence means: whether or not I'm in
gdb... gotta try to reproduce this one tonight after the kiddies go to bed
> > +static apr_status_t brigade_bgets(ap_bucket_brigade *bb,
> > + int remove_buckets,
> > + char *line,
> > + apr_ssize_t max_line_size)
>
> <brigade_bgets snipped for brevity>
>
> Why isn't this just getline? This does a bit less than getline, but
> getline is already there, and it is one less place for things to go
> wrong.
Tonight or tomorrow I'll look at using getline()...
> > +struct dechunk_ctx {
> > + apr_ssize_t chunk_size;
> > + apr_ssize_t bytes_delivered;
> > + enum {WANT_HDR /* must have value zero */, WANT_BODY, WANT_TRL} state;
> > + int remove_chunk_proto;
> > +};
> > +
> > +static long get_chunk_size(char *);
> > +
> > +apr_status_t dechunk_filter(ap_filter_t *f, ap_bucket_brigade *bb,
> > + apr_ssize_t length)
> > +{
> > + apr_status_t rv;
> > + struct dechunk_ctx *ctx = f->ctx;
> > + ap_bucket *b;
> > + const char *buf;
> > + apr_ssize_t len;
> > +
> > + if (!ctx) {
> > + f->ctx = ctx = apr_pcalloc(f->r->pool, sizeof(struct dechunk_ctx));
> > + ctx->remove_chunk_proto = f->r->read_body != REQUEST_CHUNKED_PASS;
> > + }
> > +
> > + do {
> > + if (ctx->chunk_size == ctx->bytes_delivered) {
> > + /* Time to read another chunk header or trailer... http_filter() is
> > + * the next filter in line and it knows how to return a brigade with
> > + * one line.
> > + */
> > + char line[30];
> > +
> > + ap_debug_assert(!strcmp(f->next->frec->name, "HTTP_IN"));
>
> This is bogus. You can not assume that the next filter in the chain is
> the http_filter. All you know, is that the next filter in the chain looks
> like http_filter. Remove the assert, and I'll be perfectly happy
> though.
it will be gone very soon...
> > + if ((rv = ap_get_brigade(f->next, bb, AP_GET_LINE)) != APR_SUCCESS) {
> > + return rv;
> > + }
> > + if ((rv = brigade_bgets(bb, ctx->remove_chunk_proto,
> > + line, sizeof(line))) != APR_SUCCESS) {
> > + return rv;
> > + }
>
> This should just be a single call to getline IMO.
(same as above comment regarding the use of getline())
> > apr_status_t http_filter(ap_filter_t *f, ap_bucket_brigade *b, apr_ssize_t length)
> > {
> > #define ASCII_LF '\012'
> > @@ -889,32 +1026,35 @@
> > }
> > }
> >
> > - if (length > 0) {
> > - int remain = length;
> > + if (ctx->remaining > 0) {
> > const char *ignore;
> >
> > e = AP_BRIGADE_FIRST(b);
> > while (e != AP_BRIGADE_SENTINEL(b)) {
> > ap_bucket_read(e, &ignore, &len, 0);
> > - if (remain <= len) {
> > + if (ctx->remaining < len) {
> > break;
> > }
> > - remain -= len;
> > + ctx->remaining -= len;
> > e = AP_BUCKET_NEXT(e);
> > }
> > if (e != AP_BRIGADE_SENTINEL(b)) {
> > - if (remain <= len) {
> > - ap_bucket_split(e, remain);
> > - remain = 0;
> > + if (ctx->remaining < len) {
> > + ap_bucket_split(e, ctx->remaining);
> > + ctx->remaining = 0;
> > }
> > bb = ap_brigade_split(b, AP_BUCKET_NEXT(e));
> > ctx->b = bb;
> > - return APR_SUCCESS;
> > }
> > else {
> > ctx->b = NULL;
> > - return APR_SUCCESS;
> > }
>
> All of the changes above should be removed, because they are already
> basically working.
I went to the version of this code in cvs which I replaced today but
can't find much that I should pick up. Part of it is that I report
eos a little sooner... another difference is that the length parm is
unused in my version... another issue is that some of the replaced
code seemed to have the intention of telling the bucket read routine
how much data to read. I've got some minor tweaks which make it look
more like what I replaced, but there isn't anything major.
> > + if (ctx->remaining == 0) {
> > + ap_bucket *eos = ap_bucket_create_eos();
> > +
> > + AP_BRIGADE_INSERT_TAIL(b, eos);
> > + }
> > + return APR_SUCCESS;
>
> Assuming ap_get_client_block works properly, this is 100% correct. This
> is the way I had originally hoped to do the EOS bucket on input, but I
> didn't want to do too much to ap_get_client_block while we were still
> discussing it. I am ++1 for this change.
>
> > +/*XXX hack to use mod_cgi[d] for testing chunked request bodies XXX */
> > + if (r->read_body == REQUEST_CHUNKED_ERROR) {
> > + r->read_body = REQUEST_CHUNKED_DECHUNK;
> > + }
> > +/*XXX end of this particular hack :) */
>
> I assume this will be gone in the real commit. :-)
yes, but only commented out at the moment... I'll zap it completely
momentarily.
> > r->read_chunked = 1;
> > + if (r->read_body == REQUEST_CHUNKED_PASS) {
> > + /* can't filter the body *and* preserve the chunk headers */
> > + r->input_filters = r->connection->input_filters;
> > + }
> > + ap_add_input_filter("DECHUNK", NULL, r, r->connection);
>
> This doesn't actually disable filtering. I'm not really sure of the
> correct way to disable filtering, but I have the feeling this will
> actually need to happen in ap_get_client_block, not
> ap_setup_client_block. The problem is that it is perfectly possible for a
> module to insert a filter between the two function calls.
> Hmmmm.....
no obvious solution...
> > + if (!r->filtered_input) {
> > + r->filtered_input = ap_brigade_create(r->pool);
> > }
>
> This should be done when creating the core_dir_config, not when reading
> the request IMO.
yes, that sounds much better... I'll get to that tonight or tomorrow.
> > + while (b->type != ap_eos_type() &&
> > + b->length == 0 && b != AP_BRIGADE_SENTINEL(bb)) {
> > + ap_bucket *e = b;
> > + b = AP_BUCKET_NEXT(e);
> > + AP_BUCKET_REMOVE(e);
> > + ap_bucket_destroy(e);
> > }
> > + } while (AP_BRIGADE_EMPTY(bb));
>
> Remove this loop. It just clutters up the code, and it doesn't actually
> do anything. If we have a brigade full of empty buckets the mainline code
> has to deal with that case anyway. The idea is that we could have an
> empty bucket anyplace in the brigade. If we encounter one while
> processing the data, then we have to just deal with it. Looping through a
> brigade for the case where we have a brigade full of empty buckets is not
> worth it IMHO. It makes the code harder to understand and doesn't really
> buy us anything. I removed these loops from a lot of the code this
> weekend.
this will be gone very soon
> > /* In HTTP/1.1, any method can have a body. However, most GET handlers
> >
> >
> > Off to catch some zzzs...
>
>
> Fix the stuff from my comments above, and please commit away. :-) Does
> this finish the filtering stuff once and for all, other than bug fixes? I
> think we are finally there!!!!!!
We're definitely getting close. Greg Ames and I need to look at the
EBCDIC issues, but I don't think that will change the shape of
anything.
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] dechunking, body filtering
Posted by rb...@covalent.net.
> > Jeff and I both looked at REQUEST_CHUNKED_PASS, and the whole point of it
> > is to throw away data. I personally believe it made sense in the BUFF
> > model, but it doesn't make any sense anymore. I think it should just go
> > away before our next alpha. If anybody REALLY needs it, we can always put
> > it back. Jeff says he searched on google, and I reviewed the code that
> > currently uses it in the core and the comments. I really don't think we
> > need to keep this behavior around.
>
> Ummm, not quite. The point was to be able to implement filtered
> request streams within modules that understood HTTP/1.1 and needed
> the chunks (and chunk-extensions) to perform things like message
> integrity checks. Since we can do that with filters in 2.0, we can
> remove that option IF we have a bucket data type for passing metadata
> downstream and a way to differentiate between completed EOS and
> aborted EOS. I haven't checked if today's incarnation of buckets
> has that functionality yet, but it will by the end of the hackathon.
I think that would be done best by creating a filter that sits between the
chunking and http filters. That filter would get to see the chunk header
and on the next call it would see the actual chunk.
Yes, I do believe this can go away now. :-)
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] dechunking, body filtering
Posted by "Roy T. Fielding" <fi...@ebuilt.com>.
On Mon, Oct 16, 2000 at 03:50:19PM -0700, rbb@covalent.net wrote:
>
> Jeff and I both looked at REQUEST_CHUNKED_PASS, and the whole point of it
> is to throw away data. I personally believe it made sense in the BUFF
> model, but it doesn't make any sense anymore. I think it should just go
> away before our next alpha. If anybody REALLY needs it, we can always put
> it back. Jeff says he searched on google, and I reviewed the code that
> currently uses it in the core and the comments. I really don't think we
> need to keep this behavior around.
Ummm, not quite. The point was to be able to implement filtered
request streams within modules that understood HTTP/1.1 and needed
the chunks (and chunk-extensions) to perform things like message
integrity checks. Since we can do that with filters in 2.0, we can
remove that option IF we have a bucket data type for passing metadata
downstream and a way to differentiate between completed EOS and
aborted EOS. I haven't checked if today's incarnation of buckets
has that functionality yet, but it will by the end of the hackathon.
....Roy
Re: [PATCH] dechunking, body filtering
Posted by rb...@covalent.net.
> > But this is a non-issue because of how it should be used. The only
> > problem I see is that brigade_bgets requires that the bb is received
> > before we call brigade_bgets. But this is solved by removing the call to
> > get_brigade.
>
> Then we are saying that the chunk filter needs to always be right above
> http_filter (since we are picking up c->input_filters in getline). That
> could well be true...I dunno. If there are other content filters
> between chunk and http_filter and they stash data away, we are hosed.
No, because we know that content filters shouldn't be taking any of the
chunk headers that we want. If we have chunk headers, then we aren't
grabbing whole blocks of data from lower level filters, we are just
grabbing one chunk at a time.
> It is easily solved by passing a bucket brigade as a parm to the
> function that's doing the real work, like brigade_bgets's bb parm. Then
> the getline() wrapper gets the brigade from c->input_filters.
But there is no reason to do all of that. no filter can have the chunk
header unless it is acting like http_filter. This is just the way input
filtering works.
> > Then the only remaining problem is that bgets can save the
> > buckets. But, that only happens when read_body is REQUEST_CHUNKED_PASS is
> > true, which only happens when we are throwing away the data. In this
> > case, I have no problem as all with re-wrapping the data in a new bucket,
> > because the browser was stupid to send us a body with a method that
> > doesn't use it.
>
> I thought REQUEST_CHUNKED_PASS was a module API thing, for modules that
> want to see chunk headers. I personally don't know _why_ a module
> should see chunk headers. But if this is an API that we need to
> maintain, it also isn't too hard to do. Just emulate Jeff's code in
> whatever the function is that's doing the real work. I really haven't
> investigated REQUEST_CHUNKED_PASS in any detail.
Jeff and I both looked at REQUEST_CHUNKED_PASS, and the whole point of it
is to throw away data. I personally believe it made sense in the BUFF
model, but it doesn't make any sense anymore. I think it should just go
away before our next alpha. If anybody REALLY needs it, we can always put
it back. Jeff says he searched on google, and I reviewed the code that
currently uses it in the core and the comments. I really don't think we
need to keep this behavior around.
> > > I'm doing minor tweaks to getline() at the moment for other reasons.
>
> The tweaks are committed & my dog probably needs to go outside, so feel
> free to beat me to it.
I'm fixing sub-requests right now, but I'll do this right after.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] dechunking, body filtering
Posted by Greg Ames <gr...@raleigh.ibm.com>.
rbb@covalent.net wrote:
>
> > > Why isn't this just getline?
> >
> > Hmmm, reusing code that gets pounded is a good idea. But it won't work
> > as-is because of minor differences in parms, function, and environment
> > (filter vs. non-filter).
>
> But this is a non-issue because of how it should be used. The only
> problem I see is that brigade_bgets requires that the bb is received
> before we call brigade_bgets. But this is solved by removing the call to
> get_brigade.
Then we are saying that the chunk filter needs to always be right above
http_filter (since we are picking up c->input_filters in getline). That
could well be true...I dunno. If there are other content filters
between chunk and http_filter and they stash data away, we are hosed.
It is easily solved by passing a bucket brigade as a parm to the
function that's doing the real work, like brigade_bgets's bb parm. Then
the getline() wrapper gets the brigade from c->input_filters.
> Then the only remaining problem is that bgets can save the
> buckets. But, that only happens when read_body is REQUEST_CHUNKED_PASS is
> true, which only happens when we are throwing away the data. In this
> case, I have no problem as all with re-wrapping the data in a new bucket,
> because the browser was stupid to send us a body with a method that
> doesn't use it.
I thought REQUEST_CHUNKED_PASS was a module API thing, for modules that
want to see chunk headers. I personally don't know _why_ a module
should see chunk headers. But if this is an API that we need to
maintain, it also isn't too hard to do. Just emulate Jeff's code in
whatever the function is that's doing the real work. I really haven't
investigated REQUEST_CHUNKED_PASS in any detail.
>
> > I'm doing minor tweaks to getline() at the moment for other reasons.
The tweaks are committed & my dog probably needs to go outside, so feel
free to beat me to it.
Greg
Re: [PATCH] dechunking, body filtering
Posted by rb...@covalent.net.
> > <brigade_bgets snipped for brevity>
> >
> > Why isn't this just getline? This does a bit less than getline, but
> > getline is already there, and it is one less place for things to go wrong.
> >
>
> Hmmm, reusing code that gets pounded is a good idea. But it won't work
> as-is because of minor differences in parms, function, and environment
> (filter vs. non-filter).
But this is a non-issue because of how it should be used. The only
problem I see is that brigade_bgets requires that the bb is received
before we call brigade_bgets. But this is solved by removing the call to
get_brigade. Then the only remaining problem is that bgets can save the
buckets. But, that only happens when read_body is REQUEST_CHUNKED_PASS is
true, which only happens when we are throwing away the data. In this
case, I have no problem as all with re-wrapping the data in a new bucket,
because the browser was stupid to send us a body with a method that
doesn't use it.
> I'm doing minor tweaks to getline() at the moment for other reasons.
> Once I get done with that, I'll see if we can factor out the common
> denominator & turn getline() into a wrapper.
I don't see the issue. I haven't tested this at all, but all I see as
being necessary is:
Index: http_protocol.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/main/http_protocol.c,v
retrieving revision 1.177
diff -u -d -b -w -r1.177 http_protocol.c
--- http_protocol.c 2000/10/16 20:08:14 1.177
+++ http_protocol.c 2000/10/16 21:27:56
@@ -903,11 +903,7 @@
*/
char line[30];
- if ((rv = ap_get_brigade(f->next, bb, AP_GET_LINE)) != APR_SUCCESS) {
- return rv;
- }
- if ((rv = brigade_bgets(bb, ctx->remove_chunk_proto,
- line, sizeof(line))) != APR_SUCCESS) {
+ if ((rv = getline(line, sizeof(line), f->c, 0)) != APR_SUCCESS) {
return rv;
}
switch(ctx->state) {
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] dechunking, body filtering
Posted by Greg Ames <gr...@raleigh.ibm.com>.
rbb@covalent.net wrote:
>
> > +static apr_status_t brigade_bgets(ap_bucket_brigade *bb,
> > + int remove_buckets,
> > + char *line,
> > + apr_ssize_t max_line_size)
>
> <brigade_bgets snipped for brevity>
>
> Why isn't this just getline? This does a bit less than getline, but
> getline is already there, and it is one less place for things to go wrong.
>
Hmmm, reusing code that gets pounded is a good idea. But it won't work
as-is because of minor differences in parms, function, and environment
(filter vs. non-filter).
I'm doing minor tweaks to getline() at the moment for other reasons.
Once I get done with that, I'll see if we can factor out the common
denominator & turn getline() into a wrapper.
Greg
Re: [PATCH] dechunking, body filtering
Posted by rb...@covalent.net.
Well, I promised to do a more thorough review a few days ago, but I
forgot, here it is.
> r
>
> Add "void *filtered_input" field; ap_get_client_block() holds onto
> leftover body in this field. Ugly that we clutter the r... Ugly that
> I made it "void *" instead of "ap_bucket_brigade *" (avoids an additional
> #include in "httpd.h" for (probably) no good reason).
Mentioned in another note, but repeating it here for completeness. This
belongs in core_dir_conf, not the request_rec. This is information only
for a core function, so it shouldn't be available to everything. Also,
this really should be a bucket_brigade *, but that's REALLY minor.
> remember that the read_policy parameter is copied to r->read_body;
>
> if read_policy is REQUEST_CHUNKED_PASS and the transport encoding is
> chunked...
>
> Disable filtering of the request body, because chunk headers would
> be invalid
> if the filter changed the length.
I'm not sure I like this, but I dislike the other options more. The
problem here, is that we are changing how a request is received based on
information in the filter. Then again, we only use REQUEST_CHUNKED_PASS
when we are discarding the whole body anyway. I have no good answer, but
the solution I hate more than disabling filtering, is for the chunk filter
to fix the chunk headers. The solution I like a bit more, is to remove
this option altogether, although that too is a bit bogus. Just some food
for thought on this.
> r->input_filters = r->connection->input_filters;
>
> dechunk_filter() will look at the read policy to know whether or not to leave the
> chunk headers.
>
> core's register_hooks()
>
> register the dechunk filter; play games with filter type so that it is
> added "properly"
BTW, I hate this, but that is a problem with our filter registration that
we need to fix, and it has nothing to do with this patch.
> X CORE_IN shouldn't deliver the whole request body if it is big... one I/O
> buffer worth of request body is plenty... otherwise we eat up virtual storage
Core in just passes the brigade up to http_filter, or whatever filter is
there. http_filter should use a length of MAX_STRING_LENGTH when doing a
read.
> Not-so-temporary hacks (i.e., I don't know how to fix it):
>
> There must be a nice way to avoid r->filtered_input (other than imposing
> length games on all input filters).
Not that I can see either. I hate both solutions almost as much as you
do. We disagree on the cleaner method, but I'll live with the
r->filtered_input, as long as the brigade lives in core_dir_conf, not
request_rec.
> Known bugs:
>
> X post 10000 bytes -- CGI sees 10002 bytes; the extra two are CR+LF at the end of
> the body; depends on whether or not I'm debugging it :(
That's kind of cute.
> +static apr_status_t brigade_bgets(ap_bucket_brigade *bb,
> + int remove_buckets,
> + char *line,
> + apr_ssize_t max_line_size)
<brigade_bgets snipped for brevity>
Why isn't this just getline? This does a bit less than getline, but
getline is already there, and it is one less place for things to go wrong.
> +struct dechunk_ctx {
> + apr_ssize_t chunk_size;
> + apr_ssize_t bytes_delivered;
> + enum {WANT_HDR /* must have value zero */, WANT_BODY, WANT_TRL} state;
> + int remove_chunk_proto;
> +};
> +
> +static long get_chunk_size(char *);
> +
> +apr_status_t dechunk_filter(ap_filter_t *f, ap_bucket_brigade *bb,
> + apr_ssize_t length)
> +{
> + apr_status_t rv;
> + struct dechunk_ctx *ctx = f->ctx;
> + ap_bucket *b;
> + const char *buf;
> + apr_ssize_t len;
> +
> + if (!ctx) {
> + f->ctx = ctx = apr_pcalloc(f->r->pool, sizeof(struct dechunk_ctx));
> + ctx->remove_chunk_proto = f->r->read_body != REQUEST_CHUNKED_PASS;
> + }
> +
> + do {
> + if (ctx->chunk_size == ctx->bytes_delivered) {
> + /* Time to read another chunk header or trailer... http_filter() is
> + * the next filter in line and it knows how to return a brigade with
> + * one line.
> + */
> + char line[30];
> +
> + ap_debug_assert(!strcmp(f->next->frec->name, "HTTP_IN"));
This is bogus. You can not assume that the next filter in the chain is
the http_filter. All you know, is that the next filter in the chain looks
like http_filter. Remove the assert, and I'll be perfectly happy though.
> + if ((rv = ap_get_brigade(f->next, bb, AP_GET_LINE)) != APR_SUCCESS) {
> + return rv;
> + }
> + if ((rv = brigade_bgets(bb, ctx->remove_chunk_proto,
> + line, sizeof(line))) != APR_SUCCESS) {
> + return rv;
> + }
This should just be a single call to getline IMO.
> apr_status_t http_filter(ap_filter_t *f, ap_bucket_brigade *b, apr_ssize_t length)
> {
> #define ASCII_LF '\012'
> @@ -889,32 +1026,35 @@
> }
> }
>
> - if (length > 0) {
> - int remain = length;
> + if (ctx->remaining > 0) {
> const char *ignore;
>
> e = AP_BRIGADE_FIRST(b);
> while (e != AP_BRIGADE_SENTINEL(b)) {
> ap_bucket_read(e, &ignore, &len, 0);
> - if (remain <= len) {
> + if (ctx->remaining < len) {
> break;
> }
> - remain -= len;
> + ctx->remaining -= len;
> e = AP_BUCKET_NEXT(e);
> }
> if (e != AP_BRIGADE_SENTINEL(b)) {
> - if (remain <= len) {
> - ap_bucket_split(e, remain);
> - remain = 0;
> + if (ctx->remaining < len) {
> + ap_bucket_split(e, ctx->remaining);
> + ctx->remaining = 0;
> }
> bb = ap_brigade_split(b, AP_BUCKET_NEXT(e));
> ctx->b = bb;
> - return APR_SUCCESS;
> }
> else {
> ctx->b = NULL;
> - return APR_SUCCESS;
> }
All of the changes above should be removed, because they are already
basically working.
> + if (ctx->remaining == 0) {
> + ap_bucket *eos = ap_bucket_create_eos();
> +
> + AP_BRIGADE_INSERT_TAIL(b, eos);
> + }
> + return APR_SUCCESS;
Assuming ap_get_client_block works properly, this is 100% correct. This
is the way I had originally hoped to do the EOS bucket on input, but I
didn't want to do too much to ap_get_client_block while we were still
discussing it. I am ++1 for this change.
> +/*XXX hack to use mod_cgi[d] for testing chunked request bodies XXX */
> + if (r->read_body == REQUEST_CHUNKED_ERROR) {
> + r->read_body = REQUEST_CHUNKED_DECHUNK;
> + }
> +/*XXX end of this particular hack :) */
I assume this will be gone in the real commit. :-)
> r->read_chunked = 1;
> + if (r->read_body == REQUEST_CHUNKED_PASS) {
> + /* can't filter the body *and* preserve the chunk headers */
> + r->input_filters = r->connection->input_filters;
> + }
> + ap_add_input_filter("DECHUNK", NULL, r, r->connection);
This doesn't actually disable filtering. I'm not really sure of the
correct way to disable filtering, but I have the feeling this will
actually need to happen in ap_get_client_block, not
ap_setup_client_block. The problem is that it is perfectly possible for a
module to insert a filter between the two function calls. Hmmmm.....
> + if (!r->filtered_input) {
> + r->filtered_input = ap_brigade_create(r->pool);
> }
This should be done when creating the core_dir_config, not when reading
the request IMO.
> + while (b->type != ap_eos_type() &&
> + b->length == 0 && b != AP_BRIGADE_SENTINEL(bb)) {
> + ap_bucket *e = b;
> + b = AP_BUCKET_NEXT(e);
> + AP_BUCKET_REMOVE(e);
> + ap_bucket_destroy(e);
> }
> + } while (AP_BRIGADE_EMPTY(bb));
Remove this loop. It just clutters up the code, and it doesn't actually
do anything. If we have a brigade full of empty buckets the mainline code
has to deal with that case anyway. The idea is that we could have an
empty bucket anyplace in the brigade. If we encounter one while
processing the data, then we have to just deal with it. Looping through a
brigade for the case where we have a brigade full of empty buckets is not
worth it IMHO. It makes the code harder to understand and doesn't really
buy us anything. I removed these loops from a lot of the code this
weekend.
> /* In HTTP/1.1, any method can have a body. However, most GET handlers
>
>
> Off to catch some zzzs...
Fix the stuff from my comments above, and please commit away. :-) Does
this finish the filtering stuff once and for all, other than bug fixes? I
think we are finally there!!!!!!
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------