Merge r609394, r609538, r609540 from trunk:

* Fix cases with non blocking reads from the ap_http_filter input filter where
  chunk size lines or empty lines after a chunk are read incomplete. This can
  lead to a corruption inside the dechunking algorithm.
  This patch has an issue with larger chunk-extensions which need to get thrown
  away since we ignore them anyway.

PR: 19954, 41056
Tested by: niq

* Optimize solution from r609394 and remove chunk-extensions restriction that
  was in r609394.

* Optimize alignment.

Submitted by: rpluem
Reviewed by: rpluem, jim, niq

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x@610119 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 32cae191e694e006bba5f6f7f30541b4c9b17a5f..b0fa79903710de328768c76e302f3c8fdd5f8698 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -83,7 +83,7 @@ Changes with Apache 2.2.7
 
   *) core: Fix broken chunk filtering that causes all non blocking reads to be
      converted into blocking reads.  PR 19954, 41056.
-     [Jean-Frederic Clere, Jim Jagielski]
+     [Jean-Frederic Clere, Jim Jagielski, Ruediger Pluem, Nick Kew]
 
   *) mod_rewrite: Add the novary flag to RewriteCond.
      [Ruediger Pluem]

diff --git a/STATUS b/STATUS
index cec03e797f57d3c9b21bc9754c1cb2e79377a311..b4f2afd30bd89b94331ba0e22712039d20207578 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -80,15 +80,6 @@ RELEASE SHOWSTOPPERS:
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
-  * core: Fix regression with non blocking reads from chunk input filter
-    Trunk version of patch:
-        http://svn.apache.org/viewvc?rev=609394&view=rev
-        http://svn.apache.org/viewvc?rev=609538&view=rev
-        http://svn.apache.org/viewvc?rev=609540&view=rev
-    Backport version for 2.2.x of patch:
-        Trunk version of patch works
-    +1: rpluem, jim, niq
-
 PATCHES PROPOSED TO BACKPORT FROM TRUNK:
   [ New proposals should be added at the end of the list ]
 

diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c
index e5d821cc250c3d1950a9e172009e947bdbdd0ff7..aa658faf21ee12119db197ed83d6ae4699964cc8 100644 (file)
--- a/modules/http/http_filters.c
+++ b/modules/http/http_filters.c
@@ -68,8 +68,106 @@ typedef struct http_filter_ctx {
         BODY_CHUNK_PART
     } state;
     int eos_sent;
+    char chunk_ln[32];
+    char *pos;
+    apr_off_t linesize;
 } http_ctx_t;
 
