Created attachment 5861 [details]
Possible fix for the issue

Not sure why do_meta_tests couldn't be run, any fix will need detailed explanation anyway.. I'll try also looking soon when I have time..

There is nothing mysterious about this issue.

Original complaint was that shortcircuiting RCVD_IN_VALIDITY_CERTIFIED results in MISSING_* rules hitting.

If you look at the MISSING_* rules, they are just dumb metas referring to various exists: rules.

meta MISSING_FROM              !__HAS_FROM
header    __HAS_FROM              exists:From

Network rules are run at -100 priority, so if they receive a quick response and shortcircuiting is activated, rules like __HAS_FROM are not even run yet since default rule priority 0 is not reached.

Since we decided to let meta rules evaluate with unrun dependencies, everything works as designed.

Logical fix is to adjust priority for all commonly referred rules in metas. Lowest priority seen in stock ruleset is -1000, for example shortcircuiting USER_IN_WELCOMELIST has the same effect. So __HAS_FROM and others need to be set at -2000 for example. This requires no code commits, only rule changes, I'll change the necessary ones.

Sending        rules/20_compensate.cf
Sending        rules/20_head_tests.cf
Sending        rules/20_html_tests.cf
Sending        rules/20_meta_tests.cf
Transmitting file data ....done
Committing transaction...
Committed revision 1905734.

Ok I'll reopen this myself per list discussion to try to move things along.

Looking at do_meta_tests(), I think it doesn't have any dependencies to anything aside from start_rules/Reuse, which I guess is never used with shortcircuiting. So not running it looks fine, and should prevent evaluating any rules with unexpected meta results. I'd leave the priority -2000 changes around in case someone is using older trunk.

I would add deadline check, and probably move this before start_rules is called, since we don't want to start anything more?

sub do_meta_tests {
  my ($self, $pms, $priority, $finish) = @_;

  return if $pms->{deadline_exceeded} || $pms->{shortcircuited};


+1, vote to commit?

+1 to commit that conditional return at line 283 of lib/Mail/SpamAssassin/Plugin/Check.pm

Thanks, Henrik.

(In reply to Bill Cole from comment #6)
> 
> Thanks, Henrik.

Please redirect thanks to Giovanni, I'm just confirming his solution was correct. :-)

Created attachment 5863 [details]
skip meta tests asap

patch as suggested in
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=8078#c5

+1 to the latest patch.  Do we need to revert the few rule changes that were made recently to work on this issue as well?

(In reply to Kevin A. McGrail from comment #9)
> +1 to the latest patch.  Do we need to revert the few rule changes that were
> made recently to work on this issue as well?

No, those priority changes to a few very low-cost header presence rules make sense with or without the patch.

As said the rule priority fixes are useful "in case someone is using older trunk".

Enough votes so commit and back to release schedule:

Sending        trunk/lib/Mail/SpamAssassin/Plugin/Check.pm
Transmitting file data .done
Committing transaction...
Committed revision 1905778.

FWIW, this maybe causing some unexpected/unintended behavior for 3.x installations...

I manage SA for two sites which run 3.4.0 & 3.4.2 respectively.  Starting the day they got the updated rules (revision 1905734) the sa-compile process began to issue several warnings:

-------------------------------
body_neg2000.c: In function ‘XS_Mail__SpamAssassin__CompiledRegexps__body_neg2000_scan’:
body_neg2000.xs:45:17: warning: variable ‘pend’ set but not used [-Wunused-but-set-variable]
  unsigned char *pend;
                 ^
body_neg2000.xs:43:17: warning: unused variable ‘cursor’ [-Wunused-variable]
  unsigned char *cursor;
                 ^
body_neg2000.xs:42:8: warning: unused variable ‘match’ [-Wunused-variable]
  char *match;
        ^
body_neg2000.c: At top level:
body_neg2000.xs:17:3: warning: ‘split_and_add’ defined but not used [-Wunused-function]
   split_and_add (AV *results, char *match)
   ^
Running Mkbootstrap for Mail::SpamAssassin::CompiledRegexps::body_neg2000 ()
-------------------------------


After a couple of hours of reverse engineering how this all works, I determined that it was due to these new -2000 priority designations -- more specifically the two body rules.  Thanks to the comment in that revision, it lead me here.

What seems to happen is that the BodyRuleBaseExtractor produces an empty list of "bases" because of the nature of those two rules not being typical string matches.  As a result of that, rule2xs in sa-compile creates the .xs file with the PREINIT section that declares those variables.  However, because $numscans is still 0 (due to no bases), it skips the loop that builds $xscode that would normally reference them.  As such, warnings about unused/etc.


For anyone else affected by this, a workaround is to ensure there is at least one body rule that will get picked up by sa-compile.  Probably a bad idea, but my quick'n'dirty fix was to create one like this:

body IGNOREME_NEG_2000 /neg2000/
score IGNOREME_NEG_2000 0.001
priority IGNOREME_NEG_2000 -2000
describe IGNOREME_NEG_2000 Just a workaround, see Bug 8078

(In reply to DC from comment #13)
> FWIW, this maybe causing some unexpected/unintended behavior for 3.x
> installations...
> 
> I manage SA for two sites which run 3.4.0 & 3.4.2 respectively.  Starting
> the day they got the updated rules (revision 1905734) the sa-compile process
> began to issue several warnings:

Thanks for analyzing. It seems on your system compiler uses many -W flags, so I could only reproduce it manually adding them. From what I see, this is only informational warnings and in no way fatal. I've created Bug 8091 if someone wants to look at it (personally I'd rather get rid of whole sa-compile mess).