svn commit: r169589 - /spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm

[do...@apache.org]

Author: dos
Date: Tue May 10 22:21:41 2005
New Revision: 169589

URL: http://svn.apache.org/viewcvs?rev=169589&view=rev
Log:
bug 3846: verify RE before including a rule

Modified:
    spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm

Modified: spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm
URL: http://svn.apache.org/viewcvs/spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm?rev=169589&r1=169588&r2=169589&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm Tue May 10 22:21:41 2005
@@ -816,13 +816,16 @@
 sub is_regexp_valid {
   my ($self, $name, $re) = @_;
 
+  # get rid of the / delimiters in $re so we can verify it
+  $re =~ s/^\/(.*)\/$/$1/;
+
   if (eval { ("" =~ m{$re}); 1; }) {
     return 1;
   }
   else {
     my $err = $@;
     $err =~ s/ at .*? line \d+\.\n?//;
-    warn "config: invalid regexp for rule $name: $re: $err\n";
+    warn "config: invalid regexp for rule $name: /$re/: $err\n";
     $self->{conf}->{errors}++;
     return 0;
   }

Re: redirector code broken

[Daniel Quinlan <qu...@pathname.com>]

Daryl C W O'Shea <sp...@dostech.ca> writes:

> So you'd rather not qr// the regexp (in Conf.pm) and do an eval in PMS
> instead?  This would allow for modifiers to be included but will be a
> little slower.

I'm fine with qr// of the regexp *if* we can get at it cleanly
(supporting any legal perl regexp syntax).  We don't manage to do that
anywhere currently.

It doesn't have to be an eval, but I think an eval does solve/avoid the
problem.

Daniel

-- 
Daniel Quinlan
http://www.pathname.com/~quinlan/

Re: redirector code broken

["Daryl C. W. O'Shea" <sp...@dostech.ca>]

Daniel Quinlan wrote:
> "Daryl C. W. O'Shea" <sp...@dostech.ca> writes:
> 
>>Hmm, it doesn't handle modifiers or things like "Subject =~" which are 
>>present in $re either.
>>
>>Will fix this afternoon.
> 
> 
> Already done.  The redirector code is broken, though.  We should:
> 
>  - keep the redirector regexp strings in original form, also don't
>    require / to start and end, m{http://...foo=(.*)} should also be
>    legal
>  - use an eval when running like PMS and is_regexp_valid() do

So you'd rather not qr// the regexp (in Conf.pm) and do an eval in PMS 
instead?  This would allow for modifiers to be included but will be a 
little slower.

The current code fixes the demlimiter issue, but I'm open to changing to 
an eval in PMS.


>  - the redirector regexps are "broken", / is used as the delimiters, but
>    the internal / are not protected

Wow, I messed that one up while pondering why is_regexp_valid() was 
accepting invalid regexps.  When I realized is_regexp_valid() was broken 
itself, I forgot all about it.


> The third issue was exposed by fixing the is_regexp_valid code.  Hacking
> around it by kludging regexps going into is_regexp_valid is not the way
> to fix it, though.  ;-)

Well, the original regexp is passed to is_regexp_valid() now.  Since I 
don't know of a way to pass a modifier variable to qr// (or even 
delimiters for that matter), the (any valid) delimiters and modifiers 
are stripped.

So... the regexp is validated, but modifiers will be ignored.  If 
there's a way to include the modifiers in a qr// that would be optimal, 
otherwise moving to an eval in PMS would (be more processing) work.


Daryl

redirector code broken

[Daniel Quinlan <qu...@pathname.com>]

"Daryl C. W. O'Shea" <sp...@dostech.ca> writes:

> Hmm, it doesn't handle modifiers or things like "Subject =~" which are 
> present in $re either.
> 
> Will fix this afternoon.

Already done.  The redirector code is broken, though.  We should:

 - keep the redirector regexp strings in original form, also don't
   require / to start and end, m{http://...foo=(.*)} should also be
   legal
 - use an eval when running like PMS and is_regexp_valid() do
 - the redirector regexps are "broken", / is used as the delimiters, but
   the internal / are not protected

The third issue was exposed by fixing the is_regexp_valid code.  Hacking
around it by kludging regexps going into is_regexp_valid is not the way
to fix it, though.  ;-)

Daniel

-- 
Daniel Quinlan
http://www.pathname.com/~quinlan/

Re: svn commit: r169589 - /spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm

["Daryl C. W. O'Shea" <sp...@dostech.ca>]

Daniel Quinlan wrote:
> dos@apache.org writes:
> 
> 
>>+  # get rid of the / delimiters in $re so we can verify it
>>+  $re =3D~ s/^\/(.*)\/$/$1/;
> 
> 
> I believe other delimiters are legal.

Hmm, it doesn't handle modifiers or things like "Subject =~" which are 
present in $re either.

Will fix this afternoon.


Daryl

Re: svn commit: r169589 - /spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm

[Loren Wilton <lw...@earthlink.net>]

> I believe other delimiters are legal.

Indeed.  I commonly write
    m'stuff'i
if I'm going to be matching slashes.

BTW, I also will commonly write things like
    =~ /BADWord/    # no /i
to make it clear that, no, I *did not* forget to make that test case
insensitive.

Depending on what leads up to the substitution noted, this case may cause
that to fail.  For that matter /stuff/i will make that right / not match
\/$.

Re: svn commit: r169589 - /spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm

[Daniel Quinlan <qu...@pathname.com>]

dos@apache.org writes:

> +  # get rid of the / delimiters in $re so we can verify it
> +  $re =3D~ s/^\/(.*)\/$/$1/;

I believe other delimiters are legal.

Daniel

-- 
Daniel Quinlan
http://www.pathname.com/~quinlan/