This is an automated email from the ASF dual-hosted git repository. chibenwa pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-jspf.git
commit 188da42eae652456799578156e5fb303853b9a83 Author: Quan Tran <[email protected]> AuthorDate: Tue Nov 3 14:29:30 2026 +0700 [FIX] FallbackPolicy: give each check its own copy of an entry's record FallbackPolicy and OverridePolicy parse the record of an entry once, then gave that same object to every check of a matching domain. The policies after them (NeutralIfNotMatchPolicy, DefaultExplanationPolicy) add a modifier to the record they get, so each check added 2 modifiers to the stored record: 10006 after 5003 checks, each check running all of them. Checks running at once changed the list while another one went through it: with an override entry for the checked domain, 8 threads of 500 checks each gave 3836 neutral (a ConcurrentModificationException) and 164 pass, instead of 4000 pass. Give each check a copy of the stored record, with the same terms: they keep the state of a check in its session. TrustedForwarderPolicy still copies the record it changes, so the given record stays as it is. --- .../james/jspf/policies/local/FallbackPolicy.java | 14 ++++++++++- .../policies/local/TrustedForwarderPolicy.java | 2 +- ...rderPolicyTest.java => FallbackPolicyTest.java} | 27 ++++++++++++---------- .../policies/local/TrustedForwarderPolicyTest.java | 1 - 4 files changed, 29 insertions(+), 15 deletions(-) diff --git a/resolver/src/main/java/org/apache/james/jspf/policies/local/FallbackPolicy.java b/resolver/src/main/java/org/apache/james/jspf/policies/local/FallbackPolicy.java index d2c6209..8b0056c 100644 --- a/resolver/src/main/java/org/apache/james/jspf/policies/local/FallbackPolicy.java +++ b/resolver/src/main/java/org/apache/james/jspf/policies/local/FallbackPolicy.java @@ -41,6 +41,18 @@ import org.slf4j.LoggerFactory; public class FallbackPolicy implements PolicyPostFilter { private static final Logger LOGGER = LoggerFactory.getLogger(FallbackPolicy.class); + /** + * @param record the record of an entry + * @return a record of the same terms, for one check: the policies after this one add their + * modifiers to the record they get + */ + private static SPF1Record copyOf(SPF1Record record) { + SPF1Record copy = new SPF1Record(record.getRecord()); + copy.getDirectives().addAll(record.getDirectives()); + copy.getModifiers().addAll(record.getModifiers()); + return copy; + } + private Map<String,SPF1Record> entryMap; private SPFRecordParser parser; @@ -134,7 +146,7 @@ public class FallbackPolicy implements PolicyPostFilter { } if (entry != null) { - return entry; + return copyOf(entry); } else { return null; } diff --git a/resolver/src/main/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicy.java b/resolver/src/main/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicy.java index e1e968b..a94fdb9 100644 --- a/resolver/src/main/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicy.java +++ b/resolver/src/main/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicy.java @@ -99,7 +99,7 @@ public class TrustedForwarderPolicy implements PolicyPostFilter, MacroExpandEnab }.setHost(TRUSTED_FORWARDER_HOST); trusted.enableMacroExpand(macroExpand); trusted.enableSPFChecking(spfChecker); - // a copy: the record can be shared, as the cached ones of FallbackPolicy + // a copy: the given record stays as it is SPF1Record filtered = new SPF1Record(spfRecord.getRecord()); filtered.getDirectives().addAll(directives); filtered.getModifiers().addAll(spfRecord.getModifiers()); diff --git a/resolver/src/test/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicyTest.java b/resolver/src/test/java/org/apache/james/jspf/policies/local/FallbackPolicyTest.java similarity index 56% copy from resolver/src/test/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicyTest.java copy to resolver/src/test/java/org/apache/james/jspf/policies/local/FallbackPolicyTest.java index 8b65a02..2fc1c52 100644 --- a/resolver/src/test/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicyTest.java +++ b/resolver/src/test/java/org/apache/james/jspf/policies/local/FallbackPolicyTest.java @@ -21,30 +21,33 @@ package org.apache.james.jspf.policies.local; import static org.junit.Assert.assertEquals; -import org.apache.james.jspf.core.SPF1Record; +import org.apache.james.jspf.core.SPFRecordParser; import org.apache.james.jspf.impl.DefaultTermsFactory; import org.apache.james.jspf.parser.RFC4408SPF1Parser; +import org.apache.james.jspf.policies.NeutralIfNotMatchPolicy; import org.junit.Test; -public class TrustedForwarderPolicyTest { +public class FallbackPolicyTest { + private static final SPFRecordParser PARSER = new RFC4408SPF1Parser(new DefaultTermsFactory()); @Test - public void getSPFRecordShouldAddTheIncludeBeforeTheLastAll() throws Exception { - SPF1Record record = new RFC4408SPF1Parser(new DefaultTermsFactory()).parse("v=spf1 ip4:192.0.2.1 -all"); + public void aFallbackRecordShouldNotKeepTheModifiersAddedToItInACheck() throws Exception { + FallbackPolicy policy = new FallbackPolicy(PARSER); + policy.addEntry("example.com.", "v=spf1 -all"); - SPF1Record filtered = new TrustedForwarderPolicy().getSPFRecord("example.com", record); + // as the policies after the fallback do in a check + new NeutralIfNotMatchPolicy().getSPFRecord("example.com.", policy.getSPFRecord("example.com.", null)); - assertEquals("[ip4:192.0.2.1, include:" + TrustedForwarderPolicy.TRUSTED_FORWARDER_HOST + ", -all]", - filtered.getDirectives().toString()); + assertEquals(0, policy.getSPFRecord("example.com.", null).getModifiers().size()); } @Test - public void getSPFRecordShouldNotChangeTheGivenRecord() throws Exception { - // FallbackPolicy hands the same record to every check - SPF1Record record = new RFC4408SPF1Parser(new DefaultTermsFactory()).parse("v=spf1 -all"); + public void anOverrideRecordShouldNotKeepTheModifiersAddedToItInACheck() throws Exception { + OverridePolicy policy = new OverridePolicy(PARSER); + policy.addEntry("example.com.", "v=spf1 -all"); - new TrustedForwarderPolicy().getSPFRecord("example.com", record); + new NeutralIfNotMatchPolicy().getSPFRecord("example.com.", policy.getSPFRecord("example.com.")); - assertEquals("[-all]", record.getDirectives().toString()); + assertEquals(0, policy.getSPFRecord("example.com.").getModifiers().size()); } } diff --git a/resolver/src/test/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicyTest.java b/resolver/src/test/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicyTest.java index 8b65a02..934f4e3 100644 --- a/resolver/src/test/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicyTest.java +++ b/resolver/src/test/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicyTest.java @@ -40,7 +40,6 @@ public class TrustedForwarderPolicyTest { @Test public void getSPFRecordShouldNotChangeTheGivenRecord() throws Exception { - // FallbackPolicy hands the same record to every check SPF1Record record = new RFC4408SPF1Parser(new DefaultTermsFactory()).parse("v=spf1 -all"); new TrustedForwarderPolicy().getSPFRecord("example.com", record); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
