On Thu, Oct 04, 2001 at 01:14:31PM -0700, Justin Erenkrantz wrote:
> On Thu, Oct 04, 2001 at 12:52:17PM -0700, Aaron Bannert wrote:
> > I came across a few things that needed attention in my review of the
> > http filter code:
>
> Cool.
[hey! Aren't you supposed to be in class? ;) ]
> > - ctx->remaining was uninitialized.
>
> We don't care what ctx->remaining is unless we have a special
> parsing type which sets ctx->remaining. But, yeah, this may
> make more sense with my below comment...
It seemed to me that we were making assumptions about our input. There
were code paths that could be reached where ctx->remaining would be
used before being initialized.
> > - I'm a little uncomfortable with the Content-Length: parsing, please
> > see my comments in the diff.
>
> I didn't touch that loop. =) But, yeah, once we see a digit,
> we should read until we don't see a digit.
I don't really know where the loop came from, I just wanna make sure it's
safe. (more comments below)
> > - a logic reduction in a do-while loop (reduces the number of times
> > we call APR_BRIGADE_EMPTY by half).
> > - made the ctx->remaining condition check more robust.
>
> We should never return more than ctx->remaining. That's the whole
> point. =) So, checking it for negative value may not be necessary.
> In fact, if it is negative, we have major problems. If you want
> to place an assert there - because it should never happen...
"should" being the operative word here. It was not obvious to me how
we ensure ctx->remaining is always non-negative. We are trusting our
input variables on a filter where we aren't supposed to know what our
neighbors are. Maybe I've missed some crutial piece of logic that
would quell my fears, but robust >= reliable. :)
> Some quick comments inline...
>
> > Index: modules/http/http_protocol.c
> > ===================================================================
> > RCS file: /home/cvspublic/httpd-2.0/modules/http/http_protocol.c,v
> > retrieving revision 1.369
> > diff -u -r1.369 http_protocol.c
> > --- modules/http/http_protocol.c 2001/10/02 21:13:41 1.369
> > +++ modules/http/http_protocol.c 2001/10/04 18:34:25
> > @@ -504,6 +504,7 @@
> > if (!ctx) {
> > const char *tenc, *lenp;
> > f->ctx = ctx = apr_palloc(f->r->pool, sizeof(*ctx));
> > + ctx->remaining = 0;
> > ctx->state = BODY_NONE;
> >
> > tenc = apr_table_get(f->r->headers_in, "Transfer-Encoding");
> > @@ -523,26 +524,32 @@
> > else if (lenp) {
> > const char *pos = lenp;
> >
> > + /* XXX: Is this safe? I can do "Content-Length: 3 3 3" ... */
> > while (apr_isdigit(*pos) || apr_isspace(*pos))
> > ++pos;
> > if (*pos == '\0') {
> > ctx->state = BODY_LENGTH;
> > ctx->remaining = atol(lenp);
> > + /* XXX: Are there any platforms where apr_off_t != long?
> > + aka: Would it be possible to overflow here? */
> > + /* XXX: Can we get here with an invalid number? (ie <0) */
>
> As I said, this was just copied from the content-length parsing routines
> we already had. So, possibly. =)
No prob, just bringing it to our attention.
I thought it was kind of neat that I could do:
curl -H "Content-Length: 3 3 3 3 3 3 3 3 3" -d "foo" [some url] and it
would work fine. This part is NBD ("flexible on input, strict on output"),
I'm really much more concerned about the implicit type cast from a long
to an off_t (apr_off_t). What would happen if we had a platform where
those didn't match, and someone could get an overflow (and potentially
a negative number) into ctx->remaining? Would bad things happen? That's
why I made the next change:
> > }
> > }
> > }
> >
> > - if (!ctx->remaining)
> > + if (ctx->remaining <= 0)
>
> I think this negative check is a bit bogus for reasons above...
They are both logically correct, but now it makes fewer assumptions
about the input.
> > {
> > switch (ctx->state)
> > {
> > case BODY_NONE:
> > break;
> > case BODY_LENGTH:
> > + /* We've already consumed the Content-length size, so flush. */
> > e = apr_bucket_eos_create();
> > APR_BRIGADE_INSERT_TAIL(b, e);
> > return APR_SUCCESS;
> > case BODY_CHUNK:
> > + /* Done with this chunk, go get the next chunk length. */
> > {
> > char line[30];
> >
> > @@ -560,8 +567,7 @@
> > ctx->state = BODY_CHUNK;
> > ctx->remaining = get_chunk_size(line);
> >
> > - if (!ctx->remaining)
> > - {
> > + if (ctx->remaining <= 0) {
>
> No. =) Java weenie.
Who's the Java weenie? This is the same change as above with the additional
benefit of meeting our style guide.
(btw, Where do you get this java thing from?)
> > /* Handle trailers by calling get_mime_headers again! */
> > e = apr_bucket_eos_create();
> > APR_BRIGADE_INSERT_TAIL(b, e);
> > @@ -572,13 +578,13 @@
> > }
> > }
> >
> > - rv = ap_get_brigade(f->next, b, mode, readbytes);
> > -
> > - if (rv != APR_SUCCESS)
> > + if ((rv = ap_get_brigade(f->next, b, mode, readbytes)) != APR_SUCCESS) {
> > return rv;
> > + }
>
> I don't like this style. Blech. It makes the line too long...
That one was a little gratuitous, sorry. ;)
> >
> > - if (ctx->state != BODY_NONE)
> > + if (ctx->state != BODY_NONE) {
>
> No need for braces here...
It's a safety thing. If you're willing to commit my patch I'll take them
out. :)
> > ctx->remaining -= *readbytes;
> > + }
> >
> > return APR_SUCCESS;
> > }
> > @@ -1360,19 +1366,17 @@
> > &core_module);
> > apr_bucket_brigade *bb = req_cfg->bb;
> >
> > - do {
> > - if (APR_BRIGADE_EMPTY(bb)) {
> > - if (ap_get_brigade(r->input_filters, bb, AP_MODE_BLOCKING,
> > - &r->remaining) != APR_SUCCESS) {
> > - /* if we actually fail here, we want to just return and
> > - * stop trying to read data from the client.
> > - */
> > - r->connection->keepalive = -1;
> > - apr_brigade_destroy(bb);
> > - return -1;
> > - }
> > + while (APR_BRIGADE_EMPTY(bb)) {
> > + if (ap_get_brigade(r->input_filters, bb, AP_MODE_BLOCKING,
> > + &r->remaining) != APR_SUCCESS) {
> > + /* if we actually fail here, we want to just return and
> > + * stop trying to read data from the client.
> > + */
> > + r->connection->keepalive = -1;
> > + apr_brigade_destroy(bb);
> > + return -1;
> > }
> > - } while (APR_BRIGADE_EMPTY(bb));
> > + }
>
> We purposely chose to ignore ap_get_client_block this go around.
> I had a complete rewrite for it, but put it on hold until the
> filters are more stable. So, this entire section of code is
> getting thrown out as soon as someone gets to it. So, yeah,
> it sucks majorly. -- justin
I'm a filter newbie, so I have no idea what parts are in flux or not. Just
been proofreading the code and you know how much I love to make the logic
faster :)
-aaron
