httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fritsch ...@sfritsch.de>
Subject Re: Fwd: svn commit: r1197405 - in /httpd/httpd/trunk: CHANGES docs/manual/upgrading.xml modules/filters/mod_substitute.c
Date Fri, 04 Nov 2011 16:04:10 GMT
On Fri, 4 Nov 2011, Rüdiger Plüm wrote:
> Modified: httpd/httpd/trunk/modules/filters/mod_substitute.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_substitute.c?rev=1197405&r1=1197404&r2=1197405&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_substitute.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_substitute.c Fri Nov  4 05:35:53 
> 2011
> @@ -158,6 +164,9 @@ static void do_pattmatch(ap_filter_t *f,
>                              * as its own bucket, then isolate the pattern
>                              * and delete it.
>                              */
> +                            if (space_left<  len + repl_len)
> +                                return APR_ENOMEM;
> +                            space_left -= len + repl_len;
>
>
> Why is this needed at all? IMHO we only consume memory by creating additional 
> bucket structures,
> but this does not depend on the length of the replacement strings.
> As we use transient buckets below we use the same data (script->replacement) 
> over and over
> again for new buckets.

I wanted the behavior to be more predictable for the user. For a single 
rule, we always force quick mode. IMHO, it would be confusing if the 
line-too-long error appears just because the user added a second rule. We 
could make the length limit only apply if quick mode has not been 
explicitly set. But the bucket structures can also use significant memory 
(as demonstrated by the range issue). Having the limit not depend on the 
number of buckets and therefore on the content of the line but only on the 
line length is more comprehensible, IMHO.

>                             SEDRMPATBCKT(b, len, tmp_b, script->patlen);
>                             /*
>                              * Finally, we create a bucket that contains the
> @@ -188,25 +197,36 @@ static void do_pattmatch(ap_filter_t *f,
>                     int left = bytes;
>                     const char *pos = buff;
>                     char *repl;
> +                    apr_size_t space_left = AP_SUBST_MAX_LINE_LENGTH;
>                     while (!ap_regexec_len(script->regexp, pos, left,
>                                        AP_MAX_REG_MATCH, regm, 0)) {
> +                        apr_status_t rv;
>                         have_match = 1;
>                         if (script->flatten&&  !force_quick) {
>                             /* copy bytes before the match */
>                             if (regm[0].rm_so>  0)
>                                 ap_varbuf_strmemcat(&vb, pos, 
> regm[0].rm_so);
>                             /* add replacement string */
> -                            ap_varbuf_regsub(&vb, script->replacement, pos,
> -                                             AP_MAX_REG_MATCH, regm, 0);
> +                            rv = ap_varbuf_regsub(&vb, script->replacement, 
> pos,
> +                                                  AP_MAX_REG_MATCH, regm,
> +                                                  AP_SUBST_MAX_LINE_LENGTH - 
> vb.strlen);
> +                            if (rv != APR_SUCCESS)
> +                                return rv;
>                         }
>                         else {
> -                            ap_pregsub_ex(pool,&repl, script->replacement, 
> pos,
> -                                              AP_MAX_REG_MATCH, regm, 0);
> +                            apr_size_t repl_len;
> +                            rv = ap_pregsub_ex(pool,&repl,
> +                                               script->replacement, pos,
> +                                               AP_MAX_REG_MATCH, regm,
> +                                               space_left);
> +                            if (rv != APR_SUCCESS)
> +                                return rv;
>                             len = (apr_size_t) (regm[0].rm_eo - 
> regm[0].rm_so);
> +                            repl_len = strlen(repl);
> +                            space_left -= len + repl_len;
>
>
> Isn't it sufficient to reduce space_left only by repl_len?
> IMHO we only allocate repl_len new memory in ap_pregsub_ex and not len + 
> repl_len or am I wrong here?
>
>
>
>                             SEDRMPATBCKT(b, regm[0].rm_so, tmp_b, len);
> -                            tmp_b = apr_bucket_transient_create(repl,
> -                                             strlen(repl),
> - 
> f->r->connection->bucket_alloc);
> +                            tmp_b = apr_bucket_transient_create(repl, 
> repl_len,
> + 
> f->r->connection->bucket_alloc);
>                             APR_BUCKET_INSERT_BEFORE(b, tmp_b);
>                         }
>                         /*
>
> @@ -414,9 +450,8 @@ static apr_status_t substitute_filter(ap
>                             rv = ap_pass_brigade(f->next, ctx->passbb);
>                             apr_brigade_cleanup(ctx->passbb);
>                             num = 0;
> -                            apr_pool_clear(ctx->tpool);
>
>
>
> Why don't we clear this pool any longer? It was cleared in both cases:
> The error one and the none error one.
> And calling  apr_pool_clear twice does not harm.

Good catch. I will fix this shortly. As always, your review is very 
welcome.

Cheers,
Stefan
Mime
  • Unnamed multipart/mixed (inline, None, 0 bytes)
View raw message