From 9f86c0a23de111b61a6f64579fcda58f6420e34f Mon Sep 17 00:00:00 2001 From: Eric Covener Date: Thu, 21 Aug 2014 15:39:21 +0000 Subject: [PATCH 1/6] leave a hint while scrolling through inflate() calls git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1619448 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 30939481aada84442af5450def49a6905cb82320) --- modules/filters/mod_deflate.c | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/filters/mod_deflate.c b/modules/filters/mod_deflate.c index de18606c795..682ce39810e 100644 --- a/modules/filters/mod_deflate.c +++ b/modules/filters/mod_deflate.c @@ -1882,6 +1882,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 " From cecb25ec92fcbfeb211992e0c2f724cc6cf366aa Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Thu, 21 Aug 2014 17:02:00 +0000 Subject: [PATCH 2/6] 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). git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1619486 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 2b94587206a0bbfcae16fd75caac836684a1b37f) --- modules/filters/mod_deflate.c | 145 +++++++++++++--------------------- 1 file changed, 55 insertions(+), 90 deletions(-) diff --git a/modules/filters/mod_deflate.c b/modules/filters/mod_deflate.c index 682ce39810e..c9b493d5193 100644 --- 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); @@ -945,15 +950,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 +970,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; } @@ -999,21 +1005,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; - - ctx->stream.next_out = ctx->buffer; - len = c->bufferSize - ctx->stream.avail_out; + consume_buffer(ctx, c, c->bufferSize, NO_UPDATE_CRC, ctx->bb); - 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; } @@ -1340,14 +1340,8 @@ static apr_status_t deflate_in_filter(ap_filter_t *f, return APR_EINVAL; } - 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); - - 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 +1387,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 +1431,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 +1459,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 +1506,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 +1623,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 +1644,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 +1686,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 +1711,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 +1828,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; } From 8836713e0497732876c0f5d64ebee2ff517e9b9c Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Fri, 3 Dec 2021 13:07:42 +0000 Subject: [PATCH 3/6] * 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) Github: closes #280 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1895552 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 6aef687d933fc285192dcf819bc64d0019cead53) --- modules/filters/mod_deflate.c | 60 ++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/modules/filters/mod_deflate.c b/modules/filters/mod_deflate.c index c9b493d5193..6dbcb2bf5f6 100644 --- a/modules/filters/mod_deflate.c +++ b/modules/filters/mod_deflate.c @@ -1310,38 +1310,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 (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 (!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; + } - consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out, - UPDATE_CRC, ctx->proc_bb); + 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). From b1f3978dd6eee87b6a2405f27935590ed94f9b66 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Tue, 12 Oct 2021 08:27:15 +0000 Subject: [PATCH 4/6] * modules/filters/mod_deflate.c (deflate_out_filter): Catch apr_bucket_read() errors. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1894152 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit c1a2dfc9084eeebe3e32b010f0aa92197d07c448) --- modules/filters/mod_deflate.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/modules/filters/mod_deflate.c b/modules/filters/mod_deflate.c index 6dbcb2bf5f6..754fad0ccdf 100644 --- a/modules/filters/mod_deflate.c +++ b/modules/filters/mod_deflate.c @@ -988,7 +988,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; From 9e376267f94a6eac2a9d549db662d3214d43ec35 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Wed, 20 Dec 2023 13:07:40 +0000 Subject: [PATCH 5/6] mod_deflate: remove filter after seeing EOS Submitted by: Eric Norris Github: closes #387 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1914800 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 4889f667f8c9b8f09843b7c79c12c78a0e325d07) --- modules/filters/mod_deflate.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/filters/mod_deflate.c b/modules/filters/mod_deflate.c index 754fad0ccdf..5a541e73877 100644 --- a/modules/filters/mod_deflate.c +++ b/modules/filters/mod_deflate.c @@ -940,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); From 82a2fb780414540b7f43ae74d458a6321b08588a Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 18 Mar 2024 16:19:26 +0000 Subject: [PATCH 6/6] Document changes. --- changes-entries/deflate-cleanups.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes-entries/deflate-cleanups.txt diff --git a/changes-entries/deflate-cleanups.txt b/changes-entries/deflate-cleanups.txt new file mode 100644 index 00000000000..18a32a61062 --- /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 ] +