Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=51509

Fix potential concurrency issue in CSRF prevention filter that may lead to some requests failing that should not.

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1148471 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/java/org/apache/catalina/filters/CsrfPreventionFilter.java b/java/org/apache/catalina/filters/CsrfPreventionFilter.java
index 0189632..0ef0b84 100644 (file)
--- a/java/org/apache/catalina/filters/CsrfPreventionFilter.java
+++ b/java/org/apache/catalina/filters/CsrfPreventionFilter.java
@@ -310,11 +310,15 @@ public class CsrfPreventionFilter extends FilterBase {
         }
         
         public void add(T key) {
-            cache.put(key, null);
+            synchronized (cache) {
+                cache.put(key, null);
+            }
         }
-        
+
         public boolean contains(T key) {
-            return cache.containsKey(key);
+            synchronized (cache) {
+                return cache.containsKey(key);
+            }
         }
     }
 }

diff --git a/test/org/apache/catalina/filters/TestCsrfPreventionFilter2.java b/test/org/apache/catalina/filters/TestCsrfPreventionFilter2.java
new file mode 100644 (file)
index 0000000..2b9cb5c
--- /dev/null
+++ b/test/org/apache/catalina/filters/TestCsrfPreventionFilter2.java
@@ -0,0 +1,88 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one or more
+ *  contributor license agreements.  See the NOTICE file distributed with
+ *  this work for additional information regarding copyright ownership.
+ *  The ASF licenses this file to You under the Apache License, Version 2.0
+ *  (the "License"); you may not use this file except in compliance with
+ *  the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+
+package org.apache.catalina.filters;
+
+import junit.framework.TestCase;
+
+import org.apache.catalina.filters.CsrfPreventionFilter.LruCache;
+
+public class TestCsrfPreventionFilter2 extends TestCase {
+
+    /**
+     * When this test fails, it tends to enter a long running loop but it will
+     * eventually finish (after ~70s on a 8-core Windows box).
+     */
+    public void testLruCacheConcurrency() throws Exception {
+        int threadCount = 2;
+        long iterationCount = 100000L;
+        
+        assertTrue(threadCount > 1);
+
+        LruCache<String> cache = new LruCache<String>(threadCount - 1);
+        
+        LruTestThread[] threads = new LruTestThread[threadCount];
+        for (int i = 0; i < threadCount; i++) {
+            threads[i] = new LruTestThread(cache, iterationCount);
+        }
+        
+        for (int i = 0; i < threadCount; i++) {
+            threads[i].start();
+        }
+
+        for (int i = 0; i < threadCount; i++) {
+            threads[i].join();
+        }
+
+        for (int i = 0; i < threadCount; i++) {
+            assertTrue(threads[i].getResult());
+        }
+
+    }
+
+    private static class LruTestThread extends Thread {
+        private final LruCache<String> cache;
+        private long iterationCount = 0;
+        private volatile boolean result = false;
+
+        public LruTestThread(LruCache<String> cache, long iterationCount) {
+            this.cache = cache;
+            this.iterationCount = iterationCount;
+        }
+
+        public boolean getResult() {
+            return result;
+        }
+
+        @Override
+        public void run() {
+            String test = getName();
+            try {
+                for (long i = 0; i < iterationCount; i++) {
+                    cache.add(test + i);
+                    if (!cache.contains(test + i)) {
+                        // Expected
+                    }
+                }
+            } catch (Exception e) {
+                e.printStackTrace();
+                return;
+            }
+            result = true;
+        }
+    }
+}

diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 94b3f6e..3c2eed7 100644 (file)
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -63,6 +63,10 @@
         ignored when scanning jars for tag libraries. (kkolinko)
       </fix>
       <fix>
+        <bug>51509</bug>: Fix potential concurrency issue in CSRF prevention
+        filter that may lead to some requests failing that should not. (markt)
+      </fix>
+      <fix>
         <bug>51518</bug>: Correct error in web.xml parsing rules for the
         &lt;others/&gt; tag when using absolute ordering. (markt)
       </fix>