httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aaron Bannert <aa...@clove.org>
Subject [PATCH] some fixes for ap_http_filter
Date Thu, 04 Oct 2001 19:52:17 GMT
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 */

Mime
View raw message