[prev in list] [next in list] [prev in thread] [next in thread] 

List:       apache-httpd-dev
Subject:    Re: svn commit: r559837 -
From:       Nick Kew <nick () webthing ! com>
Date:       2007-07-26 21:19:55
Message-ID: 20070726221955.201932d6 () grimnir
[Download RAW message or body]

On Thu, 26 Jul 2007 22:08:10 +0200
Ruediger Pluem <rpluem@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/
[prev in list] [next in list] [prev in thread] [next in thread]