Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (nike.apache.org: domain of rainer.jung@kippdata.de
 designates 195.227.30.149 as permitted sender)
Message-ID: <543D6381.6060103@kippdata.de>
Date: Tue, 14 Oct 2014 19:55:13 +0200
From: Rainer Jung <rainer.jung@kippdata.de>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:31.0) Gecko/20100101 Thunderbird/31.1.2
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Re: svn commit: r1628919 - in /httpd/httpd/trunk: CHANGES
 modules/filters/mod_substitute.c
References: <20141002095025.6A90F23889E2@eris.apache.org>
 <543D1584.3010602@wanadoo.fr>
In-Reply-To: <543D1584.3010602@wanadoo.fr>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

Am 14.10.2014 um 14:22 schrieb Christophe JAILLET:
> Hi,
>
> this patch is in the backport proposal for 2.4.x.
>
> See my remarks below.
> The only one that worse it is the one for comparison on new varbuf
> length either with > or with >=
>
> Best regards,
> CJ
>
>
> Le 02/10/2014 11:50, rjung@apache.org a écrit :
>> Author: rjung
>> Date: Thu Oct  2 09:50:24 2014
>> New Revision: 1628919
>>
>> URL: http://svn.apache.org/r1628919
>> Log:
>> mod_substitute: Make maximum line length configurable.
>>
>> Modified:
>>      httpd/httpd/trunk/CHANGES
>>      httpd/httpd/trunk/modules/filters/mod_substitute.c
>>
>> Modified: httpd/httpd/trunk/modules/filters/mod_substitute.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_substitute.c?rev=1628919&r1=1628918&r2=1628919&view=diff
>>
>> ==============================================================================
>>
>> --- httpd/httpd/trunk/modules/filters/mod_substitute.c (original)
>> +++ httpd/httpd/trunk/modules/filters/mod_substitute.c Thu Oct  2
>> 09:50:24 2014
>> @@ -33,6 +33,13 @@
>>   #define APR_WANT_STRFUNC
>>   #include "apr_want.h"
>> +/*
>> + * We want to limit the memory usage in a way that is predictable.
>> + * Therefore we limit the resulting length of the line.
>> + * This is the default value.
>> + */
>> +#define AP_SUBST_MAX_LINE_LENGTH (128*MAX_STRING_LEN)
>
> Why not use directly 1048576 or (1024*1024) or MBYTE defined below),
> should MAX_STRING_LEN change one day?

I'm totally fine with either of your proposals. The chosen form was what 
I found in the code and I didn't want to do an unrelated change. but 
yes, i also had to first find out what the MAX_STRING_LEN is, befor I 
knew, what the actual value was. So setting it to some fixed value is 
clearer. +1 to either 1024*1024 or 1048576.

>>   #define SEDRMPATBCKT(b, offset, tmp_b, patlen) do {  \
>>       apr_bucket_split(b, offset);                     \
>> @@ -143,9 +152,9 @@ static apr_status_t do_pattmatch(ap_filt
>>                       const char *repl;
>>                       /*
>>                        * space_left counts how many bytes we have left
>> until the
>> -                     * line length reaches AP_SUBST_MAX_LINE_LENGTH.
>> +                     * line length reaches max_line_length.
>>                        */
>> -                    apr_size_t space_left = AP_SUBST_MAX_LINE_LENGTH;
>> +                    apr_size_t space_left = cfg->max_line_length;
>>                       apr_size_t repl_len = strlen(script->replacement);
>>                       while ((repl = apr_strmatch(script->pattern,
>> buff, bytes)))
>>                       {
>> @@ -161,7 +170,7 @@ static apr_status_t do_pattmatch(ap_filt
>>                                * are constanting allocing space and
>> copying
>>                                * strings.
>>                                */
>> -                            if (vb.strlen + len + repl_len >
>> AP_SUBST_MAX_LINE_LENGTH)
>> +                            if (vb.strlen + len + repl_len >
>> cfg->max_line_length)
>
> why > there...
>
>>                                   return APR_ENOMEM;
>>                               ap_varbuf_strmemcat(&vb, buff, len);
>>                               ap_varbuf_strmemcat(&vb,
>> script->replacement, repl_len);
>> @@ -228,21 +237,21 @@ static apr_status_t do_pattmatch(ap_filt
>>                       int left = bytes;
>>                       const char *pos = buff;
>>                       char *repl;
>> -                    apr_size_t space_left = AP_SUBST_MAX_LINE_LENGTH;
>> +                    apr_size_t space_left = cfg->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 (vb.strlen + regm[0].rm_so >=
>> AP_SUBST_MAX_LINE_LENGTH)
>> +                            if (vb.strlen + regm[0].rm_so >=
>> cfg->max_line_length)
>
> and >= here ?

That block is followed by first copying regm[0].rm_so to the end of vb 
and then

rv = ap_varbuf_regsub(&vb, script->replacement, pos, AP_MAX_REG_MATCH, 
regm, cfg->max_line_length - vb.strlen);

If we start with vb.strlen + regm[0].rm_so == cfg->max_line_length, then 
after appending regm[0].rm_so we have vb.strlen == cfg->max_line_length 
and the last param in ap_varbuf_regsub() gets 0. But a 0 there does not 
mean at most 0 bytes, but instead unlimited number of bytes :(

So yes, we could change the condition to a ">", but we would then need 
to skip over the ap_varbuf_regsub() call below. I think we can keep as 
is but I should add a comment about that special case. OK?

>>                                   return APR_ENOMEM;
>>                               if (regm[0].rm_so > 0)
>>                                   ap_varbuf_strmemcat(&vb, pos,
>> regm[0].rm_so);
>> @@ -629,6 +641,44 @@ static const char *set_pattern(cmd_parms
>>       return NULL;
>>   }
>> +#define KBYTE         1024
>> +#define MBYTE         1048576
>> +#define GBYTE         1073741824
>> +
>> +static const char *set_max_line_length(cmd_parms *cmd, void *cfg,
>> const char *arg)
>> +{
>> +    subst_dir_conf *dcfg = (subst_dir_conf *)cfg;
>> +    apr_off_t max;
>> +    char *end;
>> +    apr_status_t rv;
>> +
>> +    rv = apr_strtoff(&max, arg, &end, 10);
>> +    if (rv == APR_SUCCESS) {
>> +        if ((*end == 'K' || *end == 'k') && !end[1]) {
>> +            max *= KBYTE;
>> +        }
>> +        else if ((*end == 'M' || *end == 'm') && !end[1]) {
>> +            max *= MBYTE;
>> +        }
>> +        else if ((*end == 'G' || *end == 'g') && !end[1]) {
>> +            max *= GBYTE;
>> +        }
>> +        else if (*end && /* neither empty nor [Bb] */
>> +                 ((*end != 'B' && *end != 'b') || end[1])) {
>> +            rv = APR_EGENERAL;
>> +        }
>> +    }
>> +
>> +    if (rv != APR_SUCCESS || max < 0)
>> +    {
>> +        return "SubstituteMaxLineLength must be a non-negative
>> integer optionally "
>> +               "suffixed with 'k', 'm' or 'g'.";
>
> and 'b' ?

You are right, I probably added the 'b'later while working on it and 
forgot to update the description text.

>> +    }
>> +    dcfg->max_line_length = (apr_size_t)max;
>> +    dcfg->max_line_length_set = 1;
>> +    return NULL;
>> +}
>> +

As always thanks a bunch for your review.

Rainer