You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2009/07/19 13:04:15 UTC
Re: svn commit: r795445 - in /httpd/httpd/trunk: include/ap_expr.h
modules/filters/mod_include.c server/util_expr.c
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)
> + 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.
Regards
Rüdiger
Re: svn commit: r795445 - in /httpd/httpd/trunk: include/ap_expr.h
modules/filters/mod_include.c server/util_expr.c
Posted by Graham Leggett <mi...@sharp.fm>.
Nick Kew wrote:
> I've updated the trunk patch in the light of your
> comments in r795642.
>
> If you're happy with that, feel free to apply the
> same additional patch to the backport proposal
> and carry my +1 across.
>
> I won't revisit STATUS until we've had time for
> your reaction, and anyone else (Graham - is
> your +1 affected by the change in r795642?)
The changes make sense, so +1.
Regards,
Graham
--
RE: svn commit: r795445 - in /httpd/httpd/trunk: include/ap_expr.h modules/filters/mod_include.c server/util_expr.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Nick Kew
> Sent: Sonntag, 19. Juli 2009 23:59
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r795445 - in /httpd/httpd/trunk:
> include/ap_expr.h modules/filters/mod_include.c server/util_expr.c
>
> Ruediger Pluem wrote:
>
> > [chop]
>
> I've updated the trunk patch in the light of your
> comments in r795642.
>
> If you're happy with that, feel free to apply the
> same additional patch to the backport proposal
> and carry my +1 across.
>
> I won't revisit STATUS until we've had time for
> your reaction, and anyone else (Graham - is
> your +1 affected by the change in r795642?)
I have no write access to people.apache.org before this evening
to upload a patch, so if someone else could put some together
before I would be happy to review and vote on it.
Regards
Rüdiger
Re: svn commit: r795445 - in /httpd/httpd/trunk: include/ap_expr.h
modules/filters/mod_include.c server/util_expr.c
Posted by Nick Kew <ni...@webthing.com>.
Ruediger Pluem wrote:
> [chop]
I've updated the trunk patch in the light of your
comments in r795642.
If you're happy with that, feel free to apply the
same additional patch to the backport proposal
and carry my +1 across.
I won't revisit STATUS until we've had time for
your reaction, and anyone else (Graham - is
your +1 affected by the change in r795642?)
--
Nick Kew
Re: svn commit: r795445 - in /httpd/httpd/trunk: include/ap_expr.h modules/filters/mod_include.c server/util_expr.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jul 19, 2009, at 11:37 AM, Ruediger Pluem wrote:
>
> But if idx for whatever reason is above AP_MAX_REG_MATCH we can
> segfault
> again in re->match[idx].rm_so == re->match[idx].rm_eo. So I still
> suggest
> to reverse the order of both if statements. This doesn't cost us
> anything
> but make things more foolprove.
>
+1
Re: svn commit: r795445 - in /httpd/httpd/trunk: include/ap_expr.h
modules/filters/mod_include.c server/util_expr.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 07/19/2009 05:26 PM, Nick Kew wrote:
> Ruediger Pluem wrote:
>>
>> On 07/19/2009 01:12 AM, niq@apache.org wrote:
>
>>> - 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.
But if idx for whatever reason is above AP_MAX_REG_MATCH we can segfault
again in re->match[idx].rm_so == re->match[idx].rm_eo. So I still suggest
to reverse the order of both if statements. This doesn't cost us anything
but make things more foolprove.
>
> 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).
As I see no benefit in returning NULL here I would say that we should
reverse this in trunk as well as it keeps 2.2.x and trunk closer together.
I have no problem if trunk is different from 2.2.x but it should be for
a reason which I fail to see here.
Regards
Rüdiger
Re: svn commit: r795445 - in /httpd/httpd/trunk: include/ap_expr.h
modules/filters/mod_include.c server/util_expr.c
Posted by Nick Kew <ni...@webthing.com>.
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