Ruediger Pluem wrote:
On 07/19/2009 01:12 AM, [email protected] wrote:
Author: niq
Date: Sat Jul 18 23:12:58 2009
New Revision: 795445
URL: http://svn.apache.org/viewvc?rev=795445&view=rev
Log:
Fix mod_include potential segfault checking backref from unmatched regexp
http://markmail.org/message/jlc7t5edsjujbe37
Patch by rpluem, lars, niq
Modified:
httpd/httpd/trunk/include/ap_expr.h
httpd/httpd/trunk/modules/filters/mod_include.c
httpd/httpd/trunk/server/util_expr.c
Modified: httpd/httpd/trunk/modules/filters/mod_include.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_include.c?rev=795445&r1=795444&r2=795445&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/filters/mod_include.c (original)
+++ httpd/httpd/trunk/modules/filters/mod_include.c Sat Jul 18 23:12:58 2009
@@ -605,25 +605,30 @@
* The choice of returning NULL strings on not-found,
* v.s. empty strings on an empty match is deliberate.
*/
- if (!re) {
+ if (!re || !re->have_match) {
ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
"regex capture $%" APR_SIZE_T_FMT " refers to no regex in %s",
idx, r->filename);
return NULL;
}
- else {
- if (re->nsub < idx || idx >= AP_MAX_REG_MATCH) {
- ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
- "regex capture $%" APR_SIZE_T_FMT
- " is out of range (last regex was: '%s') in %s",
- idx, re->rexp, r->filename);
- return NULL;
- }
-
- if (re->match[idx].rm_so < 0 || re->match[idx].rm_eo < 0) {
- return NULL;
- }
+ else if (re->match[idx]rm_so == re->match[idx].rm_eo) {
+ return NULL;
+ }
+ else if (re->match[idx].rm_so < 0 || re->match[idx].rm_eo < 0) {
+ /* I don't think this can happen if have_match is true.
+ * But let's not risk a regression by dropping this
+ */
+ return NULL;
+ }
+ else if (re->nsub < idx || idx >= AP_MAX_REG_MATCH) {
IMHO this check should be the first (as in the old code) as it ensures that
idx is of correct range. So it should be moved before re->match[idx].rm_so ==
re->match[idx].rm_eo)
The problem with that is that re->nsub isn't the actual number of
matches, it's the maximum number we gave to regexec. So that check
returns true even when idx indexes a non-match.
I'd say it would be better to set it to the number of actual matches
when we run regexec. But I didn't want to think through the risk
of side-effects in a quick-fix scenario.
OTOH, bearing in mind the history of that code, it's never been
part of a public API in a stable release, so if we do create
side-effects, we're not at risk of breaking anything we shouldn't.
I guess we could reasonably hack it in trunk?
+ ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
+ "regex capture $%" APR_SIZE_T_FMT
+ " is out of range (last regex was: '%s') in %s",
+ idx, re->rexp, r->filename);
+ return NULL;
+ }
+ else {
val = apr_pstrmemdup(ctx->dpool, re->source + re->match[idx].rm_so,
re->match[idx].rm_eo - re->match[idx].rm_so);
As apr_pstrmemdup does return '\0' instead of NULL when re->match[idx].rm_so ==
re->match[idx].rm_eo
we change the behaviour by doing the re->match[idx].rm_so ==
re->match[idx].rm_eo check above.
Fair point. Again, side-effects. Let's reverse that change for 2.2
(and in trunk if you're unhappy with it).
--
Nick Kew
