Re: svn commit: r559837 - /httpd/httpd/trunk/modules/filters/mod_filter.c

On 07/26/2007 04:48 PM, niqapache.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/tru
nk/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

On Thu, 26 Jul 2007 22:08:10 +0200
Ruediger Pluem <rpluemapache.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.or
g/