Return-Path: Delivered-To: apmail-httpd-dev-archive@httpd.apache.org Received: (qmail 72780 invoked by uid 500); 4 Oct 2001 20:14:52 -0000 Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm Precedence: bulk Reply-To: dev@httpd.apache.org list-help: list-unsubscribe: list-post: Delivered-To: mailing list dev@httpd.apache.org Received: (qmail 72758 invoked from network); 4 Oct 2001 20:14:51 -0000 Date: Thu, 4 Oct 2001 13:14:31 -0700 From: Justin Erenkrantz To: dev@httpd.apache.org Subject: Re: [PATCH] some fixes for ap_http_filter Message-ID: <20011004131430.Q21545@ebuilt.com> References: <20011004125217.C13476@clove.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20011004125217.C13476@clove.org> User-Agent: Mutt/1.3.22.1i X-AntiVirus: scanned for viruses by AMaViS 0.2.1-pre3 (http://amavis.org/) X-Spam-Rating: daedalus.apache.org 1.6.2 0/1000/N 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. > - 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... > - 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. > - 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... 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. =) > } > } > } > > - if (!ctx->remaining) > + if (ctx->remaining <= 0) I think this negative check is a bit bogus for reasons above... > { > 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. > /* 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... > > - if (ctx->state != BODY_NONE) > + if (ctx->state != BODY_NONE) { No need for braces here... > 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