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
