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...@ebuilt.com> on 2001/09/23 13:12:03 UTC
[PATCH] Rework httpd buckets/filtering
I started out trying to get flood to support chunked encoding (as
recent changes to httpd have started to let chunked encoding work).
Then, I decided to look at how httpd-2.0 handles dechunking (as it
needs to be able to handle chunked input).
The current dechunk code can't handle trailers properly (does anybody
actually use them?) - according to my reading of RFC 2616, you can
have multiple trailers (Roy?). So, I believe that part is broken.
While I was examining the code, I ran into all of gstein's comments
in dechunk_filter. And, then in ap_http_filter. So, this is an
attempt to try to address some of the issues in http_protocol.c.
This also has some patches and new functions to apr-util's buckets
that seem like they are needed. Also, why is the socket (and pipe)
code creating 0-length immortal buckets? I don't understand why
and it serves to complicate things.
The only thing I can say with this is that it compiles and still
serves requests. But, I haven't put it through any extensive
testing (i.e. any POSTs). I'd like to go to bed now, so I'm
sending this out to the list so that people can review this and
give feedback/flames. I'll keep playing with this tomorrow...
The only question I have right now is whether we need anyone
to produce the EOS bucket on input. I'm not terribly sure of
that (seems Greg was thinking along these lines too). I know it
is needed in the output, but I wonder where the right place is
for the EOS on input. Thoughts? -- justin
Index: include/apr_buckets.h
===================================================================
RCS file: /home/cvs/apr-util/include/apr_buckets.h,v
retrieving revision 1.118
diff -u -r1.118 apr_buckets.h
--- include/apr_buckets.h 2001/09/22 22:36:07 1.118
+++ include/apr_buckets.h 2001/09/23 10:36:12
@@ -689,6 +689,17 @@
apr_off_t *length);
/**
+ * create a flat buffer of the elements in a bucket_brigade.
+ * @param b The bucket brigade to create the buffer from
+ * @param c The pointer to the returned character string
+ * @param len The length of the returned character string
+ * @param p The pool to allocate the character string from.
+ * Note: You *must* have enough space allocated to store all of the data.
+ * See apr_brigade_length().
+ */
+APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b, char *c);
+
+/**
* create an iovec of the elements in a bucket_brigade... return number
* of elements used. This is useful for writing to a file or to the
* network efficiently.
Index: buckets/apr_brigade.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
retrieving revision 1.25
diff -u -r1.25 apr_brigade.c
--- buckets/apr_brigade.c 2001/09/03 22:00:36 1.25
+++ buckets/apr_brigade.c 2001/09/23 10:36:12
@@ -221,6 +221,29 @@
return APR_SUCCESS;
}
+APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b, char *c)
+{
+ apr_bucket *e;
+ char *current;
+ apr_size_t curlen;
+ apr_status_t status;
+
+ current = c;
+
+ APR_BRIGADE_FOREACH(e, b) {
+
+ status = apr_bucket_read(e, (const char **)¤t, &curlen,
+ APR_BLOCK_READ);
+ if (status != APR_SUCCESS)
+ return status;
+
+ current += curlen;
+ }
+
+ return APR_SUCCESS;
+
+}
+
APU_DECLARE(apr_status_t) apr_brigade_to_iovec(apr_bucket_brigade *b,
struct iovec *vec, int *nvec)
{
Index: buckets/apr_buckets_pipe.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_buckets_pipe.c,v
retrieving revision 1.41
diff -u -r1.41 apr_buckets_pipe.c
--- buckets/apr_buckets_pipe.c 2001/09/22 22:36:07 1.41
+++ buckets/apr_buckets_pipe.c 2001/09/23 10:36:13
@@ -106,15 +106,8 @@
}
else {
free(buf);
- a = apr_bucket_immortal_make(a, "", 0);
- *str = a->data;
- if (rv == APR_EOF) {
+ if (rv == APR_EOF)
apr_file_close(p);
- if (block == APR_NONBLOCK_READ) {
- /* XXX: this is bogus, should return APR_SUCCESS */
- return APR_EOF;
- }
- }
}
return APR_SUCCESS;
}
Index: buckets/apr_buckets_socket.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_buckets_socket.c,v
retrieving revision 1.30
diff -u -r1.30 apr_buckets_socket.c
--- buckets/apr_buckets_socket.c 2001/09/22 22:36:07 1.30
+++ buckets/apr_buckets_socket.c 2001/09/23 10:36:13
@@ -79,7 +79,7 @@
}
if (rv != APR_SUCCESS && rv != APR_EOF) {
- free(buf);
+ free(buf);
return rv;
}
/*
@@ -109,12 +109,6 @@
}
else {
free(buf);
- a = apr_bucket_immortal_make(a, "", 0);
- *str = a->data;
- if (rv == APR_EOF && block == APR_NONBLOCK_READ) {
- /* XXX: this is bogus... should return APR_SUCCESS */
- return APR_EOF;
- }
}
return APR_SUCCESS;
}
Index: modules/http/http_protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.362
diff -u -r1.362 http_protocol.c
--- modules/http/http_protocol.c 2001/09/17 13:12:37 1.362
+++ modules/http/http_protocol.c 2001/09/23 10:56:30
@@ -487,6 +487,7 @@
enum {
WANT_HDR /* must have value zero */,
WANT_BODY,
+ WANT_ENDCHUNK,
WANT_TRL
} state;
};
@@ -498,9 +499,7 @@
{
apr_status_t rv;
struct dechunk_ctx *ctx = f->ctx;
- apr_bucket *b;
- const char *buf;
- apr_size_t len;
+ apr_off_t len;
if (!ctx) {
f->ctx = ctx = apr_pcalloc(f->r->pool, sizeof(struct dechunk_ctx));
@@ -508,43 +507,38 @@
do {
if (ctx->chunk_size == ctx->bytes_delivered) {
- /* Time to read another chunk header or trailer... ap_http_filter() is
- * the next filter in line and it knows how to return a brigade with
- * one line.
- */
+ /* Time to read another chunk header or trailer... We only want
+ * one line - by specifying 0 as the length to read. */
char line[30];
- if ((rv = ap_getline(line, sizeof(line), f->r,
- 0 /* readline */)) < 0) {
+ if ((rv = ap_getline(line, sizeof(line), f->r, 0)) < 0) {
return rv;
}
switch(ctx->state) {
case WANT_HDR:
ctx->chunk_size = get_chunk_size(line);
ctx->bytes_delivered = 0;
- if (ctx->chunk_size == 0) {
+ if (ctx->chunk_size == 0)
ctx->state = WANT_TRL;
- }
- else {
+ else
ctx->state = WANT_BODY;
- }
break;
+ case WANT_ENDCHUNK:
+ /* We don't care. Next state please. */
+ ctx->state = WANT_TRL;
+ 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? */
- /* ### woah... ap_http_filter() is doing this, too */
- /* append eos bucket and get out */
- b = apr_bucket_eos_create();
- APR_BRIGADE_INSERT_TAIL(bb, b);
+ /* Keep reading until we get to a blank line. */
+ /* Should add these headers to r->headers_in? */
+ if (!strlen(line))
+ {
+ ctx->state = WANT_HDR;
return APR_SUCCESS;
}
- ctx->state = WANT_HDR;
- break;
+ break;
default:
- ap_assert(ctx->state == WANT_HDR || ctx->state == WANT_TRL);
+ ap_assert(ctx->state == WANT_HDR || ctx->state == WANT_TRL ||
+ ctx->state == WANT_ENDCHUNK);
}
}
} while (ctx->state != WANT_BODY);
@@ -558,27 +552,11 @@
return rv;
}
- /* Walk through the body, accounting for bytes, and removing an eos
- * bucket if ap_http_filter() delivered the entire chunk.
- *
- * ### this shouldn't be necessary. 1) ap_http_filter shouldn't be
- * ### adding EOS buckets. 2) it shouldn't return more bytes than
- * ### we requested, therefore the total len can be found with a
- * ### simple call to apr_brigade_length(). no further munging
- * ### would be needed.
- */
- b = APR_BRIGADE_FIRST(bb);
- while (b != APR_BRIGADE_SENTINEL(bb) && !APR_BUCKET_IS_EOS(b)) {
- apr_bucket_read(b, &buf, &len, mode);
- AP_DEBUG_ASSERT(len <= ctx->chunk_size - ctx->bytes_delivered);
- ctx->bytes_delivered += len;
- b = APR_BUCKET_NEXT(b);
- }
- if (ctx->bytes_delivered == ctx->chunk_size) {
- AP_DEBUG_ASSERT(APR_BUCKET_IS_EOS(b));
- apr_bucket_delete(b);
- ctx->state = WANT_TRL;
- }
+ /* Have we read a complete chunk? */
+ apr_brigade_length(bb, 1, &len);
+ ctx->bytes_delivered += len;
+ if (ctx->bytes_delivered == ctx->chunk_size)
+ ctx->state = WANT_ENDCHUNK;
}
return APR_SUCCESS;
@@ -588,7 +566,8 @@
apr_bucket_brigade *b;
} http_ctx_t;
-apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_off_t *readbytes)
+apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
+ ap_input_mode_t mode, apr_off_t *readbytes)
{
apr_bucket *e;
char *buff;
@@ -641,7 +620,8 @@
}
if (APR_BRIGADE_EMPTY(ctx->b)) {
- if ((rv = ap_get_brigade(f->next, ctx->b, mode, readbytes)) != APR_SUCCESS) {
+ if ((rv = ap_get_brigade(f->next, ctx->b, mode,
+ readbytes)) != APR_SUCCESS) {
return rv;
}
}
@@ -655,12 +635,16 @@
if (*readbytes == -1) {
apr_bucket *e;
apr_off_t total;
+ const char *str;
+ apr_size_t len;
APR_BRIGADE_FOREACH(e, ctx->b) {
- const char *str;
- apr_size_t len;
+ /* We don't care about these values. We just want to force the
+ * lower level to convert it. This seems hackish. */
apr_bucket_read(e, &str, &len, APR_BLOCK_READ);
}
APR_BRIGADE_CONCAT(b, ctx->b);
+
+ /* Force a recompute of the length */
apr_brigade_length(b, 1, &total);
*readbytes = total;
e = apr_bucket_eos_create();
@@ -670,67 +654,40 @@
/* readbytes == 0 is "read a single line". otherwise, read a block. */
if (*readbytes) {
apr_off_t total;
+ apr_bucket *e;
+ apr_bucket_brigade *newbb;
- /* ### the code below, which moves bytes from one brigade to the
- ### other is probably bogus. presuming the next filter down was
- ### working properly, it should not have returned more than
- ### READBYTES bytes, and we wouldn't have to do any work.
- */
-
- APR_BRIGADE_NORMALIZE(ctx->b);
- if (APR_BRIGADE_EMPTY(ctx->b)) {
- if ((rv = ap_get_brigade(f->next, ctx->b, mode, readbytes)) != APR_SUCCESS) {
- return rv;
- }
- }
-
+ newbb = NULL;
+
apr_brigade_partition(ctx->b, *readbytes, &e);
- APR_BRIGADE_CONCAT(b, ctx->b);
+ /* Must do split before CONCAT */
if (e != APR_BRIGADE_SENTINEL(ctx->b)) {
- apr_bucket_brigade *temp;
+ newbb = apr_brigade_split(ctx->b, e);
+ }
+ APR_BRIGADE_CONCAT(b, ctx->b);
- temp = apr_brigade_split(b, e);
+ /* FIXME: Is this really needed? Due to pointer use in sentinels,
+ * I think so. */
+ if (newbb)
+ APR_BRIGADE_CONCAT(ctx->b, newbb);
- /* ### darn. gotta ensure the split brigade is in the proper pool.
- ### this is a band-aid solution; we shouldn't even be doing
- ### all of this brigade munging (per the comment above).
- ### until then, this will get the right lifetimes. */
- APR_BRIGADE_CONCAT(ctx->b, temp);
- }
- else {
- if (!APR_BRIGADE_EMPTY(ctx->b)) {
- ctx->b = NULL; /*XXX*/
- }
- }
+ /* FIXME: Are we assuming that b is empty when we enter? */
apr_brigade_length(b, 1, &total);
- *readbytes -= total;
+ *readbytes = total;
- /* ### this is a hack. it is saying, "if we have read everything
- ### that was requested, then we are at the end of the request."
- ### it presumes that the next filter up will *only* call us
- ### with readbytes set to the Content-Length of the request.
- ### that may not always be true, and this code is *definitely*
- ### too presumptive of the caller's intent. the point is: this
- ### filter is not the guy that is parsing the headers or the
- ### chunks to determine where the end of the request is, so we
- ### shouldn't be monkeying with EOS buckets.
- */
- if (*readbytes == 0) {
- apr_bucket *eos = apr_bucket_eos_create();
-
- APR_BRIGADE_INSERT_TAIL(b, eos);
- }
return APR_SUCCESS;
}
/* we are reading a single line, e.g. the HTTP headers */
while (!APR_BRIGADE_EMPTY(ctx->b)) {
e = APR_BRIGADE_FIRST(ctx->b);
- if ((rv = apr_bucket_read(e, (const char **)&buff, &len, mode)) != APR_SUCCESS) {
+ if ((rv = apr_bucket_read(e, (const char **)&buff, &len,
+ mode)) != APR_SUCCESS) {
return rv;
}
pos = memchr(buff, APR_ASCII_LF, len);
+ /* We found a match. */
if (pos != NULL) {
apr_bucket_split(e, pos - buff + 1);
APR_BUCKET_REMOVE(e);
@@ -740,6 +697,7 @@
APR_BUCKET_REMOVE(e);
APR_BRIGADE_INSERT_TAIL(b, e);
}
+
return APR_SUCCESS;
}
@@ -1517,14 +1475,12 @@
*/
AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer, apr_size_t bufsiz)
{
- apr_size_t len_read, total;
- apr_status_t rv;
- apr_bucket *b, *old;
- const char *tempbuf;
+ apr_off_t len;
+ apr_bucket *b;
core_request_config *req_cfg =
(core_request_config *)ap_get_module_config(r->request_config,
&core_module);
- apr_bucket_brigade *bb = req_cfg->bb;
+ apr_bucket_brigade *bb = req_cfg->bb, *newbb;
do {
if (APR_BRIGADE_EMPTY(bb)) {
@@ -1546,41 +1502,32 @@
return 0;
}
- /* ### it would be nice to replace the code below with "consume N bytes
- ### from this brigade, placing them into that buffer." there are
- ### other places where we do the same...
- ###
- ### alternatively, we could partition the brigade, then call a
- ### function which serializes a given brigade into a buffer. that
- ### semantic is used elsewhere, too...
- */
-
- total = 0;
- while (total < bufsiz
- && b != APR_BRIGADE_SENTINEL(bb)
- && !APR_BUCKET_IS_EOS(b)) {
-
- if ((rv = apr_bucket_read(b, &tempbuf, &len_read, APR_BLOCK_READ)) != APR_SUCCESS) {
- return -1;
- }
- if (total + len_read > bufsiz) {
- apr_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; gotta see if anybody else actually uses
- * these
- */
- r->read_length += len_read; /* XXX yank me? */
- old = b;
- b = APR_BUCKET_NEXT(b);
- apr_bucket_delete(old);
- }
+ /* 1) Determine length to see if we may overrun.
+ * 2) Partition the brigade at the appropriate point.
+ * 3) Split the brigade at the new partition.
+ * 4) Read the old brigade into the buffer.
+ * 5) Destroy the old brigade.
+ * 6) Point the context at the new brigade.
+ */
+ apr_brigade_length(bb, 1, &len);
+
+ if (bufsiz < len)
+ len = bufsiz;
+
+ if (apr_brigade_partition(bb, len, &b) != APR_SUCCESS)
+ return -1;
+
+ newbb = apr_brigade_split(bb, b);
+
+ if (apr_brigade_to_buffer(bb, buffer) != APR_SUCCESS)
+ return -1;
+
+ if (apr_brigade_destroy(bb) != APR_SUCCESS)
+ return -1;
+
+ req_cfg->bb = newbb;
- return total;
+ return bufsiz;
}
/* In HTTP/1.1, any method can have a body. However, most GET handlers
Re: [PATCH] Rework httpd buckets/filtering
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Sun, Sep 23, 2001 at 02:53:14PM -0700, Greg Stein wrote:
> Euh... not sure if you typoed. Input filters should *NOT* return more than
> they were asked for. Never.
Yup. That's currently what can happen.
> >...
> > > --- include/apr_buckets.h 2001/09/22 22:36:07 1.118
> > > +++ include/apr_buckets.h 2001/09/23 10:36:12
> > > @@ -689,6 +689,17 @@
> > > apr_off_t *length);
> > >
> > > /**
> > > + * create a flat buffer of the elements in a bucket_brigade.
> > > + * @param b The bucket brigade to create the buffer from
> > > + * @param c The pointer to the returned character string
> > > + * @param len The length of the returned character string
> > > + * @param p The pool to allocate the character string from.
> > > + * Note: You *must* have enough space allocated to store all of the data.
> > > + * See apr_brigade_length().
> > > + */
> > > +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b,
> > > char *c); +
>
> Ryan said this should take a reading mode. I don't understand why that
> should be necessary. You have a brigade of a defined size, and you're
> flattening that much data. You're already past the point of non-blocking
> reads being used.
>
> If you had a partial read, then how would you communiate that? And don't say
> strlen(c).
>
> No... the function does not need a mode, nor an output length. You give it a
> brigade and it flattens it in its entirety. If there is a problem getting
> the whole thing flattened, then it returns an error.
>
> Personally, I would pass in a length of the output buffer so the function
> can verify that it is flattening the proper amount -- not too little, and
> not overwriting.
>
> >...
> > > + case WANT_ENDCHUNK:
> > > + /* We don't care. Next state please. */
> > > + ctx->state = WANT_TRL;
> > > + 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? */
> > > - /* ### woah... ap_http_filter() is doing this, too */
> > > - /* append eos bucket and get out */
> > > - b = apr_bucket_eos_create();
> > > - APR_BRIGADE_INSERT_TAIL(bb, b);
>
> DECHUNK is the guy to insert the EOS. The HTTP filter cannot know how much
> data is in the request (if you're chunking, the C-L header is ignored). The
> above code should probably stay, and the HTTP filter should stop inserting
> EOS.
>
> But then again, if you aren't chunking, then the HTTP filter *does* define
> the end of the request. Fun, huh? That is why we discussed combining the two
> filters into one. The state stuff is completely wiped out, and we can easily
> determine EOS, simplifying everything greatly.
>
> Consider that when somebody asks the (new) HTTP filter for input. It can
> zoom through the headers in direct procedural code. Then it gets to the
> body, start dechunking if necessary, and places the proper amount of read
> data into the passed-brigade.
Damn, you're good (we knew that already). I just figured that out over
here. =-)
>
> >...
> > > - /* ### the code below, which moves bytes from one brigade to the
> > > - ### other is probably bogus. presuming the next filter down was
> > > - ### working properly, it should not have returned more than
> > > - ### READBYTES bytes, and we wouldn't have to do any work.
> > > - */
>
> You have to fix the lower code before tossing this stuff.
>
> >...
> > > - /* ### this is a hack. it is saying, "if we have read everything
> > > - ### that was requested, then we are at the end of the request."
> > > - ### it presumes that the next filter up will *only* call us
> > > - ### with readbytes set to the Content-Length of the request.
> > > - ### that may not always be true, and this code is *definitely*
> > > - ### too presumptive of the caller's intent. the point is: this
> > > - ### filter is not the guy that is parsing the headers or the
> > > - ### chunks to determine where the end of the request is, so we
> > > - ### shouldn't be monkeying with EOS buckets.
> > > - */
> > > - if (*readbytes == 0) {
> > > - apr_bucket *eos = apr_bucket_eos_create();
> > > -
> > > - APR_BRIGADE_INSERT_TAIL(b, eos);
> > > - }
>
> Somebody has to return an EOS somewhere.
>
> Consider the highest-level input filter. How does it know when it has read
> all of the data for the current request? It needs to see an EOS bucket.
>
>
> I'll wait for the new patch and review again. All the formatting changes
> were getting in the way.
I've got it right now that it passes the apache tests in httpd-test,
but not the filter/input tests. This is where I hit the EOS that
you described - we need to join the two filters again.
> But I really think the proper tack is to fix the amount-returned, and to
> combine the HTTP_IN and DECHUNK filters.
Since I know I'm going in the right direction, I'll head towards doing
this.
Expect a patch later tonight. -- justin
Re: [PATCH] Rework httpd buckets/filtering
Posted by Ryan Bloom <rb...@covalent.net>.
On Sunday 23 September 2001 03:28 pm, Justin Erenkrantz wrote:
> On Sun, Sep 23, 2001 at 03:21:38PM -0700, Ryan Bloom wrote:
> > What we are doing now, is forcing our clean bucket API into an API
> > designed around the BUFF logic.
>
> Which is exactly what ap_get_client_block does and where this function
> is used. =-)
>
> If we'd like to deprecate ap_get_client_block... -- justin
That's what I'm suggesting.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] Rework httpd buckets/filtering
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Sun, Sep 23, 2001 at 03:21:38PM -0700, Ryan Bloom wrote:
> What we are doing now, is forcing our clean bucket API into an API designed
> around the BUFF logic.
Which is exactly what ap_get_client_block does and where this function
is used. =-)
If we'd like to deprecate ap_get_client_block... -- justin
Re: [PATCH] Rework httpd buckets/filtering
Posted by Ryan Bloom <rb...@covalent.net>.
On Sunday 23 September 2001 03:12 pm, Justin Erenkrantz wrote:
> On Sun, Sep 23, 2001 at 03:04:54PM -0700, Ryan Bloom wrote:
> > That would be true if this function were to go into httpd, but it isn't.
> > It is going into APR-util, and there is nothing saying that you will have
> > a brigade of a defined size.
>
> The doc for my proposed function (ap_brigade_to_buffer) says so. =-)
> Otherwise, this function is useless - you have to have a defined
> size in order to copy it to a flat buffer.
>
> I used apr_brigade_partition to ensure that I had a defined length.
> If the partition failed, then it would have refused to copy.
Okay. I still think it is bogus to have a function that takes an arbitrary brigade,
and tries to flatten it into a string, and does so by always calling
APR_READ_BLOCK. Actually, I think the whole function is bogus when I
really think about it. We have already copied the data once, when we read
it from the socket. We should really fix the ap_get_client_block API to either
return a bucket brigade, or it should return a pointer to a string.
What we are doing now, is forcing our clean bucket API into an API designed
around the BUFF logic.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] Rework httpd buckets/filtering
Posted by Greg Stein <gs...@lyra.org>.
On Sun, Sep 23, 2001 at 03:04:54PM -0700, Ryan Bloom wrote:
> On Sunday 23 September 2001 02:53 pm, Greg Stein wrote:
> > On Sun, Sep 23, 2001 at 08:33:07AM -0700, Ryan Bloom wrote:
> > > None of this can be committed until it has passed all of the tests in the
> > > perl test framework. The last time somebody (me) mucked with these
> > > filters it broke all of the CGI script handling. I would also like to
> > > hear from Jeff about this, because when we wrote the filters originally,
> > > we had input filtering use this design, and Jeff and Greg were adamant
> > > that filters needed to be able to return more than they were asked for.
> >
> > Euh... not sure if you typoed. Input filters should *NOT* return more than
> > they were asked for. Never.
> >
> > If a filter did that, it could end up passing data up to a filter that is
> > not responsible for that data. For example, reading past the end of a
> > request, or reading past the headers yet that portion of the body was
> > supposed to go into a newly-inserted unzip or dechunk filter.
> >
> > I can think of more examples, if called upon :-)
>
> And I am saying, that was the original design, and Jeff Trawick and Greg
> Ames
Ah. An unqualified "Greg" in your post :-) Shouldn't he be OtherGreg,
following in OtherBill's footsteps? hehe...
> posted a couple of examples that proved that design incorrect.
The typical example that I recall was related to decompression filters.
"If my caller asks for 100 bytes, then I read 100 bytes from the lower
level, and decompress to 150 bytes, then my caller is going to receive 150
bytes."
The correct answer is "return the 100 bytes they asked for, and set the
other 50 aside for the next invocation."
Consider the case where you have a multipart message coming in, and the
entire message is compressed (a Transfer-Encoding is used for the message;
Content-Encoding is used to compress the entities in the multipart). Next,
let us say that each multipart is handled by a different filter (say,
because they are handling different Content-Encodings, such as base64 or
quoted-printable).
To apply some symbology here: M is the message-body filter which is
decompressing the message according to the Transfer-Encoding header. E1 and
E2 are the entity-related filters, handling each entity in the multipart
message.
Now, let's say that E1 asks for 100 bytes. M returns 150. But E1 *only*
wanted 100 bytes. Those other fifty were supposed to go to E2.
Oops :-)
Instead, M should decompress the incoming data, return 100 (to E1) and put
the other 50 aside for the next filter invocation. E2 comes along and asks
for 300 bytes. We return the 50 we had set aside.
(M might decide to read more data, but it is also allowed to return less
than asked for, assuming the caller will simply ask again for more)
If Jeff/OtherGreg :-) have an example where returning more than asked is
*required*, then it should be brought up again. I can't imagine a case where
it would ever be safe to return more than is desired.
[ other examples may relate to character decoding, where you want to avoid
returning a partial multibyte character, so you return N+3 to get the rest
of the character in the response; I say that you return N-1 and put rest
of the character aside for the next invocation ]
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] Rework httpd buckets/filtering
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Sun, Sep 23, 2001 at 03:04:54PM -0700, Ryan Bloom wrote:
> That would be true if this function were to go into httpd, but it isn't. It is
> going into APR-util, and there is nothing saying that you will have a
> brigade of a defined size.
The doc for my proposed function (ap_brigade_to_buffer) says so. =-)
Otherwise, this function is useless - you have to have a defined
size in order to copy it to a flat buffer.
I used apr_brigade_partition to ensure that I had a defined length.
If the partition failed, then it would have refused to copy.
-- justin
Re: [PATCH] Rework httpd buckets/filtering
Posted by Ryan Bloom <rb...@covalent.net>.
On Sunday 23 September 2001 08:35 pm, Cliff Woolley wrote:
> On Sun, 23 Sep 2001, Greg Stein wrote:
> > > > +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade
> > > > *b, char *c); +
> >
> > Ryan said this should take a reading mode. I don't understand why that
> > should be necessary. You have a brigade of a defined size, and you're
> > flattening that much data. You're already past the point of non-blocking
> > reads being used.
>
> FWIW, I agree with Greg on this point. I can't see any point to allowing
> nonblocking mode, just as there's no point to allowing nonblocking mode in
> apr_brigade_partition or apr_brigade_length. What good would nonblocking
> mode be? If you really want the brigade flattened out that bad, then you
> REALLY want it flattened... you don't want half of it flattened.
>
> But more importantly, you have to REALLY want it flat in the first place.
> IMO adding a function like this encourages lazy coders to abuse it so that
> they don't have to deal with things spanning buckets. If mod_include just
> flattened the brigade it worked on it would be a much simpler module.
> But it would suck for performance and break our whole zero-copy mantra.
> I'm -0 for adding a function like this to the buckets API.
I'll see your -0, and raise you to a -0.5. :-)
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] Rework httpd buckets/filtering
Posted by Cliff Woolley <cl...@yahoo.com>.
On Sun, 23 Sep 2001, Greg Stein wrote:
> > > +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b,
> > > char *c); +
>
> Ryan said this should take a reading mode. I don't understand why that
> should be necessary. You have a brigade of a defined size, and you're
> flattening that much data. You're already past the point of non-blocking
> reads being used.
FWIW, I agree with Greg on this point. I can't see any point to allowing
nonblocking mode, just as there's no point to allowing nonblocking mode in
apr_brigade_partition or apr_brigade_length. What good would nonblocking
mode be? If you really want the brigade flattened out that bad, then you
REALLY want it flattened... you don't want half of it flattened.
But more importantly, you have to REALLY want it flat in the first place.
IMO adding a function like this encourages lazy coders to abuse it so that
they don't have to deal with things spanning buckets. If mod_include just
flattened the brigade it worked on it would be a much simpler module.
But it would suck for performance and break our whole zero-copy mantra.
I'm -0 for adding a function like this to the buckets API.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: [PATCH] Rework httpd buckets/filtering
Posted by Ryan Bloom <rb...@covalent.net>.
On Sunday 23 September 2001 02:53 pm, Greg Stein wrote:
> On Sun, Sep 23, 2001 at 08:33:07AM -0700, Ryan Bloom wrote:
> > None of this can be committed until it has passed all of the tests in the
> > perl test framework. The last time somebody (me) mucked with these
> > filters it broke all of the CGI script handling. I would also like to
> > hear from Jeff about this, because when we wrote the filters originally,
> > we had input filtering use this design, and Jeff and Greg were adamant
> > that filters needed to be able to return more than they were asked for.
>
> Euh... not sure if you typoed. Input filters should *NOT* return more than
> they were asked for. Never.
>
> If a filter did that, it could end up passing data up to a filter that is
> not responsible for that data. For example, reading past the end of a
> request, or reading past the headers yet that portion of the body was
> supposed to go into a newly-inserted unzip or dechunk filter.
>
> I can think of more examples, if called upon :-)
And I am saying, that was the original design, and Jeff Trawick and Greg
Ames posted a couple of examples that proved that design incorrect. I
can't remember them right now, and I don't have time to review all of the
archives, but the original design had input filters only ever returning the
amount of data requested, and it broke their modules. The way the current
design makes sure that we don't read past the end of the request, is by
having a single filter that kept track of where the end of the request is.
> The current input filter code *does* return too much, though, and that is
> one of its problems. I'm not sure that this change improves upon that (need
> to review some more, me thinks).
>
> A filter can return *less* than what was asked for, but never more.
>
> >...
> >
> > > --- include/apr_buckets.h 2001/09/22 22:36:07 1.118
> > > +++ include/apr_buckets.h 2001/09/23 10:36:12
> > > @@ -689,6 +689,17 @@
> > > apr_off_t *length);
> > >
> > > /**
> > > + * create a flat buffer of the elements in a bucket_brigade.
> > > + * @param b The bucket brigade to create the buffer from
> > > + * @param c The pointer to the returned character string
> > > + * @param len The length of the returned character string
> > > + * @param p The pool to allocate the character string from.
> > > + * Note: You *must* have enough space allocated to store all of the
> > > data. + * See apr_brigade_length().
> > > + */
> > > +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b,
> > > char *c); +
>
> Ryan said this should take a reading mode. I don't understand why that
> should be necessary. You have a brigade of a defined size, and you're
> flattening that much data. You're already past the point of non-blocking
> reads being used.
>
> If you had a partial read, then how would you communiate that? And don't
> say strlen(c).
>
> No... the function does not need a mode, nor an output length. You give it
> a brigade and it flattens it in its entirety. If there is a problem getting
> the whole thing flattened, then it returns an error.
>
> Personally, I would pass in a length of the output buffer so the function
> can verify that it is flattening the proper amount -- not too little, and
> not overwriting.
That would be true if this function were to go into httpd, but it isn't. It is
going into APR-util, and there is nothing saying that you will have a
brigade of a defined size.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] Rework httpd buckets/filtering
Posted by Greg Stein <gs...@lyra.org>.
On Sun, Sep 23, 2001 at 08:33:07AM -0700, Ryan Bloom wrote:
>
> None of this can be committed until it has passed all of the tests in the perl test
> framework. The last time somebody (me) mucked with these filters it broke all of the
> CGI script handling. I would also like to hear from Jeff about this, because when we
> wrote the filters originally, we had input filtering use this design, and Jeff and Greg
> were adamant that filters needed to be able to return more than they were asked for.
Euh... not sure if you typoed. Input filters should *NOT* return more than
they were asked for. Never.
If a filter did that, it could end up passing data up to a filter that is
not responsible for that data. For example, reading past the end of a
request, or reading past the headers yet that portion of the body was
supposed to go into a newly-inserted unzip or dechunk filter.
I can think of more examples, if called upon :-)
The current input filter code *does* return too much, though, and that is
one of its problems. I'm not sure that this change improves upon that (need
to review some more, me thinks).
A filter can return *less* than what was asked for, but never more.
>...
> > --- include/apr_buckets.h 2001/09/22 22:36:07 1.118
> > +++ include/apr_buckets.h 2001/09/23 10:36:12
> > @@ -689,6 +689,17 @@
> > apr_off_t *length);
> >
> > /**
> > + * create a flat buffer of the elements in a bucket_brigade.
> > + * @param b The bucket brigade to create the buffer from
> > + * @param c The pointer to the returned character string
> > + * @param len The length of the returned character string
> > + * @param p The pool to allocate the character string from.
> > + * Note: You *must* have enough space allocated to store all of the data.
> > + * See apr_brigade_length().
> > + */
> > +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b,
> > char *c); +
Ryan said this should take a reading mode. I don't understand why that
should be necessary. You have a brigade of a defined size, and you're
flattening that much data. You're already past the point of non-blocking
reads being used.
If you had a partial read, then how would you communiate that? And don't say
strlen(c).
No... the function does not need a mode, nor an output length. You give it a
brigade and it flattens it in its entirety. If there is a problem getting
the whole thing flattened, then it returns an error.
Personally, I would pass in a length of the output buffer so the function
can verify that it is flattening the proper amount -- not too little, and
not overwriting.
>...
> > + case WANT_ENDCHUNK:
> > + /* We don't care. Next state please. */
> > + ctx->state = WANT_TRL;
> > + 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? */
> > - /* ### woah... ap_http_filter() is doing this, too */
> > - /* append eos bucket and get out */
> > - b = apr_bucket_eos_create();
> > - APR_BRIGADE_INSERT_TAIL(bb, b);
DECHUNK is the guy to insert the EOS. The HTTP filter cannot know how much
data is in the request (if you're chunking, the C-L header is ignored). The
above code should probably stay, and the HTTP filter should stop inserting
EOS.
But then again, if you aren't chunking, then the HTTP filter *does* define
the end of the request. Fun, huh? That is why we discussed combining the two
filters into one. The state stuff is completely wiped out, and we can easily
determine EOS, simplifying everything greatly.
Consider that when somebody asks the (new) HTTP filter for input. It can
zoom through the headers in direct procedural code. Then it gets to the
body, start dechunking if necessary, and places the proper amount of read
data into the passed-brigade.
>...
> > - /* ### the code below, which moves bytes from one brigade to the
> > - ### other is probably bogus. presuming the next filter down was
> > - ### working properly, it should not have returned more than
> > - ### READBYTES bytes, and we wouldn't have to do any work.
> > - */
You have to fix the lower code before tossing this stuff.
>...
> > - /* ### this is a hack. it is saying, "if we have read everything
> > - ### that was requested, then we are at the end of the request."
> > - ### it presumes that the next filter up will *only* call us
> > - ### with readbytes set to the Content-Length of the request.
> > - ### that may not always be true, and this code is *definitely*
> > - ### too presumptive of the caller's intent. the point is: this
> > - ### filter is not the guy that is parsing the headers or the
> > - ### chunks to determine where the end of the request is, so we
> > - ### shouldn't be monkeying with EOS buckets.
> > - */
> > - if (*readbytes == 0) {
> > - apr_bucket *eos = apr_bucket_eos_create();
> > -
> > - APR_BRIGADE_INSERT_TAIL(b, eos);
> > - }
Somebody has to return an EOS somewhere.
Consider the highest-level input filter. How does it know when it has read
all of the data for the current request? It needs to see an EOS bucket.
I'll wait for the new patch and review again. All the formatting changes
were getting in the way.
But I really think the proper tack is to fix the amount-returned, and to
combine the HTTP_IN and DECHUNK filters.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] Rework httpd buckets/filtering
Posted by Ryan Bloom <rb...@covalent.net>.
On Sunday 23 September 2001 09:48 am, Justin Erenkrantz wrote:
> > > Index: buckets/apr_brigade.c
> > > ===================================================================
> > > RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
> > > retrieving revision 1.25
> > > diff -u -r1.25 apr_brigade.c
> > > --- buckets/apr_brigade.c 2001/09/03 22:00:36 1.25
> > > +++ buckets/apr_brigade.c 2001/09/23 10:36:12
> > > @@ -221,6 +221,29 @@
> > > return APR_SUCCESS;
> > > }
> > >
> > > +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b,
> > > char *c) +{
> > > + apr_bucket *e;
> > > + char *current;
> > > + apr_size_t curlen;
> > > + apr_status_t status;
> > > +
> > > + current = c;
> > > +
> > > + APR_BRIGADE_FOREACH(e, b) {
> > > +
> > > + status = apr_bucket_read(e, (const char **)¤t, &curlen,
> > > + APR_BLOCK_READ);
> >
> > This is most likely wrong. You probably want to do a non-blocking read,
> > and return immediately if you can't find any data. That way, the caller
> > can keep working with the data he has. You also probably want to split
> > the brigade at that location, so that the caller can keep track of where
> > they are in the brigade.
>
> The code that it replaced in http_protocol.c used a blocking read
> (see http_protocol.c:1563). I guess the mode could be passed in
> to this function?
Yeah. In httpd, this makes sense for the location that we are talking about.
>From a strictly library POV, this ties it too much to APR.
> > > + if (status != APR_SUCCESS)
> > > + return status;
> > > +
> > > + current += curlen;
> > > + }
> > > +
> > > + return APR_SUCCESS;
> > > +
> > > +}
> > > +
> > > APU_DECLARE(apr_status_t) apr_brigade_to_iovec(apr_bucket_brigade *b,
> > > struct iovec *vec, int
> > > *nvec) {
> > > Index: buckets/apr_buckets_pipe.c
> > > ===================================================================
> > > RCS file: /home/cvs/apr-util/buckets/apr_buckets_pipe.c,v
> > > retrieving revision 1.41
> > > diff -u -r1.41 apr_buckets_pipe.c
> > > --- buckets/apr_buckets_pipe.c 2001/09/22 22:36:07 1.41
> > > +++ buckets/apr_buckets_pipe.c 2001/09/23 10:36:13
> > > @@ -106,15 +106,8 @@
> > > }
> > > else {
> > > free(buf);
> > > - a = apr_bucket_immortal_make(a, "", 0);
> > > - *str = a->data;
> > > - if (rv == APR_EOF) {
> > > + if (rv == APR_EOF)
> > > apr_file_close(p);
> > > - if (block == APR_NONBLOCK_READ) {
> > > - /* XXX: this is bogus, should return APR_SUCCESS */
> > > - return APR_EOF;
> > > - }
> > > - }
> >
> > You can't do this. The reason for the immortal bucket, was that we are
> > closing the pipe or socket bucket. What this change does, is it closes
> > the pipe, but leaves the bucket in the brigade. This will cause problems
> > later. You can't just remove the bucket, because you only have access to
> > the one bucket, and if you remove this bucket, then you also need to
> > update the pointer from the previous bucket, which you can't do.
>
> This adds all of the 0-length buckets that make some of the higher-level
> code more complicated. Wouldn't calling apr_bucket_delete(a) work here?
> The bucket contains a link to which brigade it is in - so it should be
> able to update the right pointers, no?
See Cliff's response, he is right on.
> > > @@ -655,12 +635,16 @@
> > > if (*readbytes == -1) {
> > > apr_bucket *e;
> > > apr_off_t total;
> > > + const char *str;
> > > + apr_size_t len;
> > > APR_BRIGADE_FOREACH(e, ctx->b) {
> > > - const char *str;
> > > - apr_size_t len;
> > > + /* We don't care about these values. We just want to
> > > force the + * lower level to convert it. This seems
> > > hackish. */ apr_bucket_read(e, &str, &len, APR_BLOCK_READ);
> >
> > Why does this seem hack-ish? We can't pass a socket up the filter stack
> > and we need to get all the data. This is the defined way to do that.
>
> It seems like we should have a function that says, "go through and
> read all the data but don't return the value to me." Actually,
> calling apr_brigade_length(ctx->b, 1, &len) would do the same
> thing - but that's even more hackish.
I would personally just use apr_brigade_length. Another function is overkill
for this IMHO.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] Rework httpd buckets/filtering
Posted by Ryan Bloom <rb...@covalent.net>.
On Sunday 23 September 2001 11:28 am, Cliff Woolley wrote:
> On Sun, 23 Sep 2001, Justin Erenkrantz wrote:
> > > You can't do this. The reason for the immortal bucket, was that we
> > > are closing the pipe or socket bucket. What this change does, is it
> > > closes the pipe, but leaves the bucket in the brigade. This will
> > > cause problems later. You can't just remove the bucket, because you
> > > only have access to the one bucket, and if you remove this bucket,
> > > then you also need to update the pointer from the previous bucket,
> > > which you can't do.
> >
> > This adds all of the 0-length buckets that make some of the higher-level
> > code more complicated. Wouldn't calling apr_bucket_delete(a) work here?
> > The bucket contains a link to which brigade it is in - so it should be
> > able to update the right pointers, no?
>
> No. You can fix b->prev->next and b->next->prev, but you can't fix the
> caller's copy of b. A bucket can never be allowed to delete itself for
> that very reason. When the pipe and socket buckets become empty, the very
> most efficient way to handle it is to just remove the pipe and socket
> bucket completely; but with the above constraint, the only way to do that
> is to replace the pipe/socket bucket with the lightest-weight bucket
> possible. That's an immortal bucket of zero length.
Exactly!
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] Rework httpd buckets/filtering
Posted by Cliff Woolley <cl...@yahoo.com>.
On Sun, 23 Sep 2001, Justin Erenkrantz wrote:
> > You can't do this. The reason for the immortal bucket, was that we
> > are closing the pipe or socket bucket. What this change does, is it
> > closes the pipe, but leaves the bucket in the brigade. This will
> > cause problems later. You can't just remove the bucket, because you
> > only have access to the one bucket, and if you remove this bucket,
> > then you also need to update the pointer from the previous bucket,
> > which you can't do.
>
> This adds all of the 0-length buckets that make some of the higher-level
> code more complicated. Wouldn't calling apr_bucket_delete(a) work here?
> The bucket contains a link to which brigade it is in - so it should be
> able to update the right pointers, no?
No. You can fix b->prev->next and b->next->prev, but you can't fix the
caller's copy of b. A bucket can never be allowed to delete itself for
that very reason. When the pipe and socket buckets become empty, the very
most efficient way to handle it is to just remove the pipe and socket
bucket completely; but with the above constraint, the only way to do that
is to replace the pipe/socket bucket with the lightest-weight bucket
possible. That's an immortal bucket of zero length.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: [PATCH] Rework httpd buckets/filtering
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Sun, Sep 23, 2001 at 08:33:07AM -0700, Ryan Bloom wrote:
>
> None of this can be committed until it has passed all of the tests in the perl test
> framework. The last time somebody (me) mucked with these filters it broke all of the
> CGI script handling. I would also like to hear from Jeff about this, because when we
> wrote the filters originally, we had input filtering use this design, and Jeff and Greg
> were adamant that filters needed to be able to return more than they were asked for.
> I am uncomfortable committing this until they have a chance to look it over.
Yeah, I have no intention of committing this until it has been suitably
reviewed. =-) This is a big change, but I do hope that it is a step
in the right direction. Or, that at least, we can figure out how to
go in the right direction.
> Other than that, I made some comments in-line that should be addressed. I will
> try to run it through the test framework today or tomorrow, so that I can give it
> a +1 or not.
>
> Ryan
>
>
> > Index: include/apr_buckets.h
> > ===================================================================
> > RCS file: /home/cvs/apr-util/include/apr_buckets.h,v
> > retrieving revision 1.118
> > diff -u -r1.118 apr_buckets.h
> > --- include/apr_buckets.h 2001/09/22 22:36:07 1.118
> > +++ include/apr_buckets.h 2001/09/23 10:36:12
> > @@ -689,6 +689,17 @@
> > apr_off_t *length);
> >
> > /**
> > + * create a flat buffer of the elements in a bucket_brigade.
> > + * @param b The bucket brigade to create the buffer from
> > + * @param c The pointer to the returned character string
> > + * @param len The length of the returned character string
> > + * @param p The pool to allocate the character string from.
> > + * Note: You *must* have enough space allocated to store all of the data.
> > + * See apr_brigade_length().
> > + */
> > +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b,
> > char *c); +
> > +/**
>
> Please change the docs to match the API.
Oops. Yeah.
> > Index: buckets/apr_brigade.c
> > ===================================================================
> > RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
> > retrieving revision 1.25
> > diff -u -r1.25 apr_brigade.c
> > --- buckets/apr_brigade.c 2001/09/03 22:00:36 1.25
> > +++ buckets/apr_brigade.c 2001/09/23 10:36:12
> > @@ -221,6 +221,29 @@
> > return APR_SUCCESS;
> > }
> >
> > +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b,
> > char *c) +{
> > + apr_bucket *e;
> > + char *current;
> > + apr_size_t curlen;
> > + apr_status_t status;
> > +
> > + current = c;
> > +
> > + APR_BRIGADE_FOREACH(e, b) {
> > +
> > + status = apr_bucket_read(e, (const char **)¤t, &curlen,
> > + APR_BLOCK_READ);
>
> This is most likely wrong. You probably want to do a non-blocking read, and
> return immediately if you can't find any data. That way, the caller can keep working
> with the data he has. You also probably want to split the brigade at that location,
> so that the caller can keep track of where they are in the brigade.
The code that it replaced in http_protocol.c used a blocking read
(see http_protocol.c:1563). I guess the mode could be passed in
to this function?
> > + if (status != APR_SUCCESS)
> > + return status;
> > +
> > + current += curlen;
> > + }
> > +
> > + return APR_SUCCESS;
> > +
> > +}
> > +
> > APU_DECLARE(apr_status_t) apr_brigade_to_iovec(apr_bucket_brigade *b,
> > struct iovec *vec, int
> > *nvec) {
> > Index: buckets/apr_buckets_pipe.c
> > ===================================================================
> > RCS file: /home/cvs/apr-util/buckets/apr_buckets_pipe.c,v
> > retrieving revision 1.41
> > diff -u -r1.41 apr_buckets_pipe.c
> > --- buckets/apr_buckets_pipe.c 2001/09/22 22:36:07 1.41
> > +++ buckets/apr_buckets_pipe.c 2001/09/23 10:36:13
> > @@ -106,15 +106,8 @@
> > }
> > else {
> > free(buf);
> > - a = apr_bucket_immortal_make(a, "", 0);
> > - *str = a->data;
> > - if (rv == APR_EOF) {
> > + if (rv == APR_EOF)
> > apr_file_close(p);
> > - if (block == APR_NONBLOCK_READ) {
> > - /* XXX: this is bogus, should return APR_SUCCESS */
> > - return APR_EOF;
> > - }
> > - }
>
> You can't do this. The reason for the immortal bucket, was that we are closing
> the pipe or socket bucket. What this change does, is it closes the pipe, but
> leaves the bucket in the brigade. This will cause problems later. You can't
> just remove the bucket, because you only have access to the one bucket,
> and if you remove this bucket, then you also need to update the pointer from
> the previous bucket, which you can't do.
This adds all of the 0-length buckets that make some of the higher-level
code more complicated. Wouldn't calling apr_bucket_delete(a) work here?
The bucket contains a link to which brigade it is in - so it should be
able to update the right pointers, no?
> > Index: modules/http/http_protocol.c
> > ===================================================================
> > RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
> > retrieving revision 1.362
> > diff -u -r1.362 http_protocol.c
> > --- modules/http/http_protocol.c 2001/09/17 13:12:37 1.362
> > +++ modules/http/http_protocol.c 2001/09/23 10:56:30
> > @@ -487,6 +487,7 @@
> > enum {
> > WANT_HDR /* must have value zero */,
> > WANT_BODY,
> > + WANT_ENDCHUNK,
> > WANT_TRL
> > } state;
> > };
> > @@ -498,9 +499,7 @@
> > {
> > apr_status_t rv;
> > struct dechunk_ctx *ctx = f->ctx;
> > - apr_bucket *b;
> > - const char *buf;
> > - apr_size_t len;
> > + apr_off_t len;
> >
> > if (!ctx) {
> > f->ctx = ctx = apr_pcalloc(f->r->pool, sizeof(struct dechunk_ctx));
> > @@ -508,43 +507,38 @@
> >
> > do {
> > if (ctx->chunk_size == ctx->bytes_delivered) {
> > - /* Time to read another chunk header or trailer... ap_http_filter() is
> > - * the next filter in line and it knows how to return a brigade with
> > - * one line.
> > - */
> > + /* Time to read another chunk header or trailer... We only want
> > + * one line - by specifying 0 as the length to read. */
> > char line[30];
> >
> > - if ((rv = ap_getline(line, sizeof(line), f->r,
> > - 0 /* readline */)) < 0) {
> > + if ((rv = ap_getline(line, sizeof(line), f->r, 0)) < 0) {
> > return rv;
> > }
> > switch(ctx->state) {
> > case WANT_HDR:
> > ctx->chunk_size = get_chunk_size(line);
> > ctx->bytes_delivered = 0;
> > - if (ctx->chunk_size == 0) {
> > + if (ctx->chunk_size == 0)
> > ctx->state = WANT_TRL;
> > - }
> > - else {
> > + else
> > ctx->state = WANT_BODY;
> > - }
>
> Don't do that. The { and } aren't strictly required, but it is good practice
> to have them. It makes people who are modifying the code later not have
> to worry about them. It also shrinks the size of the patch, which is good for
> people who have to review it. In this case, we would remove about 8 lines
> from the size of the patch. Not much, but every line that isn't in the patch
> helps.
Yeah, I'll remove the reformatting... I was tired and forgot to
remove the reformatting before I submitted.
> > @@ -655,12 +635,16 @@
> > if (*readbytes == -1) {
> > apr_bucket *e;
> > apr_off_t total;
> > + const char *str;
> > + apr_size_t len;
> > APR_BRIGADE_FOREACH(e, ctx->b) {
> > - const char *str;
> > - apr_size_t len;
> > + /* We don't care about these values. We just want to force the
> > + * lower level to convert it. This seems hackish. */
> > apr_bucket_read(e, &str, &len, APR_BLOCK_READ);
>
> Why does this seem hack-ish? We can't pass a socket up the filter stack
> and we need to get all the data. This is the defined way to do that.
It seems like we should have a function that says, "go through and
read all the data but don't return the value to me." Actually,
calling apr_brigade_length(ctx->b, 1, &len) would do the same
thing - but that's even more hackish.
>
> > }
> > APR_BRIGADE_CONCAT(b, ctx->b);
> > +
> > + /* Force a recompute of the length */
> > apr_brigade_length(b, 1, &total);
> > *readbytes = total;
> > e = apr_bucket_eos_create();
> > @@ -670,67 +654,40 @@
> > /* readbytes == 0 is "read a single line". otherwise, read a block. */
> > if (*readbytes) {
> > apr_off_t total;
> > + apr_bucket *e;
> > + apr_bucket_brigade *newbb;
> >
> > - /* ### the code below, which moves bytes from one brigade to the
> > - ### other is probably bogus. presuming the next filter down was
> > - ### working properly, it should not have returned more than
> > - ### READBYTES bytes, and we wouldn't have to do any work.
> > - */
> > -
> > - APR_BRIGADE_NORMALIZE(ctx->b);
> > - if (APR_BRIGADE_EMPTY(ctx->b)) {
> > - if ((rv = ap_get_brigade(f->next, ctx->b, mode, readbytes)) !=
> > APR_SUCCESS) { - return rv;
> > - }
> > - }
> > -
> > + newbb = NULL;
> > +
> > apr_brigade_partition(ctx->b, *readbytes, &e);
> > - APR_BRIGADE_CONCAT(b, ctx->b);
> > + /* Must do split before CONCAT */
> > if (e != APR_BRIGADE_SENTINEL(ctx->b)) {
> > - apr_bucket_brigade *temp;
> > + newbb = apr_brigade_split(ctx->b, e);
> > + }
> > + APR_BRIGADE_CONCAT(b, ctx->b);
> >
> > - temp = apr_brigade_split(b, e);
> > + /* FIXME: Is this really needed? Due to pointer use in sentinels,
> > + * I think so. */
> > + if (newbb)
> > + APR_BRIGADE_CONCAT(ctx->b, newbb);
> >
> > - /* ### darn. gotta ensure the split brigade is in the proper pool.
> > - ### this is a band-aid solution; we shouldn't even be doing
> > - ### all of this brigade munging (per the comment above).
> > - ### until then, this will get the right lifetimes. */
> > - APR_BRIGADE_CONCAT(ctx->b, temp);
> > - }
> > - else {
> > - if (!APR_BRIGADE_EMPTY(ctx->b)) {
> > - ctx->b = NULL; /*XXX*/
> > - }
> > - }
> > + /* FIXME: Are we assuming that b is empty when we enter? */
>
> You are in an input filter. All brigades must be empty when passed to an
> input filter. Data is put into the filter on the way back up.
Okay.
I'll resubmit without the reformatting and with fresh eyes to see
if I catch anything else this morning. =-)
Thanks. -- justin
Re: [PATCH] Rework httpd buckets/filtering
Posted by Ryan Bloom <rb...@covalent.net>.
None of this can be committed until it has passed all of the tests in the perl test
framework. The last time somebody (me) mucked with these filters it broke all of the
CGI script handling. I would also like to hear from Jeff about this, because when we
wrote the filters originally, we had input filtering use this design, and Jeff and Greg
were adamant that filters needed to be able to return more than they were asked for.
I am uncomfortable committing this until they have a chance to look it over.
Other than that, I made some comments in-line that should be addressed. I will
try to run it through the test framework today or tomorrow, so that I can give it
a +1 or not.
Ryan
> Index: include/apr_buckets.h
> ===================================================================
> RCS file: /home/cvs/apr-util/include/apr_buckets.h,v
> retrieving revision 1.118
> diff -u -r1.118 apr_buckets.h
> --- include/apr_buckets.h 2001/09/22 22:36:07 1.118
> +++ include/apr_buckets.h 2001/09/23 10:36:12
> @@ -689,6 +689,17 @@
> apr_off_t *length);
>
> /**
> + * create a flat buffer of the elements in a bucket_brigade.
> + * @param b The bucket brigade to create the buffer from
> + * @param c The pointer to the returned character string
> + * @param len The length of the returned character string
> + * @param p The pool to allocate the character string from.
> + * Note: You *must* have enough space allocated to store all of the data.
> + * See apr_brigade_length().
> + */
> +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b,
> char *c); +
> +/**
Please change the docs to match the API.
> Index: buckets/apr_brigade.c
> ===================================================================
> RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
> retrieving revision 1.25
> diff -u -r1.25 apr_brigade.c
> --- buckets/apr_brigade.c 2001/09/03 22:00:36 1.25
> +++ buckets/apr_brigade.c 2001/09/23 10:36:12
> @@ -221,6 +221,29 @@
> return APR_SUCCESS;
> }
>
> +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b,
> char *c) +{
> + apr_bucket *e;
> + char *current;
> + apr_size_t curlen;
> + apr_status_t status;
> +
> + current = c;
> +
> + APR_BRIGADE_FOREACH(e, b) {
> +
> + status = apr_bucket_read(e, (const char **)¤t, &curlen,
> + APR_BLOCK_READ);
This is most likely wrong. You probably want to do a non-blocking read, and
return immediately if you can't find any data. That way, the caller can keep working
with the data he has. You also probably want to split the brigade at that location,
so that the caller can keep track of where they are in the brigade.
> + if (status != APR_SUCCESS)
> + return status;
> +
> + current += curlen;
> + }
> +
> + return APR_SUCCESS;
> +
> +}
> +
> APU_DECLARE(apr_status_t) apr_brigade_to_iovec(apr_bucket_brigade *b,
> struct iovec *vec, int
> *nvec) {
> Index: buckets/apr_buckets_pipe.c
> ===================================================================
> RCS file: /home/cvs/apr-util/buckets/apr_buckets_pipe.c,v
> retrieving revision 1.41
> diff -u -r1.41 apr_buckets_pipe.c
> --- buckets/apr_buckets_pipe.c 2001/09/22 22:36:07 1.41
> +++ buckets/apr_buckets_pipe.c 2001/09/23 10:36:13
> @@ -106,15 +106,8 @@
> }
> else {
> free(buf);
> - a = apr_bucket_immortal_make(a, "", 0);
> - *str = a->data;
> - if (rv == APR_EOF) {
> + if (rv == APR_EOF)
> apr_file_close(p);
> - if (block == APR_NONBLOCK_READ) {
> - /* XXX: this is bogus, should return APR_SUCCESS */
> - return APR_EOF;
> - }
> - }
You can't do this. The reason for the immortal bucket, was that we are closing
the pipe or socket bucket. What this change does, is it closes the pipe, but
leaves the bucket in the brigade. This will cause problems later. You can't
just remove the bucket, because you only have access to the one bucket,
and if you remove this bucket, then you also need to update the pointer from
the previous bucket, which you can't do.
> }
> return APR_SUCCESS;
> }
> Index: buckets/apr_buckets_socket.c
> ===================================================================
> RCS file: /home/cvs/apr-util/buckets/apr_buckets_socket.c,v
> retrieving revision 1.30
> diff -u -r1.30 apr_buckets_socket.c
> --- buckets/apr_buckets_socket.c 2001/09/22 22:36:07 1.30
> +++ buckets/apr_buckets_socket.c 2001/09/23 10:36:13
> @@ -79,7 +79,7 @@
> }
>
> if (rv != APR_SUCCESS && rv != APR_EOF) {
> - free(buf);
> + free(buf);
> return rv;
> }
> /*
> @@ -109,12 +109,6 @@
> }
> else {
> free(buf);
> - a = apr_bucket_immortal_make(a, "", 0);
> - *str = a->data;
> - if (rv == APR_EOF && block == APR_NONBLOCK_READ) {
> - /* XXX: this is bogus... should return APR_SUCCESS */
> - return APR_EOF;
> - }
> }
> return APR_SUCCESS;
> }
Same as above.
> Index: modules/http/http_protocol.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
> retrieving revision 1.362
> diff -u -r1.362 http_protocol.c
> --- modules/http/http_protocol.c 2001/09/17 13:12:37 1.362
> +++ modules/http/http_protocol.c 2001/09/23 10:56:30
> @@ -487,6 +487,7 @@
> enum {
> WANT_HDR /* must have value zero */,
> WANT_BODY,
> + WANT_ENDCHUNK,
> WANT_TRL
> } state;
> };
> @@ -498,9 +499,7 @@
> {
> apr_status_t rv;
> struct dechunk_ctx *ctx = f->ctx;
> - apr_bucket *b;
> - const char *buf;
> - apr_size_t len;
> + apr_off_t len;
>
> if (!ctx) {
> f->ctx = ctx = apr_pcalloc(f->r->pool, sizeof(struct dechunk_ctx));
> @@ -508,43 +507,38 @@
>
> do {
> if (ctx->chunk_size == ctx->bytes_delivered) {
> - /* Time to read another chunk header or trailer... ap_http_filter() is
> - * the next filter in line and it knows how to return a brigade with
> - * one line.
> - */
> + /* Time to read another chunk header or trailer... We only want
> + * one line - by specifying 0 as the length to read. */
> char line[30];
>
> - if ((rv = ap_getline(line, sizeof(line), f->r,
> - 0 /* readline */)) < 0) {
> + if ((rv = ap_getline(line, sizeof(line), f->r, 0)) < 0) {
> return rv;
> }
> switch(ctx->state) {
> case WANT_HDR:
> ctx->chunk_size = get_chunk_size(line);
> ctx->bytes_delivered = 0;
> - if (ctx->chunk_size == 0) {
> + if (ctx->chunk_size == 0)
> ctx->state = WANT_TRL;
> - }
> - else {
> + else
> ctx->state = WANT_BODY;
> - }
Don't do that. The { and } aren't strictly required, but it is good practice
to have them. It makes people who are modifying the code later not have
to worry about them. It also shrinks the size of the patch, which is good for
people who have to review it. In this case, we would remove about 8 lines
from the size of the patch. Not much, but every line that isn't in the patch
helps.
> break;
> + case WANT_ENDCHUNK:
> + /* We don't care. Next state please. */
> + ctx->state = WANT_TRL;
> + 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? */
> - /* ### woah... ap_http_filter() is doing this, too */
> - /* append eos bucket and get out */
> - b = apr_bucket_eos_create();
> - APR_BRIGADE_INSERT_TAIL(bb, b);
> + /* Keep reading until we get to a blank line. */
> + /* Should add these headers to r->headers_in? */
> + if (!strlen(line))
> + {
> + ctx->state = WANT_HDR;
> return APR_SUCCESS;
> }
> - ctx->state = WANT_HDR;
> - break;
> + break;
> default:
> - ap_assert(ctx->state == WANT_HDR || ctx->state == WANT_TRL);
> + ap_assert(ctx->state == WANT_HDR || ctx->state == WANT_TRL ||
> + ctx->state == WANT_ENDCHUNK);
> }
> }
> } while (ctx->state != WANT_BODY);
> @@ -558,27 +552,11 @@
> return rv;
> }
>
> - /* Walk through the body, accounting for bytes, and removing an eos
> - * bucket if ap_http_filter() delivered the entire chunk. -
> *
> - * ### this shouldn't be necessary. 1) ap_http_filter shouldn't be
> - * ### adding EOS buckets. 2) it shouldn't return more bytes than
> - * ### we requested, therefore the total len can be found with a
> - * ### simple call to apr_brigade_length(). no further munging
> - * ### would be needed.
> - */
> - b = APR_BRIGADE_FIRST(bb);
> - while (b != APR_BRIGADE_SENTINEL(bb) && !APR_BUCKET_IS_EOS(b)) {
> - apr_bucket_read(b, &buf, &len, mode);
> - AP_DEBUG_ASSERT(len <= ctx->chunk_size -
> ctx->bytes_delivered); - ctx->bytes_delivered += len;
> - b = APR_BUCKET_NEXT(b);
> - }
> - if (ctx->bytes_delivered == ctx->chunk_size) {
> - AP_DEBUG_ASSERT(APR_BUCKET_IS_EOS(b));
> - apr_bucket_delete(b);
> - ctx->state = WANT_TRL;
> - }
> + /* Have we read a complete chunk? */
> + apr_brigade_length(bb, 1, &len);
> + ctx->bytes_delivered += len;
> + if (ctx->bytes_delivered == ctx->chunk_size)
> + ctx->state = WANT_ENDCHUNK;
> }
>
> return APR_SUCCESS;
> @@ -588,7 +566,8 @@
> apr_bucket_brigade *b;
> } http_ctx_t;
>
> -apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_off_t *readbytes)
> +apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
> + ap_input_mode_t mode, apr_off_t *readbytes) {
> apr_bucket *e;
> char *buff;
> @@ -641,7 +620,8 @@
> }
>
> if (APR_BRIGADE_EMPTY(ctx->b)) {
> - if ((rv = ap_get_brigade(f->next, ctx->b, mode, readbytes)) != APR_SUCCESS) {
> + if ((rv = ap_get_brigade(f->next, ctx->b, mode,
> + readbytes)) != APR_SUCCESS) {
Don't do this. You have cluttered this patch with code re-formatting. That is bad.
It makes people have to review more than is necessary. Keep the patch as
small as possible by not re-formatting it as you go please.
> return rv;
> }
> }
> @@ -655,12 +635,16 @@
> if (*readbytes == -1) {
> apr_bucket *e;
> apr_off_t total;
> + const char *str;
> + apr_size_t len;
> APR_BRIGADE_FOREACH(e, ctx->b) {
> - const char *str;
> - apr_size_t len;
> + /* We don't care about these values. We just want to force the
> + * lower level to convert it. This seems hackish. */
> apr_bucket_read(e, &str, &len, APR_BLOCK_READ);
Why does this seem hack-ish? We can't pass a socket up the filter stack
and we need to get all the data. This is the defined way to do that.
> }
> APR_BRIGADE_CONCAT(b, ctx->b);
> +
> + /* Force a recompute of the length */
> apr_brigade_length(b, 1, &total);
> *readbytes = total;
> e = apr_bucket_eos_create();
> @@ -670,67 +654,40 @@
> /* readbytes == 0 is "read a single line". otherwise, read a block. */
> if (*readbytes) {
> apr_off_t total;
> + apr_bucket *e;
> + apr_bucket_brigade *newbb;
>
> - /* ### the code below, which moves bytes from one brigade to the
> - ### other is probably bogus. presuming the next filter down was
> - ### working properly, it should not have returned more than
> - ### READBYTES bytes, and we wouldn't have to do any work.
> - */
> -
> - APR_BRIGADE_NORMALIZE(ctx->b);
> - if (APR_BRIGADE_EMPTY(ctx->b)) {
> - if ((rv = ap_get_brigade(f->next, ctx->b, mode, readbytes)) !=
> APR_SUCCESS) { - return rv;
> - }
> - }
> -
> + newbb = NULL;
> +
> apr_brigade_partition(ctx->b, *readbytes, &e);
> - APR_BRIGADE_CONCAT(b, ctx->b);
> + /* Must do split before CONCAT */
> if (e != APR_BRIGADE_SENTINEL(ctx->b)) {
> - apr_bucket_brigade *temp;
> + newbb = apr_brigade_split(ctx->b, e);
> + }
> + APR_BRIGADE_CONCAT(b, ctx->b);
>
> - temp = apr_brigade_split(b, e);
> + /* FIXME: Is this really needed? Due to pointer use in sentinels,
> + * I think so. */
> + if (newbb)
> + APR_BRIGADE_CONCAT(ctx->b, newbb);
>
> - /* ### darn. gotta ensure the split brigade is in the proper pool.
> - ### this is a band-aid solution; we shouldn't even be doing
> - ### all of this brigade munging (per the comment above).
> - ### until then, this will get the right lifetimes. */
> - APR_BRIGADE_CONCAT(ctx->b, temp);
> - }
> - else {
> - if (!APR_BRIGADE_EMPTY(ctx->b)) {
> - ctx->b = NULL; /*XXX*/
> - }
> - }
> + /* FIXME: Are we assuming that b is empty when we enter? */
You are in an input filter. All brigades must be empty when passed to an
input filter. Data is put into the filter on the way back up.
> apr_brigade_length(b, 1, &total);
> - *readbytes -= total;
> + *readbytes = total;
>
> - /* ### this is a hack. it is saying, "if we have read everything
> - ### that was requested, then we are at the end of the request."
> - ### it presumes that the next filter up will *only* call us
> - ### with readbytes set to the Content-Length of the request.
> - ### that may not always be true, and this code is *definitely*
> - ### too presumptive of the caller's intent. the point is: this
> - ### filter is not the guy that is parsing the headers or the
> - ### chunks to determine where the end of the request is, so we
> - ### shouldn't be monkeying with EOS buckets.
> - */
> - if (*readbytes == 0) {
> - apr_bucket *eos = apr_bucket_eos_create();
> -
> - APR_BRIGADE_INSERT_TAIL(b, eos);
> - }
> return APR_SUCCESS;
> }
>
> /* we are reading a single line, e.g. the HTTP headers */
> while (!APR_BRIGADE_EMPTY(ctx->b)) {
> e = APR_BRIGADE_FIRST(ctx->b);
> - if ((rv = apr_bucket_read(e, (const char **)&buff, &len, mode)) != APR_SUCCESS) {
> + if ((rv = apr_bucket_read(e, (const char **)&buff, &len,
> + mode)) != APR_SUCCESS) {
Again, wasted reformatting.
> return rv;
> }
>
> pos = memchr(buff, APR_ASCII_LF, len);
> + /* We found a match. */
> if (pos != NULL) {
> apr_bucket_split(e, pos - buff + 1);
> APR_BUCKET_REMOVE(e);
> @@ -740,6 +697,7 @@
> APR_BUCKET_REMOVE(e);
> APR_BRIGADE_INSERT_TAIL(b, e);
> }
> +
> return APR_SUCCESS;
> }
>
> @@ -1517,14 +1475,12 @@
> */
> AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer,
> apr_size_t bufsiz) {
> - apr_size_t len_read, total;
> - apr_status_t rv;
> - apr_bucket *b, *old;
> - const char *tempbuf;
> + apr_off_t len;
> + apr_bucket *b;
> core_request_config *req_cfg =
> (core_request_config *)ap_get_module_config(r->request_config,
> &core_module);
> - apr_bucket_brigade *bb = req_cfg->bb;
> + apr_bucket_brigade *bb = req_cfg->bb, *newbb;
>
> do {
> if (APR_BRIGADE_EMPTY(bb)) {
> @@ -1546,41 +1502,32 @@
> return 0;
> }
>
> - /* ### it would be nice to replace the code below with "consume N bytes
> - ### from this brigade, placing them into that buffer." there are
> - ### other places where we do the same...
> - ###
> - ### alternatively, we could partition the brigade, then call a
> - ### function which serializes a given brigade into a buffer. that
> - ### semantic is used elsewhere, too...
> - */
> -
> - total = 0;
> - while (total < bufsiz
> - && b != APR_BRIGADE_SENTINEL(bb)
> - && !APR_BUCKET_IS_EOS(b)) {
> -
> - if ((rv = apr_bucket_read(b, &tempbuf, &len_read, APR_BLOCK_READ)) != APR_SUCCESS) {
> - return -1;
> - }
> - if (total + len_read > bufsiz) {
> - apr_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; gotta see if
> anybody else actually uses - * these
> - */
> - r->read_length += len_read; /* XXX yank me? */
> - old = b;
> - b = APR_BUCKET_NEXT(b);
> - apr_bucket_delete(old);
> - }
> + /* 1) Determine length to see if we may overrun.
> + * 2) Partition the brigade at the appropriate point.
> + * 3) Split the brigade at the new partition.
> + * 4) Read the old brigade into the buffer.
> + * 5) Destroy the old brigade.
> + * 6) Point the context at the new brigade.
> + */
> + apr_brigade_length(bb, 1, &len);
> +
> + if (bufsiz < len)
> + len = bufsiz;
> +
> + if (apr_brigade_partition(bb, len, &b) != APR_SUCCESS)
> + return -1;
> +
> + newbb = apr_brigade_split(bb, b);
> +
> + if (apr_brigade_to_buffer(bb, buffer) != APR_SUCCESS)
> + return -1;
> +
> + if (apr_brigade_destroy(bb) != APR_SUCCESS)
> + return -1;
> +
> + req_cfg->bb = newbb;
>
> - return total;
> + return bufsiz;
> }
>
> /* In HTTP/1.1, any method can have a body. However, most GET handlers
--
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] Rework httpd buckets/filtering
Posted by Ian Holsman <ia...@apache.org>.
Hi Justin.
A good place to test the de-chunk logic is via the proxy module.
via the proxy, go to sites like apple.com/macosx
and see if the dechunking breaks them.
Regards
Ian
Justin Erenkrantz wrote:
> I started out trying to get flood to support chunked encoding (as
> recent changes to httpd have started to let chunked encoding work).
> Then, I decided to look at how httpd-2.0 handles dechunking (as it
> needs to be able to handle chunked input).
>
> The current dechunk code can't handle trailers properly (does anybody
> actually use them?) - according to my reading of RFC 2616, you can
> have multiple trailers (Roy?). So, I believe that part is broken.
>
> While I was examining the code, I ran into all of gstein's comments
> in dechunk_filter. And, then in ap_http_filter. So, this is an
> attempt to try to address some of the issues in http_protocol.c.
>
> This also has some patches and new functions to apr-util's buckets
> that seem like they are needed. Also, why is the socket (and pipe)
> code creating 0-length immortal buckets? I don't understand why
> and it serves to complicate things.
>
> The only thing I can say with this is that it compiles and still
> serves requests. But, I haven't put it through any extensive
> testing (i.e. any POSTs). I'd like to go to bed now, so I'm
> sending this out to the list so that people can review this and
> give feedback/flames. I'll keep playing with this tomorrow...
>
> The only question I have right now is whether we need anyone
> to produce the EOS bucket on input. I'm not terribly sure of
> that (seems Greg was thinking along these lines too). I know it
> is needed in the output, but I wonder where the right place is
> for the EOS on input. Thoughts? -- justin
>
> Index: include/apr_buckets.h
> ===================================================================
> RCS file: /home/cvs/apr-util/include/apr_buckets.h,v
> retrieving revision 1.118
> diff -u -r1.118 apr_buckets.h
> --- include/apr_buckets.h 2001/09/22 22:36:07 1.118
> +++ include/apr_buckets.h 2001/09/23 10:36:12
> @@ -689,6 +689,17 @@
> apr_off_t *length);
>
> /**
> + * create a flat buffer of the elements in a bucket_brigade.
> + * @param b The bucket brigade to create the buffer from
> + * @param c The pointer to the returned character string
> + * @param len The length of the returned character string
> + * @param p The pool to allocate the character string from.
> + * Note: You *must* have enough space allocated to store all of the data.
> + * See apr_brigade_length().
> + */
> +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b, char *c);
> +
> +/**
> * create an iovec of the elements in a bucket_brigade... return number
> * of elements used. This is useful for writing to a file or to the
> * network efficiently.
> Index: buckets/apr_brigade.c
> ===================================================================
> RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
> retrieving revision 1.25
> diff -u -r1.25 apr_brigade.c
> --- buckets/apr_brigade.c 2001/09/03 22:00:36 1.25
> +++ buckets/apr_brigade.c 2001/09/23 10:36:12
> @@ -221,6 +221,29 @@
> return APR_SUCCESS;
> }
>
> +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b, char *c)
> +{
> + apr_bucket *e;
> + char *current;
> + apr_size_t curlen;
> + apr_status_t status;
> +
> + current = c;
> +
> + APR_BRIGADE_FOREACH(e, b) {
> +
> + status = apr_bucket_read(e, (const char **)¤t, &curlen,
> + APR_BLOCK_READ);
> + if (status != APR_SUCCESS)
> + return status;
> +
> + current += curlen;
> + }
> +
> + return APR_SUCCESS;
> +
> +}
> +
> APU_DECLARE(apr_status_t) apr_brigade_to_iovec(apr_bucket_brigade *b,
> struct iovec *vec, int *nvec)
> {
> Index: buckets/apr_buckets_pipe.c
> ===================================================================
> RCS file: /home/cvs/apr-util/buckets/apr_buckets_pipe.c,v
> retrieving revision 1.41
> diff -u -r1.41 apr_buckets_pipe.c
> --- buckets/apr_buckets_pipe.c 2001/09/22 22:36:07 1.41
> +++ buckets/apr_buckets_pipe.c 2001/09/23 10:36:13
> @@ -106,15 +106,8 @@
> }
> else {
> free(buf);
> - a = apr_bucket_immortal_make(a, "", 0);
> - *str = a->data;
> - if (rv == APR_EOF) {
> + if (rv == APR_EOF)
> apr_file_close(p);
> - if (block == APR_NONBLOCK_READ) {
> - /* XXX: this is bogus, should return APR_SUCCESS */
> - return APR_EOF;
> - }
> - }
> }
> return APR_SUCCESS;
> }
> Index: buckets/apr_buckets_socket.c
> ===================================================================
> RCS file: /home/cvs/apr-util/buckets/apr_buckets_socket.c,v
> retrieving revision 1.30
> diff -u -r1.30 apr_buckets_socket.c
> --- buckets/apr_buckets_socket.c 2001/09/22 22:36:07 1.30
> +++ buckets/apr_buckets_socket.c 2001/09/23 10:36:13
> @@ -79,7 +79,7 @@
> }
>
> if (rv != APR_SUCCESS && rv != APR_EOF) {
> - free(buf);
> + free(buf);
> return rv;
> }
> /*
> @@ -109,12 +109,6 @@
> }
> else {
> free(buf);
> - a = apr_bucket_immortal_make(a, "", 0);
> - *str = a->data;
> - if (rv == APR_EOF && block == APR_NONBLOCK_READ) {
> - /* XXX: this is bogus... should return APR_SUCCESS */
> - return APR_EOF;
> - }
> }
> return APR_SUCCESS;
> }
>
> Index: modules/http/http_protocol.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
> retrieving revision 1.362
> diff -u -r1.362 http_protocol.c
> --- modules/http/http_protocol.c 2001/09/17 13:12:37 1.362
> +++ modules/http/http_protocol.c 2001/09/23 10:56:30
> @@ -487,6 +487,7 @@
> enum {
> WANT_HDR /* must have value zero */,
> WANT_BODY,
> + WANT_ENDCHUNK,
> WANT_TRL
> } state;
> };
> @@ -498,9 +499,7 @@
> {
> apr_status_t rv;
> struct dechunk_ctx *ctx = f->ctx;
> - apr_bucket *b;
> - const char *buf;
> - apr_size_t len;
> + apr_off_t len;
>
> if (!ctx) {
> f->ctx = ctx = apr_pcalloc(f->r->pool, sizeof(struct dechunk_ctx));
> @@ -508,43 +507,38 @@
>
> do {
> if (ctx->chunk_size == ctx->bytes_delivered) {
> - /* Time to read another chunk header or trailer... ap_http_filter() is
> - * the next filter in line and it knows how to return a brigade with
> - * one line.
> - */
> + /* Time to read another chunk header or trailer... We only want
> + * one line - by specifying 0 as the length to read. */
> char line[30];
>
> - if ((rv = ap_getline(line, sizeof(line), f->r,
> - 0 /* readline */)) < 0) {
> + if ((rv = ap_getline(line, sizeof(line), f->r, 0)) < 0) {
> return rv;
> }
> switch(ctx->state) {
> case WANT_HDR:
> ctx->chunk_size = get_chunk_size(line);
> ctx->bytes_delivered = 0;
> - if (ctx->chunk_size == 0) {
> + if (ctx->chunk_size == 0)
> ctx->state = WANT_TRL;
> - }
> - else {
> + else
> ctx->state = WANT_BODY;
> - }
> break;
> + case WANT_ENDCHUNK:
> + /* We don't care. Next state please. */
> + ctx->state = WANT_TRL;
> + 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? */
> - /* ### woah... ap_http_filter() is doing this, too */
> - /* append eos bucket and get out */
> - b = apr_bucket_eos_create();
> - APR_BRIGADE_INSERT_TAIL(bb, b);
> + /* Keep reading until we get to a blank line. */
> + /* Should add these headers to r->headers_in? */
> + if (!strlen(line))
> + {
> + ctx->state = WANT_HDR;
> return APR_SUCCESS;
> }
> - ctx->state = WANT_HDR;
> - break;
> + break;
> default:
> - ap_assert(ctx->state == WANT_HDR || ctx->state == WANT_TRL);
> + ap_assert(ctx->state == WANT_HDR || ctx->state == WANT_TRL ||
> + ctx->state == WANT_ENDCHUNK);
> }
> }
> } while (ctx->state != WANT_BODY);
> @@ -558,27 +552,11 @@
> return rv;
> }
>
> - /* Walk through the body, accounting for bytes, and removing an eos
> - * bucket if ap_http_filter() delivered the entire chunk.
> - *
> - * ### this shouldn't be necessary. 1) ap_http_filter shouldn't be
> - * ### adding EOS buckets. 2) it shouldn't return more bytes than
> - * ### we requested, therefore the total len can be found with a
> - * ### simple call to apr_brigade_length(). no further munging
> - * ### would be needed.
> - */
> - b = APR_BRIGADE_FIRST(bb);
> - while (b != APR_BRIGADE_SENTINEL(bb) && !APR_BUCKET_IS_EOS(b)) {
> - apr_bucket_read(b, &buf, &len, mode);
> - AP_DEBUG_ASSERT(len <= ctx->chunk_size - ctx->bytes_delivered);
> - ctx->bytes_delivered += len;
> - b = APR_BUCKET_NEXT(b);
> - }
> - if (ctx->bytes_delivered == ctx->chunk_size) {
> - AP_DEBUG_ASSERT(APR_BUCKET_IS_EOS(b));
> - apr_bucket_delete(b);
> - ctx->state = WANT_TRL;
> - }
> + /* Have we read a complete chunk? */
> + apr_brigade_length(bb, 1, &len);
> + ctx->bytes_delivered += len;
> + if (ctx->bytes_delivered == ctx->chunk_size)
> + ctx->state = WANT_ENDCHUNK;
> }
>
> return APR_SUCCESS;
> @@ -588,7 +566,8 @@
> apr_bucket_brigade *b;
> } http_ctx_t;
>
> -apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_off_t *readbytes)
> +apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
> + ap_input_mode_t mode, apr_off_t *readbytes)
> {
> apr_bucket *e;
> char *buff;
> @@ -641,7 +620,8 @@
> }
>
> if (APR_BRIGADE_EMPTY(ctx->b)) {
> - if ((rv = ap_get_brigade(f->next, ctx->b, mode, readbytes)) != APR_SUCCESS) {
> + if ((rv = ap_get_brigade(f->next, ctx->b, mode,
> + readbytes)) != APR_SUCCESS) {
> return rv;
> }
> }
> @@ -655,12 +635,16 @@
> if (*readbytes == -1) {
> apr_bucket *e;
> apr_off_t total;
> + const char *str;
> + apr_size_t len;
> APR_BRIGADE_FOREACH(e, ctx->b) {
> - const char *str;
> - apr_size_t len;
> + /* We don't care about these values. We just want to force the
> + * lower level to convert it. This seems hackish. */
> apr_bucket_read(e, &str, &len, APR_BLOCK_READ);
> }
> APR_BRIGADE_CONCAT(b, ctx->b);
> +
> + /* Force a recompute of the length */
> apr_brigade_length(b, 1, &total);
> *readbytes = total;
> e = apr_bucket_eos_create();
> @@ -670,67 +654,40 @@
> /* readbytes == 0 is "read a single line". otherwise, read a block. */
> if (*readbytes) {
> apr_off_t total;
> + apr_bucket *e;
> + apr_bucket_brigade *newbb;
>
> - /* ### the code below, which moves bytes from one brigade to the
> - ### other is probably bogus. presuming the next filter down was
> - ### working properly, it should not have returned more than
> - ### READBYTES bytes, and we wouldn't have to do any work.
> - */
> -
> - APR_BRIGADE_NORMALIZE(ctx->b);
> - if (APR_BRIGADE_EMPTY(ctx->b)) {
> - if ((rv = ap_get_brigade(f->next, ctx->b, mode, readbytes)) != APR_SUCCESS) {
> - return rv;
> - }
> - }
> -
> + newbb = NULL;
> +
> apr_brigade_partition(ctx->b, *readbytes, &e);
> - APR_BRIGADE_CONCAT(b, ctx->b);
> + /* Must do split before CONCAT */
> if (e != APR_BRIGADE_SENTINEL(ctx->b)) {
> - apr_bucket_brigade *temp;
> + newbb = apr_brigade_split(ctx->b, e);
> + }
> + APR_BRIGADE_CONCAT(b, ctx->b);
>
> - temp = apr_brigade_split(b, e);
> + /* FIXME: Is this really needed? Due to pointer use in sentinels,
> + * I think so. */
> + if (newbb)
> + APR_BRIGADE_CONCAT(ctx->b, newbb);
>
> - /* ### darn. gotta ensure the split brigade is in the proper pool.
> - ### this is a band-aid solution; we shouldn't even be doing
> - ### all of this brigade munging (per the comment above).
> - ### until then, this will get the right lifetimes. */
> - APR_BRIGADE_CONCAT(ctx->b, temp);
> - }
> - else {
> - if (!APR_BRIGADE_EMPTY(ctx->b)) {
> - ctx->b = NULL; /*XXX*/
> - }
> - }
> + /* FIXME: Are we assuming that b is empty when we enter? */
> apr_brigade_length(b, 1, &total);
> - *readbytes -= total;
> + *readbytes = total;
>
> - /* ### this is a hack. it is saying, "if we have read everything
> - ### that was requested, then we are at the end of the request."
> - ### it presumes that the next filter up will *only* call us
> - ### with readbytes set to the Content-Length of the request.
> - ### that may not always be true, and this code is *definitely*
> - ### too presumptive of the caller's intent. the point is: this
> - ### filter is not the guy that is parsing the headers or the
> - ### chunks to determine where the end of the request is, so we
> - ### shouldn't be monkeying with EOS buckets.
> - */
> - if (*readbytes == 0) {
> - apr_bucket *eos = apr_bucket_eos_create();
> -
> - APR_BRIGADE_INSERT_TAIL(b, eos);
> - }
> return APR_SUCCESS;
> }
>
> /* we are reading a single line, e.g. the HTTP headers */
> while (!APR_BRIGADE_EMPTY(ctx->b)) {
> e = APR_BRIGADE_FIRST(ctx->b);
> - if ((rv = apr_bucket_read(e, (const char **)&buff, &len, mode)) != APR_SUCCESS) {
> + if ((rv = apr_bucket_read(e, (const char **)&buff, &len,
> + mode)) != APR_SUCCESS) {
> return rv;
> }
>
> pos = memchr(buff, APR_ASCII_LF, len);
> + /* We found a match. */
> if (pos != NULL) {
> apr_bucket_split(e, pos - buff + 1);
> APR_BUCKET_REMOVE(e);
> @@ -740,6 +697,7 @@
> APR_BUCKET_REMOVE(e);
> APR_BRIGADE_INSERT_TAIL(b, e);
> }
> +
> return APR_SUCCESS;
> }
>
> @@ -1517,14 +1475,12 @@
> */
> AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer, apr_size_t bufsiz)
> {
> - apr_size_t len_read, total;
> - apr_status_t rv;
> - apr_bucket *b, *old;
> - const char *tempbuf;
> + apr_off_t len;
> + apr_bucket *b;
> core_request_config *req_cfg =
> (core_request_config *)ap_get_module_config(r->request_config,
> &core_module);
> - apr_bucket_brigade *bb = req_cfg->bb;
> + apr_bucket_brigade *bb = req_cfg->bb, *newbb;
>
> do {
> if (APR_BRIGADE_EMPTY(bb)) {
> @@ -1546,41 +1502,32 @@
> return 0;
> }
>
> - /* ### it would be nice to replace the code below with "consume N bytes
> - ### from this brigade, placing them into that buffer." there are
> - ### other places where we do the same...
> - ###
> - ### alternatively, we could partition the brigade, then call a
> - ### function which serializes a given brigade into a buffer. that
> - ### semantic is used elsewhere, too...
> - */
> -
> - total = 0;
> - while (total < bufsiz
> - && b != APR_BRIGADE_SENTINEL(bb)
> - && !APR_BUCKET_IS_EOS(b)) {
> -
> - if ((rv = apr_bucket_read(b, &tempbuf, &len_read, APR_BLOCK_READ)) != APR_SUCCESS) {
> - return -1;
> - }
> - if (total + len_read > bufsiz) {
> - apr_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; gotta see if anybody else actually uses
> - * these
> - */
> - r->read_length += len_read; /* XXX yank me? */
> - old = b;
> - b = APR_BUCKET_NEXT(b);
> - apr_bucket_delete(old);
> - }
> + /* 1) Determine length to see if we may overrun.
> + * 2) Partition the brigade at the appropriate point.
> + * 3) Split the brigade at the new partition.
> + * 4) Read the old brigade into the buffer.
> + * 5) Destroy the old brigade.
> + * 6) Point the context at the new brigade.
> + */
> + apr_brigade_length(bb, 1, &len);
> +
> + if (bufsiz < len)
> + len = bufsiz;
> +
> + if (apr_brigade_partition(bb, len, &b) != APR_SUCCESS)
> + return -1;
> +
> + newbb = apr_brigade_split(bb, b);
> +
> + if (apr_brigade_to_buffer(bb, buffer) != APR_SUCCESS)
> + return -1;
> +
> + if (apr_brigade_destroy(bb) != APR_SUCCESS)
> + return -1;
> +
> + req_cfg->bb = newbb;
>
> - return total;
> + return bufsiz;
> }
>
> /* In HTTP/1.1, any method can have a body. However, most GET handlers
>
>