Merge r1619448, r1619486, r1895552, r1894152, r1914800 from trunk:

leave a hint while scrolling through inflate() calls

mod_deflate:
- fix signed/unsigned (int/size_t) comparisons,
- add consume_buffer() to factorize code used multiple times,
- cleanup passed brigade (don't rely on next output filters to do it).

* modules/filters/mod_deflate.c (deflate_in_filter): Handle FLUSH in
  the input brigade even if done inflating (ctx->done is true), but
  don't try to flush the inflate stream in that case.  (Caught by
  Coverity)

* modules/filters/mod_deflate.c (deflate_out_filter): Catch
  apr_bucket_read() errors.

mod_deflate: remove filter after seeing EOS

Github: closes #423
Submitted by: covener, ylavic, jorton, Eric Norris <enorris etsy.com>
Reviewed by: jorton, gbechis, ylavic

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

diff --git a/changes-entries/deflate-cleanups.txt b/changes-entries/deflate-cleanups.txt
new file mode 100644 (file)
index 0000000..18a32a6
--- /dev/null
+++ b/changes-entries/deflate-cleanups.txt
@@ -0,0 +1,4 @@
+  *) mod_deflate: Fixes and better logging for handling various
+     error and edge cases. [Eric Covener, Yann Ylavic, Joe Orton,
+     Eric Norris <enorris etsy.com>]
+

diff --git a/modules/filters/mod_deflate.c b/modules/filters/mod_deflate.c
index de18606c79566a20b7c7780b103fa0b81385b689..5a541e738776f28ecf8c974ec31ce0ffd1c1b7f4 100644 (file)
--- a/modules/filters/mod_deflate.c
+++ b/modules/filters/mod_deflate.c
@@ -66,7 +66,7 @@ typedef struct deflate_filter_config_t
     int windowSize;
     int memlevel;
     int compressionlevel;
-    apr_size_t bufferSize;
+    int bufferSize;
     const char *note_ratio_name;
     const char *note_input_name;
     const char *note_output_name;
@@ -254,7 +254,7 @@ static const char *deflate_set_buffer_size(cmd_parms *cmd, void *dummy,
         return "DeflateBufferSize should be positive";
     }
 
-    c->bufferSize = (apr_size_t)n;
+    c->bufferSize = n;
 
     return NULL;
 }
@@ -416,35 +416,40 @@ typedef struct deflate_ctx_t
 /* Do update ctx->crc, see comment in flush_libz_buffer */
 #define UPDATE_CRC 1
 
