You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2014/08/21 16:34:43 UTC
Re: svn commit: r1619383 - /httpd/httpd/trunk/modules/filters/mod_deflate.c
On Thu, Aug 21, 2014 at 3:11 PM, <co...@apache.org> 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?
Re: svn commit: r1619383 - /httpd/httpd/trunk/modules/filters/mod_deflate.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Aug 21, 2014 at 5:35 PM, Eric Covener <co...@gmail.com> wrote:
> On Thu, Aug 21, 2014 at 11:25 AM, Eric Covener <co...@gmail.com> wrote:
>> On Thu, Aug 21, 2014 at 10:34 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>> Should I apply it?
>>
>> yes, I think I see now that the removal is wrong and the other stuff
>> is more readable.
>>
>> But I am a bit confused as to why we check only the ratio sometimes
>> (in the blocks when moving the full zlib buffer to the heap bucket).
>> Is it intentional?
>
> I see now from your commit message -- ratio-only checks are deflate_out.
Yes, no size limit on the outgoing body.
Re: svn commit: r1619383 - /httpd/httpd/trunk/modules/filters/mod_deflate.c
Posted by Eric Covener <co...@gmail.com>.
On Thu, Aug 21, 2014 at 11:25 AM, Eric Covener <co...@gmail.com> wrote:
> On Thu, Aug 21, 2014 at 10:34 AM, Yann Ylavic <yl...@gmail.com> wrote:
>> Should I apply it?
>
> yes, I think I see now that the removal is wrong and the other stuff
> is more readable.
>
> But I am a bit confused as to why we check only the ratio sometimes
> (in the blocks when moving the full zlib buffer to the heap bucket).
> Is it intentional?
I see now from your commit message -- ratio-only checks are deflate_out.
--
Eric Covener
covener@gmail.com
Re: svn commit: r1619383 - /httpd/httpd/trunk/modules/filters/mod_deflate.c
Posted by Eric Covener <co...@gmail.com>.
On Thu, Aug 21, 2014 at 10:34 AM, Yann Ylavic <yl...@gmail.com> wrote:
> Should I apply it?
yes, I think I see now that the removal is wrong and the other stuff
is more readable.
But I am a bit confused as to why we check only the ratio sometimes
(in the blocks when moving the full zlib buffer to the heap bucket).
Is it intentional?
--
Eric Covener
covener@gmail.com
AW: svn commit: r1619383 -
/httpd/httpd/trunk/modules/filters/mod_deflate.c
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
Should be fine.
Regards
Rüdiger
> -----Ursprüngliche Nachricht-----
> Von: Yann Ylavic [mailto:ylavic.dev@gmail.com]
> Gesendet: Donnerstag, 21. August 2014 16:37
> An: httpd
> Betreff: Re: svn commit: r1619383 -
> /httpd/httpd/trunk/modules/filters/mod_deflate.c
>
> The (missing) patch...
>
> On Thu, Aug 21, 2014 at 4:34 PM, Yann Ylavic <yl...@gmail.com>
> wrote:
> > On Thu, Aug 21, 2014 at 3:11 PM, <co...@apache.org> 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_defla
> te.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?
Re: svn commit: r1619383 - /httpd/httpd/trunk/modules/filters/mod_deflate.c
Posted by Yann Ylavic <yl...@gmail.com>.
The (missing) patch...
On Thu, Aug 21, 2014 at 4:34 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Aug 21, 2014 at 3:11 PM, <co...@apache.org> 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?