+static apr_status_t get_remaining_chunk_line(http_ctx_t *ctx,
+                                             apr_bucket_brigade *b,
+                                             int linelimit)
+{
+    apr_status_t rv;
+    apr_off_t brigade_length;
+    apr_bucket *e;
+    const char *lineend;
+    apr_size_t len;
+
+    /*
+     * As the brigade b should have been requested in mode AP_MODE_GETLINE
+     * all buckets in this brigade are already some type of memory
+     * buckets (due to the needed scanning for LF in mode AP_MODE_GETLINE)
+     * or META buckets.
+     */
+    rv = apr_brigade_length(b, 0, &brigade_length);
+    if (rv != APR_SUCCESS) {
+        return rv;
+    }
+    /* Sanity check. Should never happen. See above. */
+    if (brigade_length == -1) {
+        return APR_EGENERAL;
+    }
+    if (!brigade_length) {
+        return APR_EAGAIN;
+    }
+    ctx->linesize += brigade_length;
+    if (ctx->linesize > linelimit) {
+        return APR_ENOSPC;
+    }
+    /*
+     * As all buckets are already some type of memory buckets or META buckets
+     * (see above), we only need to check the last byte in the last data bucket.
+     */
+    for (e = APR_BRIGADE_LAST(b);
+         e != APR_BRIGADE_SENTINEL(b);
+         e = APR_BUCKET_PREV(e)) {
+
+        if (!APR_BUCKET_IS_METADATA(e)) {
+            break;
+        }
+    }
+    rv = apr_bucket_read(e, &lineend, &len, APR_BLOCK_READ);
+    if (rv != APR_SUCCESS) {
+        return rv;
+    }
+    if (lineend[len - 1] != APR_ASCII_LF) {
+        return APR_EAGAIN;
+    }
+    /* Line is complete. So reset ctx->linesize for next round. */
+    ctx->linesize = 0;
+    return APR_SUCCESS;
+}
+
+static apr_status_t get_chunk_line(http_ctx_t *ctx, apr_bucket_brigade *b,
+                                   int linelimit)
+{
+    apr_size_t len;
+    apr_status_t rv;
+
+    len = sizeof(ctx->chunk_ln) - (ctx->pos - ctx->chunk_ln) - 1;
+    /*
+     * Check if there is space left in ctx->chunk_ln. If not, then either
+     * the chunk size is insane or we have chunk-extensions. Ignore both
+     * by discarding the remaining part of the line via
+     * get_remaining_chunk_line. Only bail out if the line is too long.
+     */
+    if (len > 0) {
+        rv = apr_brigade_flatten(b, ctx->pos, &len);
+        if (rv != APR_SUCCESS) {
+            return rv;
+        }
+        ctx->pos += len;
+        ctx->linesize += len;
+        *(ctx->pos) = '\0';
+        /*
+         * Check if we really got a full line. If yes the
+         * last char in the just read buffer must be LF.
+         * If not advance the buffer and return APR_EAGAIN.
+         * We do not start processing until we have the
+         * full line.
+         */
+        if (ctx->pos[-1] != APR_ASCII_LF) {
+            /* Check if the remaining data in the brigade has the LF */
+            return get_remaining_chunk_line(ctx, b, linelimit);
+        }
+        /* Line is complete. So reset ctx->pos for next round. */
+        ctx->pos = ctx->chunk_ln;
+        return APR_SUCCESS;
+    }
+    return get_remaining_chunk_line(ctx, b, linelimit);
+}
+
+
 /* This is the HTTP_INPUT filter for HTTP requests and responses from
  * proxied servers (mod_proxy).  It handles chunked and content-length
  * bodies.  This can only be inserted/used after the headers
@@ -96,6 +194,8 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
         ctx->remaining = 0;
         ctx->limit_used = 0;
         ctx->eos_sent = 0;
+        ctx->pos = ctx->chunk_ln;
+        ctx->linesize = 0;
 
         /* LimitRequestBody does not apply to proxied responses.
          * Consider implementing this check in its own filter.
@@ -227,10 +327,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
 
         /* We can't read the chunk until after sending 100 if required. */
         if (ctx->state == BODY_CHUNK) {
-            char line[30];
             apr_bucket_brigade *bb;
-            apr_size_t len = 30;
-            apr_off_t brigade_length;
 
             bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
 
@@ -246,20 +343,14 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
             }
 
             if (rv == APR_SUCCESS) {
-                /* We have to check the length of the brigade we got back.
-                 * We will not accept partial or blank lines.
-                 */
-                rv = apr_brigade_length(bb, 1, &brigade_length);
-                if (rv == APR_SUCCESS
-                    && (!brigade_length ||
-                        brigade_length > f->r->server->limit_req_line)) {
-                    rv = APR_ENOSPC;
+                rv = get_chunk_line(ctx, bb, f->r->server->limit_req_line);
+                if (APR_STATUS_IS_EAGAIN(rv)) {
+                    apr_brigade_cleanup(bb);
+                    ctx->state = BODY_CHUNK_PART;
+                    return rv;
                 }
                 if (rv == APR_SUCCESS) {
-                    rv = apr_brigade_flatten(bb, line, &len);
-                    if (rv == APR_SUCCESS) {
-                        ctx->remaining = get_chunk_size(line);
-                    }
+                    ctx->remaining = get_chunk_size(ctx->chunk_ln);
                 }
             }
             apr_brigade_cleanup(bb);
@@ -308,9 +399,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
         case BODY_CHUNK:
         case BODY_CHUNK_PART:
             {
-                char line[30];
                 apr_bucket_brigade *bb;
-                apr_size_t len = 30;
                 apr_status_t http_error = HTTP_REQUEST_ENTITY_TOO_LARGE;
 
                 bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
@@ -319,11 +408,23 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                 if (ctx->state == BODY_CHUNK) {
                     rv = ap_get_brigade(f->next, bb, AP_MODE_GETLINE,
                                         block, 0);
-                    apr_brigade_cleanup(bb);
                     if (block == APR_NONBLOCK_READ &&
-                        (APR_STATUS_IS_EAGAIN(rv))) {
+                        ( (rv == APR_SUCCESS && APR_BRIGADE_EMPTY(bb)) ||
+                          (APR_STATUS_IS_EAGAIN(rv)) )) {
                         return APR_EAGAIN;
                     }
+                    /*
+                     * We really don't care whats on this line. If it is RFC
+                     * compliant it should be only \r\n. If there is more
+                     * before we just ignore it as long as we do not get over
+                     * the limit for request lines.
+                     */
+                    rv = get_remaining_chunk_line(ctx, bb,
+                                                  f->r->server->limit_req_line);
+                    apr_brigade_cleanup(bb);
+                    if (APR_STATUS_IS_EAGAIN(rv)) {
+                        return rv;
+                    }
                 } else {
                     rv = APR_SUCCESS;
                 }
@@ -341,15 +442,23 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                     }
                     ctx->state = BODY_CHUNK;
                     if (rv == APR_SUCCESS) {
-                        rv = apr_brigade_flatten(bb, line, &len);
+                        rv = get_chunk_line(ctx, bb, f->r->server->limit_req_line);
+                        if (APR_STATUS_IS_EAGAIN(rv)) {
+                            ctx->state = BODY_CHUNK_PART;
+                            apr_brigade_cleanup(bb);
+                            return rv;
+                        }
                         if (rv == APR_SUCCESS) {
-                            /* Wait a sec, that's a blank line!  Oh no. */
-                            if (!len) {
+                            /*
+                             * Wait a sec, that's a blank line or it starts
+                             * with an invalid character!  Oh no.
+                             */
+                            if (!apr_isxdigit(*(ctx->chunk_ln))) {
                                 rv = APR_EGENERAL;
                                 http_error = HTTP_SERVICE_UNAVAILABLE;
                             }
                             else {
-                                ctx->remaining = get_chunk_size(line);
+                                ctx->remaining = get_chunk_size(ctx->chunk_ln);
                             }
                         }
                     }