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
