You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Christophe JAILLET <ch...@wanadoo.fr> on 2014/10/14 14:22:28 UTC
Re: svn commit: r1628919 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_substitute.c
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?
>
> #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 ?
> 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' ?
> + }
> + dcfg->max_line_length = (apr_size_t)max;
> + dcfg->max_line_length_set = 1;
> + return NULL;
> +}
> +
>
Re: svn commit: r1628919 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_substitute.c
Posted by Marion & Christophe JAILLET <ch...@wanadoo.fr>.
Le 14/10/2014 19:55, Rainer Jung a écrit :
> 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.
Up to you.
When I looked at it, I grep'ed source code and the first match was:
./srclib/apr/passwd/apr_getpass.c:80:#define MAX_STRING_LEN 256
Only looking at the #define (and not at the file!) I first thought that
doc was not in lone with code.
later on, I found the correct one in httpd.h :)
>
> 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?
>
OK, understood.
+1 for a comment if someone, one day, notices it and tries understand if
it is a mistake or not.
>>> + 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.
>
Was just a minor issue.
Re: svn commit: r1628919 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_substitute.c
Posted by Rainer Jung <ra...@kippdata.de>.
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