httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nick Kew <n...@webthing.com>
Subject Re: svn commit: r795445 - in /httpd/httpd/trunk: include/ap_expr.h modules/filters/mod_include.c server/util_expr.c
Date Sun, 19 Jul 2009 15:26:36 GMT
Ruediger Pluem wrote:
> 
> On 07/19/2009 01:12 AM, niq@apache.org 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

Mime
View raw message