The (missing) patch... On Thu, Aug 21, 2014 at 4:34 PM, Yann Ylavic <[email protected]> wrote: > 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?
httpd-trunk-mod_deflate-limits.patch
Description: application/download
