You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by sf...@apache.org on 2011/10/26 00:29:14 UTC
svn commit: r1188950 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h
include/httpd.h include/util_varbuf.h modules/filters/mod_substitute.c
server/util.c
Author: sf
Date: Tue Oct 25 22:29:13 2011
New Revision: 1188950
URL: http://svn.apache.org/viewvc?rev=1188950&view=rev
Log:
Limit ap_pregsub() to 64K, add ap_pregsub_ex() for longer strings and with
better error reporting. Modify ap_varbuf_regsub() to be similar to
ap_pregsub_ex().
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/include/ap_mmn.h
httpd/httpd/trunk/include/httpd.h
httpd/httpd/trunk/include/util_varbuf.h
httpd/httpd/trunk/modules/filters/mod_substitute.c
httpd/httpd/trunk/server/util.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1188950&r1=1188949&r2=1188950&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Tue Oct 25 22:29:13 2011
@@ -12,6 +12,9 @@ Changes with Apache 2.3.15
PR 51714. [Stefan Fritsch, Jim Jagielski, Ruediger Pluem, Eric Covener,
<lowprio20 gmail.com>]
+ *) core: Limit ap_pregsub() to 64K, add ap_pregsub_ex() for longer strings.
+ [Stefan Fritsch]
+
*) mod_session_crypto: Refactor to support the new apr_crypto API.
[Graham Leggett]
Modified: httpd/httpd/trunk/include/ap_mmn.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ap_mmn.h?rev=1188950&r1=1188949&r2=1188950&view=diff
==============================================================================
--- httpd/httpd/trunk/include/ap_mmn.h (original)
+++ httpd/httpd/trunk/include/ap_mmn.h Tue Oct 25 22:29:13 2011
@@ -360,12 +360,14 @@
* add ap_unixd_config.group_name
* 20111014.0 (2.3.15-dev) Remove cookie_path_str and cookie_domain_str from
* proxy_dir_conf
+ * 20111025.0 (2.3.15-dev) Add return value and maxlen to ap_varbuf_regsub(),
+ * add ap_pregsub_ex()
*/
#define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */
#ifndef MODULE_MAGIC_NUMBER_MAJOR
-#define MODULE_MAGIC_NUMBER_MAJOR 20111014
+#define MODULE_MAGIC_NUMBER_MAJOR 20111025
#endif
#define MODULE_MAGIC_NUMBER_MINOR 0 /* 0...n */
Modified: httpd/httpd/trunk/include/httpd.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/httpd.h?rev=1188950&r1=1188949&r2=1188950&view=diff
==============================================================================
--- httpd/httpd/trunk/include/httpd.h (original)
+++ httpd/httpd/trunk/include/httpd.h Tue Oct 25 22:29:13 2011
@@ -1775,7 +1775,8 @@ AP_DECLARE(void) ap_pregfree(apr_pool_t
/**
* After performing a successful regex match, you may use this function to
* perform a series of string substitutions based on subexpressions that were
- * matched during the call to ap_regexec
+ * matched during the call to ap_regexec. This function is limited to
+ * result strings of 64K. Consider using ap_pregsub_ex() instead.
* @param p The pool to allocate from
* @param input An arbitrary string containing $1 through $9. These are
* replaced with the corresponding matched sub-expressions
@@ -1788,6 +1789,25 @@ AP_DECLARE(char *) ap_pregsub(apr_pool_t
size_t nmatch, ap_regmatch_t pmatch[]);
/**
+ * After performing a successful regex match, you may use this function to
+ * perform a series of string substitutions based on subexpressions that were
+ * matched during the call to ap_regexec
+ * @param p The pool to allocate from
+ * @param result where to store the result, will be set to NULL on error
+ * @param input An arbitrary string containing $1 through $9. These are
+ * replaced with the corresponding matched sub-expressions
+ * @param source The string that was originally matched to the regex
+ * @param nmatch the nmatch returned from ap_pregex
+ * @param pmatch the pmatch array returned from ap_pregex
+ * @param maxlen the maximum string length to return
+ * @return The substituted string, or NULL on error
+ */
+AP_DECLARE(apr_status_t) ap_pregsub_ex(apr_pool_t *p, char **result,
+ const char *input, const char *source,
+ size_t nmatch, ap_regmatch_t pmatch[],
+ apr_size_t maxlen);
+
+/**
* We want to downcase the type/subtype for comparison purposes
* but nothing else because ;parameter=foo values are case sensitive.
* @param s The content-type to convert to lowercase
Modified: httpd/httpd/trunk/include/util_varbuf.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/util_varbuf.h?rev=1188950&r1=1188949&r2=1188950&view=diff
==============================================================================
--- httpd/httpd/trunk/include/util_varbuf.h (original)
+++ httpd/httpd/trunk/include/util_varbuf.h Tue Oct 25 22:29:13 2011
@@ -135,13 +135,17 @@ AP_DECLARE(char *) ap_varbuf_pdup(apr_po
* @param source The string that was originally matched to the regex
* @param nmatch the nmatch returned from ap_pregex
* @param pmatch the pmatch array returned from ap_pregex
+ * @param maxlen the maximum string length to append to vb
+ * @return APR_SUCCESS if successful
* @note Just like ap_pregsub(), this function does not copy the part of
* *source before the matching part (i.e. the first pmatch[0].rm_so
* characters).
*/
-AP_DECLARE(void) ap_varbuf_regsub(struct ap_varbuf *vb, const char *input,
- const char *source, size_t nmatch,
- ap_regmatch_t pmatch[]);
+AP_DECLARE(apr_status_t) ap_varbuf_regsub(struct ap_varbuf *vb,
+ const char *input,
+ const char *source, size_t nmatch,
+ ap_regmatch_t pmatch[],
+ apr_size_t maxlen);
/** Read a line from an ap_configfile_t into an ap_varbuf.
* @param vb pointer to the ap_varbuf struct
Modified: httpd/httpd/trunk/modules/filters/mod_substitute.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_substitute.c?rev=1188950&r1=1188949&r2=1188950&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/filters/mod_substitute.c (original)
+++ httpd/httpd/trunk/modules/filters/mod_substitute.c Tue Oct 25 22:29:13 2011
@@ -196,7 +196,8 @@ static void do_pattmatch(ap_filter_t *f,
ap_varbuf_strmemcat(&vb, pos, regm[0].rm_so);
/* add replacement string */
ap_varbuf_regsub(&vb, script->replacement, pos,
- AP_MAX_REG_MATCH, regm);
+ AP_MAX_REG_MATCH, regm,
+ APR_SIZE_MAX);
}
else {
repl = ap_pregsub(pool, script->replacement, pos,
Modified: httpd/httpd/trunk/server/util.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1188950&r1=1188949&r2=1188950&view=diff
==============================================================================
--- httpd/httpd/trunk/server/util.c (original)
+++ httpd/httpd/trunk/server/util.c Tue Oct 25 22:29:13 2011
@@ -370,32 +370,35 @@ AP_DECLARE(const char *) ap_stripprefix(
* AT&T V8 regexp package.
*/
-static char *regsub_core(apr_pool_t *p, struct ap_varbuf *vb,
- const char *input, const char *source,
- size_t nmatch, ap_regmatch_t pmatch[])
+static apr_status_t regsub_core(apr_pool_t *p, char **result,
+ struct ap_varbuf *vb, const char *input,
+ const char *source, size_t nmatch,
+ ap_regmatch_t pmatch[], apr_size_t maxlen)
{
const char *src = input;
- char *dest, *dst;
+ char *dst;
char c;
size_t no;
- int len;
+ apr_size_t len = 0;
+ AP_DEBUG_ASSERT((result && p && !vb) || (vb && !p && !result));
if (!source)
- return NULL;
+ return APR_EINVAL;
if (!nmatch || nmatch>AP_MAX_REG_MATCH) {
+ len = strlen(src);
+ if (maxlen > 0 && len > maxlen)
+ return APR_ENOMEM;
if (!vb) {
- return apr_pstrdup(p, src);
+ *result = apr_pstrmemdup(p, src, len);
+ return APR_SUCCESS;
}
else {
- ap_varbuf_strcat(vb, src);
- return NULL;
+ ap_varbuf_strmemcat(vb, src, len);
+ return APR_SUCCESS;
}
}
/* First pass, find the size */
-
- len = 0;
-
while ((c = *src++) != '\0') {
if (c == '$' && apr_isdigit(*src))
no = *src++ - '0';
@@ -413,14 +416,17 @@ static char *regsub_core(apr_pool_t *p,
}
+ if (len > maxlen && maxlen > 0)
+ return APR_ENOMEM;
+
if (!vb) {
- dest = dst = apr_pcalloc(p, len + 1);
+ *result = dst = apr_pcalloc(p, len + 1);
}
else {
if (vb->buf && vb->strlen == AP_VARBUF_UNKNOWN)
vb->strlen = strlen(vb->buf);
ap_varbuf_grow(vb, vb->strlen + len);
- dest = dst = vb->buf + vb->strlen;
+ dst = vb->buf + vb->strlen;
vb->strlen += len;
}
@@ -450,14 +456,34 @@ static char *regsub_core(apr_pool_t *p,
}
*dst = '\0';
- return dest;
+ return APR_SUCCESS;
}
+#ifndef AP_PREGSUB_MAXLEN
+#define AP_PREGSUB_MAXLEN 65536
+#endif
AP_DECLARE(char *) ap_pregsub(apr_pool_t *p, const char *input,
const char *source, size_t nmatch,
ap_regmatch_t pmatch[])
{
- return regsub_core(p, NULL, input, source, nmatch, pmatch);
+ char *result;
+ apr_status_t rc = regsub_core(p, &result, NULL, input, source, nmatch,
+ pmatch, AP_PREGSUB_MAXLEN);
+ if (rc != APR_SUCCESS)
+ result = NULL;
+ return result;
+}
+
+AP_DECLARE(apr_status_t) ap_pregsub_ex(apr_pool_t *p, char **result,
+ const char *input, const char *source,
+ size_t nmatch, ap_regmatch_t pmatch[],
+ apr_size_t maxlen)
+{
+ apr_status_t rc = regsub_core(p, result, NULL, input, source, nmatch,
+ pmatch, maxlen);
+ if (rc != APR_SUCCESS)
+ *result = NULL;
+ return rc;
}
/*
@@ -2647,11 +2673,13 @@ AP_DECLARE(char *) ap_varbuf_pdup(apr_po
return "";
}
-AP_DECLARE(void) ap_varbuf_regsub(struct ap_varbuf *vb, const char *input,
- const char *source, size_t nmatch,
- ap_regmatch_t pmatch[])
+AP_DECLARE(apr_status_t) ap_varbuf_regsub(struct ap_varbuf *vb,
+ const char *input,
+ const char *source, size_t nmatch,
+ ap_regmatch_t pmatch[],
+ apr_size_t maxlen)
{
- regsub_core(NULL, vb, input, source, nmatch, pmatch);
+ return regsub_core(NULL, NULL, vb, input, source, nmatch, pmatch, maxlen);
}
static const char * const oom_message = "[crit] Memory allocation failed, "
Re: svn commit: r1188950 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h include/util_varbuf.h modules/filters/mod_substitute.c server/util.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Oct 27, 2011, at 4:21 PM, Stefan Fritsch wrote:
>
> That's correct, it would crash if len == APR_SIZE_MAX. But my point
> was that it would also crash for len == APR_SIZE_MAX-1000, because by
> definition, the machine cannot have that much free mem and apr_pcalloc
> would call abort(). So in both cases, the amount of free memory is the
> limit. But aborting with out-of-mem is more correct than simply
> segfaulting, therefore I have fixed it.
Worrying about what the machine would do and what the code
would do are 2 different things. The code is obviously broken
should len == APR_SIZE_MAX, no matter whether or not
the machine allows it or not, and that is, imo, unacceptable.
Re: svn commit: r1188950 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h include/util_varbuf.h modules/filters/mod_substitute.c server/util.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Thursday 27 October 2011, Jim Jagielski wrote:
> On Oct 26, 2011, at 3:46 PM, Stefan Fritsch wrote:
> > On Wednesday 26 October 2011, Jim Jagielski wrote:
> >> On Oct 25, 2011, at 6:29 PM, sf@apache.org wrote:
> >>> + if (len > maxlen && maxlen > 0)
> >>> + return APR_ENOMEM;
> >>> +
> >>>
> >>> if (!vb) {
> >>>
> >>> - dest = dst = apr_pcalloc(p, len + 1);
> >>> + *result = dst = apr_pcalloc(p, len + 1);
> >>
> >> if len == maxlen and == APR_SIZE_MAX then doesn't
> >> the len+1 blow us up?
> >
> > APR_SIZE_MAX means no limit, i.e. it will blow up when there is
> > no memory left, which will be well before APR_SIZE_MAX. I guess
> > I should have used 0, that would have been clearer.
>
> I thought that
>
> #define APR_SIZE_MAX (~((apr_size_t)0))
>
> returned the one's complement of 0, which would basically
> be the largest value possible for (apr_size_t)0, and that
> adding 1 to it would overflow… right? Or am I still jet-lagged
> (which is quite possible).
That's correct, it would crash if len == APR_SIZE_MAX. But my point
was that it would also crash for len == APR_SIZE_MAX-1000, because by
definition, the machine cannot have that much free mem and apr_pcalloc
would call abort(). So in both cases, the amount of free memory is the
limit. But aborting with out-of-mem is more correct than simply
segfaulting, therefore I have fixed it.
> The main question is whether or not maxlen accounts for
> the NUL or not; we seem inconsistent in that regard with
> this patch.
True. I think this is also fixed with r1189985.
Re: svn commit: r1188950 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h include/util_varbuf.h modules/filters/mod_substitute.c server/util.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Oct 26, 2011, at 3:46 PM, Stefan Fritsch wrote:
> On Wednesday 26 October 2011, Jim Jagielski wrote:
>> On Oct 25, 2011, at 6:29 PM, sf@apache.org wrote:
>>> + if (len > maxlen && maxlen > 0)
>>> + return APR_ENOMEM;
>>> +
>>>
>>> if (!vb) {
>>>
>>> - dest = dst = apr_pcalloc(p, len + 1);
>>> + *result = dst = apr_pcalloc(p, len + 1);
>>
>> if len == maxlen and == APR_SIZE_MAX then doesn't
>> the len+1 blow us up?
>
> APR_SIZE_MAX means no limit, i.e. it will blow up when there is no
> memory left, which will be well before APR_SIZE_MAX. I guess I should
> have used 0, that would have been clearer.
>
I thought that
#define APR_SIZE_MAX (~((apr_size_t)0))
returned the one's complement of 0, which would basically
be the largest value possible for (apr_size_t)0, and that
adding 1 to it would overflow… right? Or am I still jet-lagged
(which is quite possible).
The main question is whether or not maxlen accounts for
the NUL or not; we seem inconsistent in that regard with
this patch.
Re: svn commit: r1188950 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h include/util_varbuf.h modules/filters/mod_substitute.c server/util.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Wednesday 26 October 2011, Jim Jagielski wrote:
> On Oct 25, 2011, at 6:29 PM, sf@apache.org wrote:
> > + if (len > maxlen && maxlen > 0)
> > + return APR_ENOMEM;
> > +
> >
> > if (!vb) {
> >
> > - dest = dst = apr_pcalloc(p, len + 1);
> > + *result = dst = apr_pcalloc(p, len + 1);
>
> if len == maxlen and == APR_SIZE_MAX then doesn't
> the len+1 blow us up?
APR_SIZE_MAX means no limit, i.e. it will blow up when there is no
memory left, which will be well before APR_SIZE_MAX. I guess I should
have used 0, that would have been clearer.
Re: svn commit: r1188950 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h include/util_varbuf.h modules/filters/mod_substitute.c server/util.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Oct 25, 2011, at 6:29 PM, sf@apache.org wrote:
>
> + if (len > maxlen && maxlen > 0)
> + return APR_ENOMEM;
> +
> if (!vb) {
> - dest = dst = apr_pcalloc(p, len + 1);
> + *result = dst = apr_pcalloc(p, len + 1);
if len == maxlen and == APR_SIZE_MAX then doesn't
the len+1 blow us up?