Rewrite ap_get_client_block to rely on assumptions that have been

solidified after this code was originally written.  Namely:

- AP_MODE_READBYTES will only return a brigade representing AT MOST bytes
  of data.  It can NOT return MORE than requested.
- APR_BLOCK_READ is respected - it is considered a design error of a filter
  if it returns without reading something.
- apr_brigade_flatten is available to do the heavy lifting of the copying
  into a flat buffer (as hinted at by the removed comment).

Tested with httpd-test.

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@95721 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 1b1da39dce6a1a4ad627ed9c538fcc328d540a2d..d5a91aec41003f44354b645233fcfc57d83e93c2 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,7 @@
-
 Changes with Apache 2.0.39
 
+  *) Improve ap_get_client_block implementation by using APR-util helper
+     functions.  [Justin Erenkrantz]
 
 Changes with Apache 2.0.38
 

diff --git a/modules/http/http_protocol.c b/modules/http/http_protocol.c
index 7cdc2b8e6e4958f6090b23cb4ee4b93d8bae299e..3ffe91a91d5d31bed180154dc25f78b7ccddefc5 100644 (file)
--- a/modules/http/http_protocol.c
+++ b/modules/http/http_protocol.c
@@ -1771,79 +1771,44 @@ static long get_chunk_size(char *b)
  * to read past the data provided by the client, since these reads block.
  * Returns 0 on End-of-body, -1 on error or premature chunk end.
  *
- * Reading the chunked encoding requires a buffer size large enough to
- * hold a chunk-size line, including any extensions. For now, we'll leave
- * that to the caller, at least until we can come up with a better solution.
  */
 AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer,
                                      apr_size_t bufsiz)
 {
-    apr_size_t total;
     apr_status_t rv;
-    apr_bucket *b;
-    const char *tempbuf;
-    core_request_config *req_cfg =
-        (core_request_config *)ap_get_module_config(r->request_config,
-                                                    &core_module);
-    apr_bucket_brigade *bb = req_cfg->bb;
-
-    /* read until we get a non-empty brigade */
-    while (APR_BRIGADE_EMPTY(bb)) {
-        if (ap_get_brigade(r->input_filters, bb, AP_MODE_READBYTES,
-                           APR_BLOCK_READ, bufsiz) != 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;
-        }
-    }
+    apr_bucket_brigade *bb;
 
-    b = APR_BRIGADE_FIRST(bb);
-    if (APR_BUCKET_IS_EOS(b)) { /* reached eos on previous invocation */
-        apr_bucket_delete(b);
-        return 0;
-    }
+    bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
 
-    /* ### it would be nice to replace the code below with "consume N bytes
-       ### from this brigade, placing them into that buffer." there are
-       ### other places where we do the same...
-       ###
-       ### alternatively, we could partition the brigade, then call a
-       ### function which serializes a given brigade into a buffer. that
-       ### semantic is used elsewhere, too...
-    */
-
-    total = 0;
-    while (total < bufsiz
-           && b != APR_BRIGADE_SENTINEL(bb)
-           && !APR_BUCKET_IS_EOS(b)) {
-        apr_size_t len_read;
-        apr_bucket *old;
-
-        if ((rv = apr_bucket_read(b, &tempbuf, &len_read,
-                                  APR_BLOCK_READ)) != APR_SUCCESS) {
-            return -1;
-        }
-        if (total + len_read > bufsiz) {
-            apr_bucket_split(b, bufsiz - total);
-            len_read = bufsiz - total;
-        }
-        memcpy(buffer, tempbuf, len_read);
-        buffer += len_read;
-        total += len_read;
-        /* XXX the next field shouldn't be mucked with here,
-         * as it is in terms of bytes in the unfiltered body;
-         * gotta see if anybody else actually uses it
+    rv = ap_get_brigade(r->input_filters, bb, AP_MODE_READBYTES,
+                        APR_BLOCK_READ, bufsiz);
+  
+    /* We lose the failure code here.  This is why ap_get_client_block should
+     * not be used.
+     */
+    if (rv != APR_SUCCESS) { 
+        /* if we actually fail here, we want to just return and
+         * stop trying to read data from the client.
          */
-        r->read_length += len_read; /* XXX yank me? */
-        old = b;
-        b = APR_BUCKET_NEXT(b);
-        apr_bucket_delete(old);
+        r->connection->keepalive = -1;
+        return -1;
+    }
+
+    /* If this fails, it means that a filter is written incorrectly and that
+     * it needs to learn how to properly handle APR_BLOCK_READ requests by
+     * returning data when requested.
+     */
+    AP_DEBUG_ASSERT(!APR_BRIGADE_EMPTY(bb));
+ 
+    rv = apr_brigade_flatten(bb, buffer, &bufsiz);
+    if (rv != APR_SUCCESS) {
+        return -1;
     }
 
-    return total;
+    /* XXX yank me? */
+    r->read_length += bufsiz;
+
+    return bufsiz;
 }
 
 /* In HTTP/1.1, any method can have a body.  However, most GET handlers