Skip to content

Commit

Permalink
Merge r1619448, r1619486, r1895552, r1894152, r1914800 from trunk:
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@notroj
notroj committed Mar 26, 2024
1 parent 9ab08c3 commit 5905f0f
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 118 deletions.
4 changes: 4 additions & 0 deletions changes-entries/deflate-cleanups.txt
View file
Original file line number Diff line number Diff line change
@@ -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>]

213 changes: 95 additions & 118 deletions modules/filters/mod_deflate.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand All @@ -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)",
Expand All @@ -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;
}
Expand All @@ -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;
Expand All @@ -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;
}
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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);
Expand All @@ -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",
Expand Down Expand Up @@ -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,
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand All @@ -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 "
Expand Down

0 comments on commit 5905f0f

Please sign in to comment.