You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ni...@apache.org on 2007/07/26 16:48:49 UTC
svn commit: r559837 - /httpd/httpd/trunk/modules/filters/mod_filter.c
Author: niq
Date: Thu Jul 26 07:48:48 2007
New Revision: 559837
URL: http://svn.apache.org/viewvc?view=rev&rev=559837
Log:
Fix integer comparisons in mod_filter
PR: 41835
Modified:
httpd/httpd/trunk/modules/filters/mod_filter.c
Modified: httpd/httpd/trunk/modules/filters/mod_filter.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_filter.c?view=diff&rev=559837&r1=559836&r2=559837
==============================================================================
--- httpd/httpd/trunk/modules/filters/mod_filter.c (original)
+++ httpd/httpd/trunk/modules/filters/mod_filter.c Thu Jul 26 07:48:48 2007
@@ -200,18 +200,24 @@
match = 0;
}
}
- else if (!provider->match.string) {
- match = 0;
- }
+ /* we can't check for NULL in provider as that kills integer 0
+ * so we have to test each string/regexp case in the switch
+ */
else {
- /* Now we have no nulls, so we can do string and regexp matching */
switch (provider->match_type) {
case STRING_MATCH:
- if (strcasecmp(str, provider->match.string)) {
+ if (!provider->match.string) {
+ match = 0;
+ }
+ else if (strcasecmp(str, provider->match.string)) {
match = 0;
}
break;
case STRING_CONTAINS:
+ if (!provider->match.string) {
+ match = 0;
+ break;
+ }
str1 = apr_pstrdup(r->pool, str);
ap_str_tolower(str1);
if (!strstr(str1, provider->match.string)) {
@@ -219,9 +225,12 @@
}
break;
case REGEX_MATCH:
- if (ap_regexec(provider->match.regex, str, 0, NULL, 0)
- == AP_REG_NOMATCH) {
- match = 0;
+ if (!provider->match.string) {
+ match = 0;
+ }
+ else if (ap_regexec(provider->match.regex, str, 0, NULL, 0)
+ == AP_REG_NOMATCH) {
+ match = 0;
}
break;
case INT_EQ:
@@ -229,23 +238,26 @@
match = 0;
}
break;
+ /* Integer comparisons should be [var] OP [match]
+ * We need to set match = 0 if the condition fails
+ */
case INT_LT:
- if (atoi(str) < provider->match.number) {
+ if (atoi(str) >= provider->match.number) {
match = 0;
}
break;
case INT_LE:
- if (atoi(str) <= provider->match.number) {
+ if (atoi(str) > provider->match.number) {
match = 0;
}
break;
case INT_GT:
- if (atoi(str) > provider->match.number) {
+ if (atoi(str) <= provider->match.number) {
match = 0;
}
break;
case INT_GE:
- if (atoi(str) >= provider->match.number) {
+ if (atoi(str) < provider->match.number) {
match = 0;
}
break;
Re: svn commit: r559837 - /httpd/httpd/trunk/modules/filters/mod_filter.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jul 26, 2007, at 3:46 PM, Ruediger Pluem wrote:
>
> This is correct (because provider->match is a union and provider-
> >match.string and
> provider->match.regex are the same thing), but confusing. I would
> prefer
> checking provider->match.regex instead.
>
Seems to me that avoiding unions altogether, if possible, is
a Good Thing.
Re: svn commit: r559837 - /httpd/httpd/trunk/modules/filters/mod_filter.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 07/26/2007 04:48 PM, niq@apache.org wrote:
> Author: niq
> Date: Thu Jul 26 07:48:48 2007
> New Revision: 559837
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=559837
> Log:
> Fix integer comparisons in mod_filter
> PR: 41835
>
> Modified:
> httpd/httpd/trunk/modules/filters/mod_filter.c
>
> Modified: httpd/httpd/trunk/modules/filters/mod_filter.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_filter.c?view=diff&rev=559837&r1=559836&r2=559837
> ==============================================================================
> @@ -219,9 +225,12 @@
> }
> break;
> case REGEX_MATCH:
> - if (ap_regexec(provider->match.regex, str, 0, NULL, 0)
> - == AP_REG_NOMATCH) {
> - match = 0;
> + if (!provider->match.string) {
This is correct (because provider->match is a union and provider->match.string and
provider->match.regex are the same thing), but confusing. I would prefer
checking provider->match.regex instead.
Regards
Rüdiger
AW: svn commit: r559837 - /httpd/httpd/trunk/modules/filters/mod_filter.c
Posted by Plüm,
Rüdiger,
VF-Group <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Nick Kew
> Gesendet: Donnerstag, 26. Juli 2007 23:20
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r559837 -
> /httpd/httpd/trunk/modules/filters/mod_filter.c
> Agreed. I'm happy with the changes you imply in this and your
> other post.
>
> On the other hand, removing the check might risk segfaulting in some
> future update - perhaps something that gets configuration per-request
> from a database and a rewritemap. I've recently had a similar issue
> with another module, which started life using match rules defined in
> httpd.conf, then grew to derive them from rewritemap+dbm. Worked
> fine until it constructed a match-rule from a corrupted
> database entry.
Ok, so it is a "better safe then sorry" approach. I am fine with this.
I just wanted to clarify if I missed something obvious at the current
state that makes these checks necessary :-).
So I think it is good to have the checks, but as the conditions should not
become true (provider->match.string should not be NULL in these cases)
I think it would be good to emit additionally an error message in these cases,
as something seems to be rotten then.
So the only point that remains is checking for match.regex in front of
the regular expression for the sake of clarity.
Regards
Rüdiger
Re: svn commit: r559837 -
/httpd/httpd/trunk/modules/filters/mod_filter.c
Posted by Nick Kew <ni...@webthing.com>.
On Thu, 26 Jul 2007 22:08:10 +0200
Ruediger Pluem <rp...@apache.org> wrote:
> > - else if (!provider->match.string) {
> > - match = 0;
> > - }
Lines removed; it's the replacement of those later you're questioning.
> > + /* we can't check for NULL in provider as that kills
> > integer 0
> > + * so we have to test each string/regexp case in the switch
> > + */
> > else {
> > - /* Now we have no nulls, so we can do string and
> > regexp matching */
Explanation - I don't recollect it, but I expect I had segfaults at
some point in development, and put the check in to prevent them.
Or maybe it was just to preempt them.
> Hm. How can provider->match.string == NULL if provider->match_type ~
> STRING_MATCH|STRING_CONTAINS?
Good question.
> Same here: How can provider->match.string = provider->match.regex ==
> NULL if provider->match_type == REGEX_MATCH? If ap_pregcomp fails in
> filter_provider we should bail out there IMHO.
Agreed. I'm happy with the changes you imply in this and your
other post.
On the other hand, removing the check might risk segfaulting in some
future update - perhaps something that gets configuration per-request
from a database and a rewritemap. I've recently had a similar issue
with another module, which started life using match rules defined in
httpd.conf, then grew to derive them from rewritemap+dbm. Worked
fine until it constructed a match-rule from a corrupted database entry.
--
Nick Kew
Application Development with Apache - the Apache Modules Book
http://www.apachetutor.org/
Re: svn commit: r559837 - /httpd/httpd/trunk/modules/filters/mod_filter.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 07/26/2007 04:48 PM, niq@apache.org wrote:
> Author: niq
> Date: Thu Jul 26 07:48:48 2007
> New Revision: 559837
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=559837
> Log:
> Fix integer comparisons in mod_filter
> PR: 41835
>
> Modified:
> httpd/httpd/trunk/modules/filters/mod_filter.c
>
> Modified: httpd/httpd/trunk/modules/filters/mod_filter.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_filter.c?view=diff&rev=559837&r1=559836&r2=559837
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_filter.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_filter.c Thu Jul 26 07:48:48 2007
> @@ -200,18 +200,24 @@
> match = 0;
> }
> }
> - else if (!provider->match.string) {
> - match = 0;
> - }
> + /* we can't check for NULL in provider as that kills integer 0
> + * so we have to test each string/regexp case in the switch
> + */
> else {
> - /* Now we have no nulls, so we can do string and regexp matching */
> switch (provider->match_type) {
> case STRING_MATCH:
> - if (strcasecmp(str, provider->match.string)) {
> + if (!provider->match.string) {
> + match = 0;
> + }
> + else if (strcasecmp(str, provider->match.string)) {
> match = 0;
> }
> break;
> case STRING_CONTAINS:
> + if (!provider->match.string) {
> + match = 0;
> + break;
> + }
Hm. How can provider->match.string == NULL if provider->match_type ~ STRING_MATCH|STRING_CONTAINS?
> str1 = apr_pstrdup(r->pool, str);
> ap_str_tolower(str1);
> if (!strstr(str1, provider->match.string)) {
> @@ -219,9 +225,12 @@
> }
> break;
> case REGEX_MATCH:
> - if (ap_regexec(provider->match.regex, str, 0, NULL, 0)
> - == AP_REG_NOMATCH) {
> - match = 0;
> + if (!provider->match.string) {
> + match = 0;
Same here: How can provider->match.string = provider->match.regex == NULL if provider->match_type == REGEX_MATCH?
If ap_pregcomp fails in filter_provider we should bail out there IMHO.
Regards
Rüdiger