httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: Fwd: svn commit: r1197405 - in /httpd/httpd/trunk: CHANGES docs/manual/upgrading.xml modules/filters/mod_substitute.c
Date Sun, 06 Nov 2011 15:47:09 GMT


On 11/04/2011 05:04 PM, Stefan Fritsch wrote:
> 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.

Fair enough.

> 
>>                             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?
>>
>>


Any comment on that?

Regards

Rüdiger


Mime
View raw message