+static void consume_buffer(deflate_ctx *ctx, deflate_filter_config *c,
+                           int len, int crc, apr_bucket_brigade *bb)
+{
+    apr_bucket *b;
+
+    /*
+     * Do we need to update ctx->crc? Usually this is the case for
+     * inflate action where we need to do a crc on the output, whereas
+     * in the deflate case we need to do a crc on the input
+     */
+    if (crc) {
+        ctx->crc = crc32(ctx->crc, (const Bytef *)ctx->buffer, len);
+    }
+
+    b = apr_bucket_heap_create((char *)ctx->buffer, len, NULL,
+                               bb->bucket_alloc);
+    APR_BRIGADE_INSERT_TAIL(bb, b);
+
+    ctx->stream.next_out = ctx->buffer;
+    ctx->stream.avail_out = c->bufferSize;
+}
+
 static int flush_libz_buffer(deflate_ctx *ctx, deflate_filter_config *c,
-                             struct apr_bucket_alloc_t *bucket_alloc,
                              int (*libz_func)(z_streamp, int), int flush,
                              int crc)
 {
     int zRC = Z_OK;
     int done = 0;
-    unsigned int deflate_len;
-    apr_bucket *b;
+    int deflate_len;
 
     for (;;) {
          deflate_len = c->bufferSize - ctx->stream.avail_out;
-
-         if (deflate_len != 0) {
-             /*
-              * Do we need to update ctx->crc? Usually this is the case for
-              * inflate action where we need to do a crc on the output, whereas
-              * in the deflate case we need to do a crc on the input
-              */
-             if (crc) {
-                 ctx->crc = crc32(ctx->crc, (const Bytef *)ctx->buffer,
-                                  deflate_len);
-             }
-             b = apr_bucket_heap_create((char *)ctx->buffer,
-                                        deflate_len, NULL,
-                                        bucket_alloc);
-             APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
-             ctx->stream.next_out = ctx->buffer;
-             ctx->stream.avail_out = c->bufferSize;
+         if (deflate_len > 0) {
+             consume_buffer(ctx, c, deflate_len, crc, ctx->bb);
          }
 
          if (done)
@@ -560,6 +565,7 @@ static apr_status_t deflate_out_filter(ap_filter_t *f,
     request_rec *r = f->r;
     deflate_ctx *ctx = f->ctx;
     int zRC;
+    apr_status_t rv;
     apr_size_t len = 0, blen;
     const char *data;
     deflate_filter_config *c;
@@ -891,8 +897,7 @@ static apr_status_t deflate_out_filter(ap_filter_t *f,
 
             ctx->stream.avail_in = 0; /* should be zero already anyway */
             /* flush the remaining data from the zlib buffers */
-            flush_libz_buffer(ctx, c, f->c->bucket_alloc, deflate, Z_FINISH,
-                              NO_UPDATE_CRC);
+            flush_libz_buffer(ctx, c, deflate, Z_FINISH, NO_UPDATE_CRC);
 
             buf = apr_palloc(r->pool, VALIDATION_SIZE);
             putLong((unsigned char *)&buf[0], ctx->crc);
@@ -935,6 +940,10 @@ static apr_status_t deflate_out_filter(ap_filter_t *f,
             }
 
             deflateEnd(&ctx->stream);
+
+            /* We've ended the libz stream, so remove ourselves. */
+            ap_remove_output_filter(f);
+
             /* No need for cleanup any longer */
             apr_pool_cleanup_kill(r->pool, ctx, deflate_ctx_cleanup);
 
@@ -945,15 +954,15 @@ static apr_status_t deflate_out_filter(ap_filter_t *f,
             /* Okay, we've seen the EOS.
              * Time to pass it along down the chain.
              */
-            return ap_pass_brigade(f->next, ctx->bb);
+            rv = ap_pass_brigade(f->next, ctx->bb);
+            apr_brigade_cleanup(ctx->bb);
+            return rv;
         }
 
         if (APR_BUCKET_IS_FLUSH(e)) {
-            apr_status_t rv;
-
             /* flush the remaining data from the zlib buffers */
-            zRC = flush_libz_buffer(ctx, c, f->c->bucket_alloc, deflate,
-                                    Z_SYNC_FLUSH, NO_UPDATE_CRC);
+            zRC = flush_libz_buffer(ctx, c, deflate, Z_SYNC_FLUSH,
+                                    NO_UPDATE_CRC);
             if (zRC != Z_OK) {
                 ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01385)
                               "Zlib error %d flushing zlib output buffer (%s)",
@@ -965,6 +974,7 @@ static apr_status_t deflate_out_filter(ap_filter_t *f,
             APR_BUCKET_REMOVE(e);
             APR_BRIGADE_INSERT_TAIL(ctx->bb, e);
             rv = ap_pass_brigade(f->next, ctx->bb);
+            apr_brigade_cleanup(ctx->bb);
             if (rv != APR_SUCCESS) {
                 return rv;
             }
@@ -982,7 +992,12 @@ static apr_status_t deflate_out_filter(ap_filter_t *f,
         }
 
         /* read */
-        apr_bucket_read(e, &data, &len, APR_BLOCK_READ);
+        rv = apr_bucket_read(e, &data, &len, APR_BLOCK_READ);
+        if (rv) {
+            ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(10298)
+                          "failed reading from %s bucket", e->type->name);
+            return rv;
+        }
         if (!len) {
             apr_bucket_delete(e);
             continue;
@@ -999,21 +1014,15 @@ static apr_status_t deflate_out_filter(ap_filter_t *f,
         ctx->stream.next_in = (unsigned char *)data; /* We just lost const-ness,
                                                       * but we'll just have to
                                                       * trust zlib */
-        ctx->stream.avail_in = len;
+        ctx->stream.avail_in = (int)len;
 
         while (ctx->stream.avail_in != 0) {
             if (ctx->stream.avail_out == 0) {
-                apr_status_t rv;
+                consume_buffer(ctx, c, c->bufferSize, NO_UPDATE_CRC, ctx->bb);
 
-                ctx->stream.next_out = ctx->buffer;
-                len = c->bufferSize - ctx->stream.avail_out;
-
-                b = apr_bucket_heap_create((char *)ctx->buffer, len,
-                                           NULL, f->c->bucket_alloc);
-                APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
-                ctx->stream.avail_out = c->bufferSize;
                 /* Send what we have right now to the next filter. */
                 rv = ap_pass_brigade(f->next, ctx->bb);
+                apr_brigade_cleanup(ctx->bb);
                 if (rv != APR_SUCCESS) {
                     return rv;
                 }
@@ -1310,44 +1319,40 @@ static apr_status_t deflate_in_filter(ap_filter_t *f,
             if (APR_BUCKET_IS_FLUSH(bkt)) {
                 apr_bucket *tmp_b;
 
-                ctx->inflate_total += ctx->stream.avail_out;
-                zRC = inflate(&(ctx->stream), Z_SYNC_FLUSH);
-                ctx->inflate_total -= ctx->stream.avail_out;
-                if (zRC != Z_OK) {
-                    inflateEnd(&ctx->stream);
-                    ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01391)
-                                  "Zlib error %d inflating data (%s)", zRC,
-                                  ctx->stream.msg);
-                    return APR_EGENERAL;
-                }
+                if (!ctx->done) {
+                    ctx->inflate_total += ctx->stream.avail_out;
+                    zRC = inflate(&(ctx->stream), Z_SYNC_FLUSH);
+                    ctx->inflate_total -= ctx->stream.avail_out;
+                    if (zRC != Z_OK) {
+                        inflateEnd(&ctx->stream);
+                        ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01391)
+                                      "Zlib error %d inflating data (%s)", zRC,
+                                      ctx->stream.msg);
+                        return APR_EGENERAL;
+                    }
  
-                if (inflate_limit && ctx->inflate_total > inflate_limit) { 
-                    inflateEnd(&ctx->stream);
-                    ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(02647)
-                            "Inflated content length of %" APR_OFF_T_FMT
-                            " is larger than the configured limit"
-                            " of %" APR_OFF_T_FMT, 
-                            ctx->inflate_total, inflate_limit);
-                    return APR_ENOSPC;
-                }
-
-                if (!check_ratio(r, ctx, dc)) {
-                    inflateEnd(&ctx->stream);
-                    ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(02805)
-                            "Inflated content ratio is larger than the "
-                            "configured limit %i by %i time(s)",
-                            dc->ratio_limit, dc->ratio_burst);
-                    return APR_EINVAL;
-                }
+                    if (inflate_limit && ctx->inflate_total > inflate_limit) { 
+                        inflateEnd(&ctx->stream);
+                        ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(02647)
+                                      "Inflated content length of %" APR_OFF_T_FMT
+                                      " is larger than the configured limit"
+                                      " of %" APR_OFF_T_FMT, 
+                                      ctx->inflate_total, inflate_limit);
+                        return APR_ENOSPC;
+                    }
 
-                len = c->bufferSize - ctx->stream.avail_out;
-                ctx->crc = crc32(ctx->crc, (const Bytef *)ctx->buffer, len);
-                tmp_b = apr_bucket_heap_create((char *)ctx->buffer, len,
-                                                NULL, f->c->bucket_alloc);
-                APR_BRIGADE_INSERT_TAIL(ctx->proc_bb, tmp_b);
+                    if (!check_ratio(r, ctx, dc)) {
+                        inflateEnd(&ctx->stream);
+                        ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(02805)
+                                      "Inflated content ratio is larger than the "
+                                      "configured limit %i by %i time(s)",
+                                      dc->ratio_limit, dc->ratio_burst);
+                        return APR_EINVAL;
+                    }
 
-                ctx->stream.next_out = ctx->buffer;
-                ctx->stream.avail_out = c->bufferSize;
+                    consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out,
+                                   UPDATE_CRC, ctx->proc_bb);
+                }
 
                 /* Flush everything so far in the returning brigade, but continue
                  * reading should EOS/more follow (don't lose them).
@@ -1393,16 +1398,8 @@ static apr_status_t deflate_in_filter(ap_filter_t *f,
             if (!ctx->validation_buffer) {
                 while (ctx->stream.avail_in != 0) {
                     if (ctx->stream.avail_out == 0) {
-                        apr_bucket *tmp_heap;
-
-                        ctx->stream.next_out = ctx->buffer;
-                        len = c->bufferSize - ctx->stream.avail_out;
-
-                        ctx->crc = crc32(ctx->crc, (const Bytef *)ctx->buffer, len);
-                        tmp_heap = apr_bucket_heap_create((char *)ctx->buffer, len,
-                                                          NULL, f->c->bucket_alloc);
-                        APR_BRIGADE_INSERT_TAIL(ctx->proc_bb, tmp_heap);
-                        ctx->stream.avail_out = c->bufferSize;
+                        consume_buffer(ctx, c, c->bufferSize, UPDATE_CRC,
+                                       ctx->proc_bb);
                     }
 
                     ctx->inflate_total += ctx->stream.avail_out;
@@ -1445,7 +1442,6 @@ static apr_status_t deflate_in_filter(ap_filter_t *f,
             }
 
             if (ctx->validation_buffer) {
-                apr_bucket *tmp_heap;
                 apr_size_t avail, valid;
                 unsigned char *buf = ctx->validation_buffer;
 
@@ -1474,13 +1470,8 @@ static apr_status_t deflate_in_filter(ap_filter_t *f,
                               (apr_uint64_t)ctx->stream.total_in,
                               (apr_uint64_t)ctx->stream.total_out, r->uri);
 
-                len = c->bufferSize - ctx->stream.avail_out;
-
-                ctx->crc = crc32(ctx->crc, (const Bytef *)ctx->buffer, len);
-                tmp_heap = apr_bucket_heap_create((char *)ctx->buffer, len,
-                                                  NULL, f->c->bucket_alloc);
-                APR_BRIGADE_INSERT_TAIL(ctx->proc_bb, tmp_heap);
-                ctx->stream.avail_out = c->bufferSize;
+                consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out,
+                               UPDATE_CRC, ctx->proc_bb);
 
                 {
                     unsigned long compCRC, compLen;
@@ -1526,16 +1517,8 @@ static apr_status_t deflate_in_filter(ap_filter_t *f,
     if (block == APR_BLOCK_READ &&
             APR_BRIGADE_EMPTY(ctx->proc_bb) &&
             ctx->stream.avail_out < c->bufferSize) {
-        apr_bucket *tmp_heap;
-        apr_size_t len;
-        ctx->stream.next_out = ctx->buffer;
-        len = c->bufferSize - ctx->stream.avail_out;
-
-        ctx->crc = crc32(ctx->crc, (const Bytef *)ctx->buffer, len);
-        tmp_heap = apr_bucket_heap_create((char *)ctx->buffer, len,
-                                          NULL, f->c->bucket_alloc);
-        APR_BRIGADE_INSERT_TAIL(ctx->proc_bb, tmp_heap);
-        ctx->stream.avail_out = c->bufferSize;
+        consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out,
+                       UPDATE_CRC, ctx->proc_bb);
     }
 
     if (!APR_BRIGADE_EMPTY(ctx->proc_bb)) {
@@ -1651,7 +1634,6 @@ static apr_status_t inflate_out_filter(ap_filter_t *f,
     while (!APR_BRIGADE_EMPTY(bb))
     {
         const char *data;
-        apr_bucket *b;
         apr_size_t len;
 
         e = APR_BRIGADE_FIRST(bb);
@@ -1673,8 +1655,7 @@ static apr_status_t inflate_out_filter(ap_filter_t *f,
              * fails, whereas in the deflate case you can empty a filled output
              * buffer and call it again until no more output can be created.
              */
-            flush_libz_buffer(ctx, c, f->c->bucket_alloc, inflate, Z_SYNC_FLUSH,
-                              UPDATE_CRC);
+            flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
             ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01398)
                           "Zlib: Inflated %" APR_UINT64_T_FMT 
                           " to %" APR_UINT64_T_FMT " : URL %s",
@@ -1716,15 +1697,14 @@ static apr_status_t inflate_out_filter(ap_filter_t *f,
              * Okay, we've seen the EOS.
              * Time to pass it along down the chain.
              */
-            return ap_pass_brigade(f->next, ctx->bb);
+            rv = ap_pass_brigade(f->next, ctx->bb);
+            apr_brigade_cleanup(ctx->bb);
+            return rv;
         }
 
         if (APR_BUCKET_IS_FLUSH(e)) {
-            apr_status_t rv;
-
             /* flush the remaining data from the zlib buffers */
-            zRC = flush_libz_buffer(ctx, c, f->c->bucket_alloc, inflate,
-                                    Z_SYNC_FLUSH, UPDATE_CRC);
+            zRC = flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
             if (zRC == Z_STREAM_END) {
                 if (ctx->validation_buffer == NULL) {
                     ctx->validation_buffer = apr_pcalloc(f->r->pool,
@@ -1742,6 +1722,7 @@ static apr_status_t inflate_out_filter(ap_filter_t *f,
             APR_BUCKET_REMOVE(e);
             APR_BRIGADE_INSERT_TAIL(ctx->bb, e);
             rv = ap_pass_brigade(f->next, ctx->bb);
+            apr_brigade_cleanup(ctx->bb);
             if (rv != APR_SUCCESS) {
                 return rv;
             }
@@ -1858,16 +1839,11 @@ static apr_status_t inflate_out_filter(ap_filter_t *f,
 
         while (ctx->stream.avail_in != 0) {
             if (ctx->stream.avail_out == 0) {
-                ctx->stream.next_out = ctx->buffer;
-                len = c->bufferSize - ctx->stream.avail_out;
-
-                ctx->crc = crc32(ctx->crc, (const Bytef *)ctx->buffer, len);
-                b = apr_bucket_heap_create((char *)ctx->buffer, len,
-                                           NULL, f->c->bucket_alloc);
-                APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
-                ctx->stream.avail_out = c->bufferSize;
+                consume_buffer(ctx, c, c->bufferSize, UPDATE_CRC, ctx->bb);
+
                 /* Send what we have right now to the next filter. */
                 rv = ap_pass_brigade(f->next, ctx->bb);
+                apr_brigade_cleanup(ctx->bb);
                 if (rv != APR_SUCCESS) {
                     return rv;
                 }
@@ -1882,6 +1858,7 @@ static apr_status_t inflate_out_filter(ap_filter_t *f,
                 return APR_EGENERAL;
             }
 
+            /* Don't check length limits on inflate_out */
             if (!check_ratio(r, ctx, dc)) {
                 ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(02650)
                               "Inflated content ratio is larger than the "