You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@spamassassin.apache.org by do...@apache.org on 2006/02/14 22:19:07 UTC
svn commit: r377834 - /spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm
Author: dos
Date: Tue Feb 14 13:19:04 2006
New Revision: 377834
URL: http://svn.apache.org/viewcvs?rev=377834&view=rev
Log:
bug 4795: validate score config option
Modified:
spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm
Modified: spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm
URL: http://svn.apache.org/viewcvs/spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm?rev=377834&r1=377833&r2=377834&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm Tue Feb 14 13:19:04 2006
@@ -214,6 +214,13 @@
code => sub {
my ($self, $key, $value, $line) = @_;
my($rule, @scores) = split(/\s+/, $value);
+ unless (defined $value && $value !~ /^$/ &&
+ (scalar @scores == 1 || scalar @scores == 4)) {
+ $self->{parser}->lint_warn("config: score configuration option " .
+ "requires a symbolic rule name and 1 or 4 scores, skipping: $line",
+ $rule);
+ return;
+ }
# Figure out if we're doing relative scores, remove the parens if we are
my $relative = 0;
@@ -221,12 +228,17 @@
if (s/^\((-?\d+(?:\.\d+)?)\)$/$1/) {
$relative = 1;
}
+ unless (/^-?\d+(?:\.\d+)?$/) {
+ $self->{parser}->lint_warn("config: the non-numeric score ($_) is ".
+ "invalid, numeric score required, skipping: $line", $rule);
+ return;
+ }
}
if ($relative && !exists $self->{scoreset}->[0]->{$rule}) {
$self->{parser}->lint_warn("config: relative score without ".
"previous setting in configuration, skipping: $line", $rule);
- return $INVALID_VALUE;
+ return;
}
# If we're only passed 1 score, copy it to the other scoresets
@@ -243,11 +255,6 @@
$self->{scoreset}->[$index]->{$rule} = $score + 0.0;
}
- }
- else {
- $self->{parser}->lint_warn("config: score configuration ".
- "option without actual scores, skipping: $line", $rule);
- return $INVALID_VALUE;
}
}
});
Re: svn commit: r377834 - /spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm
Posted by "Daryl C. W. O'Shea" <sp...@dostech.ca>.
Theo Van Dinter wrote:
> On Tue, Feb 14, 2006 at 05:47:39PM -0500, Daryl C. W. O'Shea wrote:
>> Or we can drop the lint_warn()s and go with the generic error messages.
>
> +1 :)
Sounds good to me. I only used lint_warn() because it was already
there. I now see it's a recent addition in r356398.
Daryl
Re: svn commit: r377834 - /spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm
Posted by Theo Van Dinter <fe...@apache.org>.
On Tue, Feb 14, 2006 at 05:47:39PM -0500, Daryl C. W. O'Shea wrote:
> Using lint_warn() and return $INVALID_VALUE (or $MISSING_REQUIRED_VALUE)
> like the existing code did results in double the number of actual lint
> errors.
True. My last message was a little off, btw (sorry). We used to just
do info("message") and then the return value which would be 1 lint error
plus some informative text (info is a higher log level than debug, but
neither goto stderr/log by default). Calling lint_warn() is something
which seems to have been added in for the normalize_charset code, which
it shouldn't be doing either for the same reason (it also confused me
leading to my previous mail about using it). In looking at the code
some more, lint_warn() is a Conf::Parser routine which should probably
be private IMO.
> I guess if we want the warnings to go to STDERR (or the log facility
> when daemonized) even when NOT linting, the value returned could be
> decided upon based on whether we're linting or not.
Just throw an info() and return an error.
> Or we can drop the lint_warn()s and go with the generic error messages.
+1 :)
--
Randomly Generated Tagline:
To err is human,
To purr feline.
-- Robert Byrne
Re: svn commit: r377834 - /spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm
Posted by "Daryl C. W. O'Shea" <sp...@dostech.ca>.
Theo Van Dinter wrote:
> On Tue, Feb 14, 2006 at 09:19:07PM -0000, dos@apache.org wrote:
>> + unless (defined $value && $value !~ /^$/ &&
>> + (scalar @scores == 1 || scalar @scores == 4)) {
>> + $self->{parser}->lint_warn("config: score configuration option " .
>> + "requires a symbolic rule name and 1 or 4 scores, skipping: $line",
>> + $rule);
>> + return;
>> + }
>>
>> + unless (/^-?\d+(?:\.\d+)?$/) {
>> + $self->{parser}->lint_warn("config: the non-numeric score ($_) is ".
>> + "invalid, numeric score required, skipping: $line", $rule);
>> + return;
>> + }
>>
>> - return $INVALID_VALUE;
>> + return;
>
> I'm leaning towards a -1 on this patch. You removed the $INVALID_VALUE
> bits and just use a return in the new code. Returning $INVALID_VALUE and
> $MISSING_REQUIRED_VALUE is what flags that there's a problem in the code
> that calls this function (Conf::Parser). Returning undef specifically
> removes the fact that there was an error -- it's the default return code.
Using lint_warn() and return $INVALID_VALUE (or $MISSING_REQUIRED_VALUE)
like the existing code did results in double the number of actual lint
errors.
> What we typically do for conf is use lint_warn() only for information we
> want to give out if the user is running --lint (there's very little that
> uses this). Otherwise, if we want a more friendly error message, specify
> info("message") and return either INVALID_VALUE or MISSING_REQUIRED_VALUE.
> If we don't want to specify a message, just return INVALID_VALUE or
> MISSING_REQUIRED_VALUE and let the default error message indicate
> the issue.
I guess if we want the warnings to go to STDERR (or the log facility
when daemonized) even when NOT linting, the value returned could be
decided upon based on whether we're linting or not.
Or we can drop the lint_warn()s and go with the generic error messages.
Daryl
Re: svn commit: r377834 - /spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm
Posted by Theo Van Dinter <fe...@apache.org>.
On Tue, Feb 14, 2006 at 09:19:07PM -0000, dos@apache.org wrote:
> + unless (defined $value && $value !~ /^$/ &&
> + (scalar @scores == 1 || scalar @scores == 4)) {
> + $self->{parser}->lint_warn("config: score configuration option " .
> + "requires a symbolic rule name and 1 or 4 scores, skipping: $line",
> + $rule);
> + return;
> + }
>
> + unless (/^-?\d+(?:\.\d+)?$/) {
> + $self->{parser}->lint_warn("config: the non-numeric score ($_) is ".
> + "invalid, numeric score required, skipping: $line", $rule);
> + return;
> + }
>
> - return $INVALID_VALUE;
> + return;
I'm leaning towards a -1 on this patch. You removed the $INVALID_VALUE
bits and just use a return in the new code. Returning $INVALID_VALUE and
$MISSING_REQUIRED_VALUE is what flags that there's a problem in the code
that calls this function (Conf::Parser). Returning undef specifically
removes the fact that there was an error -- it's the default return code.
What we typically do for conf is use lint_warn() only for information we
want to give out if the user is running --lint (there's very little that
uses this). Otherwise, if we want a more friendly error message, specify
info("message") and return either INVALID_VALUE or MISSING_REQUIRED_VALUE.
If we don't want to specify a message, just return INVALID_VALUE or
MISSING_REQUIRED_VALUE and let the default error message indicate
the issue.
--
Randomly Generated Tagline:
Bender: This is the Brooklyn-bound B train making local stops at wherever
the hell I feel like, watch for the closing doors.