Fix a bug in the input handling. ap_http_filter() was modifying *readbytes

which corresponded to r->remaining (in ap_get_client_block). However,
ap_get_client_block was *also* adjusting r->remaining. Net result was that
PUT (and probably POST) was broken. (at least on large inputs)

To fix it, I simply removed the indirection on "readbytes" for input
filters. There is no reason for them to return data (the brigade length is
the return length). This also simplifies a number of calls where people
needed to do &zero just to pass zero.

I also added a number of comments about operations and where things could be
improved, or are (semi) broken.

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

diff --git a/include/util_filter.h b/include/util_filter.h
index 17e5c0acadf2b96ff4c9c9c4fd469c65fc657351..2ee93fa8ee41e30613342d7e5a80fc37ed7165aa 100644 (file)
--- a/include/util_filter.h
+++ b/include/util_filter.h
@@ -155,7 +155,7 @@ typedef struct ap_filter_t ap_filter_t;
  */
 typedef apr_status_t (*ap_out_filter_func)(ap_filter_t *f, apr_bucket_brigade *b);
 typedef apr_status_t (*ap_in_filter_func)(ap_filter_t *f, apr_bucket_brigade *b, 
-                                          ap_input_mode_t mode, apr_size_t *readbytes);
+                                          ap_input_mode_t mode, apr_size_t readbytes);
 
 typedef union ap_filter_func {
     ap_out_filter_func out_func;
@@ -276,7 +276,7 @@ struct ap_filter_t {
  *                  a single line should be read.
  */
 AP_DECLARE(apr_status_t) ap_get_brigade(ap_filter_t *filter, apr_bucket_brigade *bucket, 
-                                        ap_input_mode_t mode, apr_size_t *readbytes);
+                                        ap_input_mode_t mode, apr_size_t readbytes);
 
 /**
  * Pass the current bucket brigade down to the next filter on the filter

diff --git a/modules/experimental/mod_charset_lite.c b/modules/experimental/mod_charset_lite.c
index 79830eed08da375cae5011a9cfea1c3eac8b4536..190ec1b4d344ecb292052281d96ebaebe578329e 100644 (file)
--- a/modules/experimental/mod_charset_lite.c
+++ b/modules/experimental/mod_charset_lite.c
@@ -1005,7 +1005,7 @@ static void transfer_brigade(apr_bucket_brigade *in, apr_bucket_brigade *out)
 }
 
 static int xlate_in_filter(ap_filter_t *f, apr_bucket_brigade *bb, 
-                           ap_input_mode_t mode, apr_size_t *readbytes)
+                           ap_input_mode_t mode, apr_size_t readbytes)
 {
     apr_status_t rv;
     charset_req_t *reqinfo = ap_get_module_config(f->r->request_config,

diff --git a/modules/experimental/mod_ext_filter.c b/modules/experimental/mod_ext_filter.c
index 46b4b1e5273df102727481e201163f58fff9e7b2..7f9d87b62c3ab1bddbf55ab0279c58a5ce34b041 100644 (file)
--- a/modules/experimental/mod_ext_filter.c
+++ b/modules/experimental/mod_ext_filter.c
@@ -749,7 +749,7 @@ static apr_status_t ef_output_filter(ap_filter_t *f, apr_bucket_brigade *bb)
 
 #if 0
 static int ef_input_filter(ap_filter_t *f, apr_bucket_brigade *bb, 
-                           ap_input_mode_t mode, apr_size_t *readbytes)
+                           ap_input_mode_t mode, apr_size_t readbytes)
 {
     apr_status_t rv;
     apr_bucket *b;

diff --git a/modules/http/http_protocol.c b/modules/http/http_protocol.c
index a4426466be8fcbefadef9aedd34f909b18559dca..f092dbdbaf86f267c01f7b2b5c814d71772de975 100644 (file)
--- a/modules/http/http_protocol.c
+++ b/modules/http/http_protocol.c
@@ -414,13 +414,17 @@ AP_DECLARE(const char *) ap_method_name_of(int methnum)
 struct dechunk_ctx {
     apr_size_t chunk_size;
     apr_size_t bytes_delivered;
-    enum {WANT_HDR /* must have value zero */, WANT_BODY, WANT_TRL} state;
+    enum {
+        WANT_HDR /* must have value zero */,
+        WANT_BODY,
+        WANT_TRL
+    } state;
 };
 
 static long get_chunk_size(char *);
 
 apr_status_t ap_dechunk_filter(ap_filter_t *f, apr_bucket_brigade *bb,
-                               ap_input_mode_t mode, apr_size_t *readbytes)
+                               ap_input_mode_t mode, apr_size_t readbytes)
 {
     apr_status_t rv;
     struct dechunk_ctx *ctx = f->ctx;
@@ -440,7 +444,8 @@ apr_status_t ap_dechunk_filter(ap_filter_t *f, apr_bucket_brigade *bb,
              */
             char line[30];
             
-            if ((rv = ap_getline(line, sizeof(line), f->r, 0)) < 0) {
+            if ((rv = ap_getline(line, sizeof(line), f->r,
+                                 0 /* readline */)) < 0) {
                 return rv;
             }
             switch(ctx->state) {
@@ -460,6 +465,7 @@ apr_status_t ap_dechunk_filter(ap_filter_t *f, apr_bucket_brigade *bb,
                     /* bad trailer */
                 }
                 if (ctx->chunk_size == 0) { /* we just finished the last chunk? */
+                    /* ### woah... ap_http_filter() is doing this, too */
                     /* append eos bucket and get out */
                     b = apr_bucket_eos_create();
                     APR_BRIGADE_INSERT_TAIL(bb, b);
@@ -475,12 +481,21 @@ apr_status_t ap_dechunk_filter(ap_filter_t *f, apr_bucket_brigade *bb,
 
     if (ctx->state == WANT_BODY) {
         /* Tell ap_http_filter() how many bytes to deliver. */
-        apr_size_t readbytes = ctx->chunk_size - ctx->bytes_delivered;
-        if ((rv = ap_get_brigade(f->next, bb, mode, &readbytes)) != APR_SUCCESS) {
+        apr_size_t chunk_bytes = ctx->chunk_size - ctx->bytes_delivered;
+
+        if ((rv = ap_get_brigade(f->next, bb, mode,
+                                 chunk_bytes)) != APR_SUCCESS) {
             return rv;
         }
-        /* Walk through the body, accounting for bytes, and removing an eos bucket if
-         * ap_http_filter() delivered the entire chunk.
+
+        /* Walk through the body, accounting for bytes, and removing an eos
+         * bucket if ap_http_filter() delivered the entire chunk.
+         *
+         * ### this shouldn't be necessary. 1) ap_http_filter shouldn't be
+         * ### adding EOS buckets. 2) it shouldn't return more bytes than
+         * ### we requested, therefore the total len can be found with a
+         * ### simple call to apr_brigade_length(). no further munging
+         * ### would be needed.
          */
         b = APR_BRIGADE_FIRST(bb);
         while (b != APR_BRIGADE_SENTINEL(bb) && !APR_BUCKET_IS_EOS(b)) {
@@ -503,7 +518,7 @@ typedef struct http_filter_ctx {
     apr_bucket_brigade *b;
 } http_ctx_t;
 
-apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_size_t *readbytes)
+apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_size_t readbytes)
 {
     apr_bucket *e;
     char *buff;
@@ -561,7 +576,16 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode
         }
     }
 
-    if (*readbytes) {
+    /* readbytes == 0 is "read a single line". otherwise, read a block. */
+    if (readbytes) {
+
+        /* ### the code below, which moves bytes from one brigade to the
+           ### other is probably bogus. presuming the next filter down was
+           ### working properly, it should not have returned more than
+           ### READBYTES bytes, and we wouldn't have to do any work. further,
+           ### we could probably just use brigade_partition() in here.
+        */
+
         while (!APR_BRIGADE_EMPTY(ctx->b)) {
             const char *ignore;
 
@@ -580,12 +604,12 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode
                  * a time - don't assume that one call to apr_bucket_read()
                  * will return the full string.
                  */
-                if (*readbytes < len) {
-                    apr_bucket_split(e, *readbytes);
-                    *readbytes = 0;
+                if (readbytes < len) {
+                    apr_bucket_split(e, readbytes);
+                    readbytes = 0;
                 }
                 else {
-                    *readbytes -= len;
+                    readbytes -= len;
                 }
                 APR_BUCKET_REMOVE(e);
                 APR_BRIGADE_INSERT_TAIL(b, e);
@@ -593,7 +617,18 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode
             }
             apr_bucket_delete(e);
         }
-        if (*readbytes == 0) {
+
+        /* ### this is a hack. it is saying, "if we have read everything
+           ### that was requested, then we are at the end of the request."
+           ### it presumes that the next filter up will *only* call us
+           ### with readbytes set to the Content-Length of the request.
+           ### that may not always be true, and this code is *definitely*
+           ### too presumptive of the caller's intent. the point is: this
+           ### filter is not the guy that is parsing the headers or the
+           ### chunks to determine where the end of the request is, so we
+           ### shouldn't be monkeying with EOS buckets.
+        */
+        if (readbytes == 0) {
             apr_bucket *eos = apr_bucket_eos_create();
                 
             APR_BRIGADE_INSERT_TAIL(b, eos);
@@ -601,6 +636,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode
         return APR_SUCCESS;
     }
 
+    /* we are reading a single line, e.g. the HTTP headers */
     while (!APR_BRIGADE_EMPTY(ctx->b)) {
         e = APR_BRIGADE_FIRST(ctx->b);
         if ((rv = apr_bucket_read(e, (const char **)&buff, &len, mode)) != APR_SUCCESS) {
@@ -1366,7 +1402,8 @@ AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer, int bufsiz)
 
     do {
         if (APR_BRIGADE_EMPTY(bb)) {
-            if (ap_get_brigade(r->input_filters, bb, AP_MODE_BLOCKING, &r->remaining) != APR_SUCCESS) {
+            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.
                  */
@@ -1375,16 +1412,28 @@ AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer, int bufsiz)
                 return -1;
             }
         }
-        b = APR_BRIGADE_FIRST(bb);
     } while (APR_BRIGADE_EMPTY(bb));
 
-    if (APR_BUCKET_IS_EOS(b)) {         /* reached eos on previous invocation */
+    b = APR_BRIGADE_FIRST(bb);
+    if (APR_BUCKET_IS_EOS(b)) { /* reached eos on previous invocation */
         apr_bucket_delete(b);
         return 0;
     }
 
+    /* ### 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)) {
+    while (total < bufsiz
+           && b != APR_BRIGADE_SENTINEL(bb)
+           && !APR_BUCKET_IS_EOS(b)) {
+
         if ((rv = apr_bucket_read(b, &tempbuf, &len_read, APR_BLOCK_READ)) != APR_SUCCESS) {
             return -1;
         }

diff --git a/modules/http/http_request.c b/modules/http/http_request.c
index 56064c451bdb0203099945de8901b031db13f460..d79f4c79878f9c022231dc7529952d7d0148ac26 100644 (file)
--- a/modules/http/http_request.c
+++ b/modules/http/http_request.c
@@ -367,7 +367,6 @@ static void check_pipeline_flush(request_rec *r)
        ### allow us to defer creation of the brigade to when we actually
        ### need to send a FLUSH. */
     apr_bucket_brigade *bb = apr_brigade_create(r->pool);
-    apr_size_t zero = 0;
 
     /* Flush the filter contents if:
      *
@@ -375,8 +374,9 @@ static void check_pipeline_flush(request_rec *r)
      *   2) there isn't a request ready to be read
      */
     /* ### shouldn't this read from the connection input filters? */
+    /* ### is zero correct? that means "read one line" */
     if (!r->connection->keepalive || 
-        ap_get_brigade(r->input_filters, bb, AP_MODE_PEEK, &zero) != APR_SUCCESS) {
+        ap_get_brigade(r->input_filters, bb, AP_MODE_PEEK, 0) != APR_SUCCESS) {
         apr_bucket *e = apr_bucket_flush_create();
 
         /* We just send directly to the connection based filters.  At

diff --git a/modules/http/mod_core.h b/modules/http/mod_core.h
index 29fef07b650ea629e1ab62339410a07972eb26e1..2f149797bff9ee62edd9ae4f582af30f40ff6053 100644 (file)
--- a/modules/http/mod_core.h
+++ b/modules/http/mod_core.h
@@ -73,8 +73,10 @@ extern "C" {
 /*
  * These (input) filters are internal to the mod_core operation.
  */
-apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_size_t *readbytes);
-apr_status_t ap_dechunk_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_size_t *readbytes);
+apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
+                            ap_input_mode_t mode, apr_size_t readbytes);
+apr_status_t ap_dechunk_filter(ap_filter_t *f, apr_bucket_brigade *b,
+                               ap_input_mode_t mode, apr_size_t readbytes);
 
 char *ap_response_code_string(request_rec *r, int error_index);
 

diff --git a/modules/tls/mod_tls.c b/modules/tls/mod_tls.c
index 2fd8cc75d341bc742b3c5dcf3715c04dacaaafeb..3f3130694b275a03b4ff85906e3cbd0312da36cb 100644 (file)
--- a/modules/tls/mod_tls.c
+++ b/modules/tls/mod_tls.c
@@ -186,7 +186,7 @@ static apr_status_t churn_output(TLSFilterCtx *pCtx)
     return APR_SUCCESS;
 }
 
-static apr_status_t churn(TLSFilterCtx *pCtx,apr_read_type_e eReadType,apr_size_t *readbytes)
+static apr_status_t churn(TLSFilterCtx *pCtx,apr_read_type_e eReadType,apr_size_t readbytes)
 {
     ap_input_mode_t eMode=eReadType == APR_BLOCK_READ ? AP_MODE_BLOCKING
       : AP_MODE_NONBLOCKING;
@@ -283,7 +283,6 @@ static apr_status_t tls_out_filter(ap_filter_t *f,apr_bucket_brigade *pbbIn)
 {
     TLSFilterCtx *pCtx=f->ctx;
     apr_bucket *pbktIn;
-    apr_size_t zero = 0;
 
     APR_BRIGADE_FOREACH(pbktIn,pbbIn) {
        const char *data;
@@ -299,7 +298,7 @@ static apr_status_t tls_out_filter(ap_filter_t *f,apr_bucket_brigade *pbbIn)
                ret=churn_output(pCtx);
                if(ret != APR_SUCCESS)
                    return ret;
-               ret=churn(pCtx,APR_NONBLOCK_READ,&zero);
+               ret=churn(pCtx,APR_NONBLOCK_READ,0);
                if(ret != APR_SUCCESS) {
                    if(ret == APR_EOF)
                        return APR_SUCCESS;
@@ -312,7 +311,7 @@ static apr_status_t tls_out_filter(ap_filter_t *f,apr_bucket_brigade *pbbIn)
 
        if(APR_BUCKET_IS_FLUSH(pbktIn)) {
            /* assume that churn will flush (or already has) if there's output */
-           ret=churn(pCtx,APR_NONBLOCK_READ,&zero);
+           ret=churn(pCtx,APR_NONBLOCK_READ,0);
            if(ret != APR_SUCCESS)
                return ret;
            continue;
@@ -334,7 +333,7 @@ static apr_status_t tls_out_filter(ap_filter_t *f,apr_bucket_brigade *pbbIn)
 }
 
 static apr_status_t tls_in_filter(ap_filter_t *f,apr_bucket_brigade *pbbOut,
-                                 ap_input_mode_t eMode, apr_size_t *readbytes)
+                                 ap_input_mode_t eMode, apr_size_t readbytes)
 {
     TLSFilterCtx *pCtx=f->ctx;
     apr_read_type_e eReadType=eMode == AP_MODE_BLOCKING ? APR_BLOCK_READ :

diff --git a/server/core.c b/server/core.c
index 092eb62f09d6948c9aeab98e48c49405aedc7f8f..b230fc974e6ffe4321743677fafeef88acdcd899 100644 (file)
--- a/server/core.c
+++ b/server/core.c
@@ -2995,10 +2995,18 @@ static int default_handler(request_rec *r)
     return ap_pass_brigade(r->output_filters, bb);
 }
 
-static int core_input_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_size_t *readbytes)
+static int core_input_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_size_t readbytes)
 {
     apr_bucket *e;
-    
+
+    /* ### we should obey readbytes. the policy is to not insert more than
+       ### READBYTES into the brigade. the caller knows the amount that is
+       ### proper for the protocol. reading more than that could cause
+       ### problems.
+       ### (of course, we can read them from the socket, we just should not
+       ###  return them until asked)
+    */
+
     if (!f->ctx) {    /* If we haven't passed up the socket yet... */
         f->ctx = (void *)1;
         e = apr_bucket_socket_create(f->c->client_socket);

diff --git a/server/mpm/experimental/perchild/perchild.c b/server/mpm/experimental/perchild/perchild.c
index 0fa14997b1fae66df2ed499484cd7c9197bc0bea..4bad0ccdfd426e8d7ce8cbbbcd906d925a5e7bcb 100644 (file)
--- a/server/mpm/experimental/perchild/perchild.c
+++ b/server/mpm/experimental/perchild/perchild.c
@@ -1362,7 +1362,6 @@ static int pass_request(request_rec *r)
                                                  &mpm_perchild_module);
     char *foo;
     apr_size_t len;
-    apr_size_t readbytes = 0;
 
     apr_pool_userdata_get((void **)&foo, "PERCHILD_BUFFER", r->connection->pool);
     len = strlen(foo);
@@ -1397,8 +1396,12 @@ static int pass_request(request_rec *r)
     }
 
     write(sconf->sd2, foo, len);
-   
-    while (ap_get_brigade(r->input_filters, bb, AP_MODE_NONBLOCKING, &readbytes) == APR_SUCCESS) {
+
+    /* ### this "read one line" doesn't seem right... shouldn't we be
+       ### reading large chunks of data or something?
+    */
+    while (ap_get_brigade(r->input_filters, bb, AP_MODE_NONBLOCKING,
+                          0 /* read one line */) == APR_SUCCESS) {
         apr_bucket *e;
         APR_BRIGADE_FOREACH(e, bb) {
             const char *str;
@@ -1492,7 +1495,7 @@ static int perchild_post_read(request_rec *r)
     return OK;
 }
 
-static apr_status_t perchild_buffer(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_size_t *readbytes)
+static apr_status_t perchild_buffer(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_size_t readbytes)
 {
     apr_bucket *e;
     apr_status_t rv;

diff --git a/server/mpm/perchild/perchild.c b/server/mpm/perchild/perchild.c
index 0fa14997b1fae66df2ed499484cd7c9197bc0bea..4bad0ccdfd426e8d7ce8cbbbcd906d925a5e7bcb 100644 (file)
--- a/server/mpm/perchild/perchild.c
+++ b/server/mpm/perchild/perchild.c
@@ -1362,7 +1362,6 @@ static int pass_request(request_rec *r)
                                                  &mpm_perchild_module);
     char *foo;
     apr_size_t len;
-    apr_size_t readbytes = 0;
 
     apr_pool_userdata_get((void **)&foo, "PERCHILD_BUFFER", r->connection->pool);
     len = strlen(foo);
@@ -1397,8 +1396,12 @@ static int pass_request(request_rec *r)
     }
 
     write(sconf->sd2, foo, len);
-   
-    while (ap_get_brigade(r->input_filters, bb, AP_MODE_NONBLOCKING, &readbytes) == APR_SUCCESS) {
+
+    /* ### this "read one line" doesn't seem right... shouldn't we be
+       ### reading large chunks of data or something?
+    */
+    while (ap_get_brigade(r->input_filters, bb, AP_MODE_NONBLOCKING,
+                          0 /* read one line */) == APR_SUCCESS) {
         apr_bucket *e;
         APR_BRIGADE_FOREACH(e, bb) {
             const char *str;
@@ -1492,7 +1495,7 @@ static int perchild_post_read(request_rec *r)
     return OK;
 }
 
-static apr_status_t perchild_buffer(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_size_t *readbytes)
+static apr_status_t perchild_buffer(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_size_t readbytes)
 {
     apr_bucket *e;
     apr_status_t rv;

diff --git a/server/protocol.c b/server/protocol.c
index 93db7c32d016de8fb3f20489b83ddf94529492d5..0d816b835e7e08fe93cb8baf73131d8e1f8efbb3 100644 (file)
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -203,7 +203,6 @@ AP_CORE_DECLARE(int) ap_getline(char *s, int n, request_rec *r, int fold)
     int retval;
     int total = 0;
     int looking_ahead = 0;
-    apr_size_t zero = 0;
     apr_size_t length;
     conn_rec *c = r->connection;
     core_request_config *req_cfg;
@@ -218,7 +217,9 @@ AP_CORE_DECLARE(int) ap_getline(char *s, int n, request_rec *r, int fold)
 
     while (1) {
         if (APR_BRIGADE_EMPTY(b)) {
-            if ((retval = ap_get_brigade(c->input_filters, b, AP_MODE_BLOCKING, &zero)) != APR_SUCCESS ||
+            if ((retval = ap_get_brigade(c->input_filters, b,
+                                         AP_MODE_BLOCKING,
+                                         0 /* readline */)) != APR_SUCCESS ||
                 APR_BRIGADE_EMPTY(b)) {
                 apr_brigade_destroy(b);
                 return -1;

diff --git a/server/util_filter.c b/server/util_filter.c
index 2291919eb9a26634f32d7cb7f020211413682099..eaa3ad3f71e02c4b2fd5e77a96ffcad3f6a30ef9 100644 (file)
--- a/server/util_filter.c
+++ b/server/util_filter.c
@@ -208,8 +208,10 @@ AP_DECLARE(void) ap_remove_output_filter(ap_filter_t *f)
  * save data off to the side should probably create their own temporary
  * brigade especially for that use.
  */
-AP_DECLARE(apr_status_t) ap_get_brigade(ap_filter_t *next, apr_bucket_brigade *bb, 
-                                        ap_input_mode_t mode, apr_size_t *readbytes)
+AP_DECLARE(apr_status_t) ap_get_brigade(ap_filter_t *next,
+                                        apr_bucket_brigade *bb, 
+                                        ap_input_mode_t mode,
+                                        apr_size_t readbytes)
 {
     if (next) {
         return next->frec->filter_func.in_func(next, bb, mode, readbytes);