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