I've been seeing the same thing (can't prove the first time worked but certainly
now getting 1 for all user-defined scores). This was happening in 3.0.1 as well,
so I upgraded in hopes that the problem would vanish.

The following are in my .spamassassin/user_prefs
 header CF_SUB_UID               Subject =~ /vlb|Vicki/i
 score CF_SUB_UID                4.0
 describe CF_SUB_UID             Subject: contains my ID
 header CF_NOT_FOR_ME            ToCc !~ /vlb\@cfcl/
 score CF_NOT_FOR_ME             3.0
 describe CF_NOT_FOR_ME          Neither To nor Cc me

Here are the headers from a piece of spam

X-Spam-Flag: YES
X-Spam-Checker-Version: SpamAssassin 3.0.2 (2004-11-16) on cfcl.com
X-Spam-Level: *****
X-Spam-Status: Yes, score=5.0 required=0.5 tests=CF_NOT_FOR_ME,CF_SUB_UID,
	FORGED_RCVD_HELO autolearn=no version=3.0.2
X-Spam-Report:
	*  1.0 CF_SUB_UID Subject: contains my ID
	*  1.0 CF_NOT_FOR_ME Neither To nor Cc me
	*  3.0 FORGED_RCVD_HELO Received: contains a forged HELO

We startspamd as
    spamd -d -c --pidfile=/tmp/spamd.pid

I haven't tried the -m 0 workaround

spamd -m 0 
had no affect for me

*** Bug 4160 has been marked as a duplicate of this bug. ***

we've got to fix this one -- it'll be tricky though :(  user rules are hackily
implemented, internally.  however I suspect there may be a possibility of an
easy fix; it could just be something Storable-related.

Taking bug. I believe I've got it fixed, just have to make sure I didn't break
anything.

reassign

Created attachment 2688 [details]
patch

The existing 'set_default_scores' code checked the wrong copy of the scores
before setting a rule's score to the default value.

This fixes the problem and doesn't appear to break anything but this is the
first time I've looked at the scoreset code so a little review wouldn't hurt.

(In reply to comment #7)
> Created an attachment (id=2688) [edit]
> patch
> 
> The existing 'set_default_scores' code checked the wrong copy of the scores
> before setting a rule's score to the default value.
> 
> This fixes the problem and doesn't appear to break anything but this is the
> first time I've looked at the scoreset code so a little review wouldn't hurt.

Great. This works for me. Thanks. I patched and watch behaviour.

It's not initially apparent to me how the patch changes anything
($conf->{scores} should always be a reference to a $conf->{scoreset}->[#]
array), but if the issue is fixed, then +1. :)

Then again, wait a minute...  I came up with a theoretical way this could
happen, but I haven't verified anything yet:

If Storable doesn't understand references, ie: $conf->{scores} =
$conf->{scoreset}->[0], the first time the rule/score is parsed, it'll get the
right score.  Then Storable will revert {scores} and {scoreset} to different
arrays.  The second time the rule/score is parsed, the score is set in
{scoreset}, and then set_default_score goes through, doesn't see the score in
{scores} (because it's not a pointer into {scoreset} anymore), and sets
{scoreset} to the default value of 1.  Then at check() time, {scores} gets
turned back into a reference and points into {scoreset}, and you get a 1.0 for
the rule.


If this is the case (and the more I think about it, the more likely it is,) then
 we'll also want to double check any other "pointer references" we use and deal
with them appropriately in M::SA::copy_config().  Alternatively, we can kill the
horrible Storable/dclone kluge thing I wrote (it seemed like a good idea at the
time) and replace it with something better.

So yes, +1 on the patch.  BTW: I'd apply it to trunk, then set the ticket
milestone to 3.0.3 in case we actually do one before 3.1 comes out.

I've never looked at Storable, but your theory sounds reasonable to me.

I'm still waiting on my ASF account, maybe they're waiting for my birthday
tomorrow.  I'll check it into trunk when I get access.

Subject: Re:  [review] Score for user defined rules become ignored

On Wed, Mar 09, 2005 at 08:21:34AM -0800, bugzilla-daemon@bugzilla.spamassassin.org wrote:
> I've never looked at Storable, but your theory sounds reasonable to me.

Yeah, the more I thought about it, the more I think that's what's happening.
Basically, our code goes through and saves out all of the conf one variable at
a time.  But it doesn't do any form of "do these two variables reference the
same data element" checking.  So as far as the code is concerned, {scores} and
{scoreset}->[#] are different hashes, even though they really just point at
the same place.

I'll poke at copy_config() and at least get {scores} dealt with correctly.

> I'm still waiting on my ASF account, maybe they're waiting for my birthday
> tomorrow.  I'll check it into trunk when I get access.

;)

fyi, this patch doesn't need a vote.  it's aimed at trunk (milestone is 3.1),
which is CTR at the moment, so you can just commit it without needing a full
vote first.

after moving the ticket to a RTC release queue (such as the 3.0.3 queue), then
there'd need to be a vote for applying it.

Yeah, that's what I had meant the vote for.  I guess that would have been
clearer if I had already committed it to trunk.

Created attachment 2691 [details]
additional patch

modify copy_config() to deal with scores reference specially

Just got around to trying out Theo's patch.  Looks like the scores reference
works correctly now, so I'm +1 on it.

Moving target to 3.0.3 for review of his patch (since that's what's actually
broken).

Comment on attachment 2688 [details]
patch

I found the problem and masked it -- not good!

*** Bug 4062 has been marked as a duplicate of this bug. ***

+1

+1

in passing, this is why using Storable to copy configs without worrying about
refs etc. isn't really helping us anyway.  ;)

ok, committed.  r164513

*** Bug 4303 has been marked as a duplicate of this bug. ***