httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Plüm, Rüdiger, Vodafone Group <ruediger.pl...@vodafone.com>
Subject AW: svn commit: r1619383 - /httpd/httpd/trunk/modules/filters/mod_deflate.c
Date Thu, 21 Aug 2014 15:08:20 GMT
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 <ylavic.dev@gmail.com>
> wrote:
> > On Thu, Aug 21, 2014 at 3:11 PM,  <covener@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?
Mime
View raw message