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, rj...@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=1628919r1=1628918r2=1628919view=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
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, rj...@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=1628919r1=1628918r2=1628919view=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
Re: svn commit: r1628919 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_substitute.c
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, rj...@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=1628919r1=1628918r2=1628919view=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.
