On Thu, Aug 21, 2014 at 3:11 PM, <[email protected]> wrote:
> Author: covener
> Date: Thu Aug 21 13:11:15 2014
> New Revision: 1619383
>
> URL: http://svn.apache.org/r1619383
> Log:
> A misplaced check for inflation limits prevented limiting relatively
> small inputs. PR56872
>
> Submitted By: Edward Lu
> Committed By: covener
>
[...]
> Modified: httpd/httpd/trunk/modules/filters/mod_deflate.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_deflate.c?rev=1619383&r1=1619382&r2=1619383&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_deflate.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_deflate.c Thu Aug 21 13:11:15 2014
[...]
> @@ -1398,6 +1378,27 @@ static apr_status_t deflate_in_filter(ap
> }
>
> zRC = inflate(&ctx->stream, Z_NO_FLUSH);
> + len = c->bufferSize - ctx->stream.avail_out;
> +
> + ctx->inflate_total += len;
Does the inflate() call garantee to fill the output buffer entirely
whenever it has enough inputs (ie. after the call, either avail_in or
avail_out is zero)?
I can't find any clue in the docs nor in the (huge-state-machine-)code.
Otherwise, I think we'd better compute the number of bytes really
produced by the inflate(), for each loop, that is :
len = ctx->stream.avail_out;
zRC = inflate(&ctx->stream, Z_NO_FLUSH);
len -= ctx->stream.avail_out;
or we way count the same bytes multiple times.
> + if (inflate_limit && ctx->inflate_total > inflate_limit)
> {
> + inflateEnd(&ctx->stream);
> + ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
> APLOGNO(02648)
> + "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(02649)
> + "Inflated content ratio is larger than the "
> + "configured limit %i by %i time(s)",
> + dc->ratio_limit, dc->ratio_burst);
> + return APR_EINVAL;
> + }
>
> if (zRC == Z_STREAM_END) {
> ctx->validation_buffer = apr_pcalloc(r->pool,
>
>
I think the following test (from the sources, not included in the diff) :
if (zRC != Z_OK) {
inflateEnd(&ctx->stream);
ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
APLOGNO(01392)
"Zlib error %d inflating data (%s)", zRC,
ctx->stream.msg);
return APR_EGENERAL;
}
should probably be moved just after the inflate() call, so to check
errors before limits.
It would then become :
zRC = inflate(&ctx->stream, Z_NO_FLUSH);
if (zRC != Z_OK && zRC != Z_STREAM_END) {
...
return APR_EGENERAL;
}
if (inflate_limit && ctx->inflate_total > inflate_limit) {
...
}
if (!check_ratio(r, ctx, dc)) {
...
}
if (zRC == Z_STREAM_END) {
...
break;
}
All this changes are in the attached patch (which is often more clear).
The patch also fixes the same issue regarding the check_ratio() call
in inflate_out_filter(), and adds a missing check_ratio() in
deflate_in_filter() when a FLUSH bucket is encountered (still
following the existing inflate_limit check).
Should I apply it?
