I came across a few things that needed attention in my review of the
http filter code:
- ctx->remaining was uninitialized.
- I'm a little uncomfortable with the Content-Length: parsing, please
see my comments in the diff.
- 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.
-aaron
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) */
}
}
}
- if (!ctx->remaining)
+ if (ctx->remaining <= 0)
{
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) {
/* 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;
+ }
- if (ctx->state != BODY_NONE)
+ if (ctx->state != BODY_NONE) {
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));
+ }
b = APR_BRIGADE_FIRST(bb);
if (APR_BUCKET_IS_EOS(b)) { /* reached eos on previous invocation */
