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