Skip to content

Commit 962f2d0

Browse files
jhouserizerjhouserizer
authored
Merge pull request #3268 from chrisdennis/eviction-security-fix
Traversers start at the start and are recycled
2 parents 900433f + d97adf7 commit 962f2d0

File tree

6 files changed

+52
-72
lines changed

6 files changed

+52
-72
lines changed

‎ehcache-impl/src/main/java/org/ehcache/impl/internal/store/heap/Backend.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import java.util.Comparator;
2626
import java.util.Iterator;
2727
import java.util.Map;
28-
import java.util.Random;
2928
import java.util.function.BiFunction;
3029

3130
/**
@@ -76,5 +75,5 @@ interface Backend<K, V> {
7675

7776
void updateUsageInBytesIfRequired(long delta);
7877

79-
Map.Entry<K, OnHeapValueHolder<V>> getEvictionCandidate(Random random, int size, final Comparator<? super Store.ValueHolder<V>> prioritizer, final EvictionAdvisor<Object, ? super OnHeapValueHolder<?>> evictionAdvisor);
78+
Map.Entry<K, OnHeapValueHolder<V>> getEvictionCandidate(int size, final Comparator<? super Store.ValueHolder<V>> prioritizer, final EvictionAdvisor<Object, ? super OnHeapValueHolder<?>> evictionAdvisor);
8079
}

‎ehcache-impl/src/main/java/org/ehcache/impl/internal/store/heap/OnHeapStore.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1567,14 +1567,13 @@ protected void enforceCapacity() {
15671567
*/
15681568
boolean evict(StoreEventSink<K, V> eventSink) {
15691569
evictionObserver.begin();
1570-
Random random = new Random();
15711570

15721571
@SuppressWarnings("unchecked")
1573-
Map.Entry<K, OnHeapValueHolder<V>> candidate = map.getEvictionCandidate(random, SAMPLE_SIZE, EVICTION_PRIORITIZER, EVICTION_ADVISOR);
1572+
Map.Entry<K, OnHeapValueHolder<V>> candidate = map.getEvictionCandidate(SAMPLE_SIZE, EVICTION_PRIORITIZER, EVICTION_ADVISOR);
15741573

15751574
if (candidate == null) {
15761575
// 2nd attempt without any advisor
1577-
candidate = map.getEvictionCandidate(random, SAMPLE_SIZE, EVICTION_PRIORITIZER, noAdvice());
1576+
candidate = map.getEvictionCandidate(SAMPLE_SIZE, EVICTION_PRIORITIZER, noAdvice());
15781577
}
15791578

15801579
if (candidate == null) {

‎ehcache-impl/src/main/java/org/ehcache/impl/internal/store/heap/SimpleBackend.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,8 @@ public boolean remove(K key, OnHeapValueHolder<V> value) {
5050
}
5151

5252
@Override
53-
public Map.Entry<K, OnHeapValueHolder<V>> getEvictionCandidate(Random random, int size, final Comparator<? super Store.ValueHolder<V>> prioritizer, final EvictionAdvisor<Object, ? super OnHeapValueHolder<?>> evictionAdvisor) {
54-
return realMap.getEvictionCandidate(random, size, prioritizer, evictionAdvisor);
53+
public Map.Entry<K, OnHeapValueHolder<V>> getEvictionCandidate(int size, final Comparator<? super Store.ValueHolder<V>> prioritizer, final EvictionAdvisor<Object, ? super OnHeapValueHolder<?>> evictionAdvisor) {
54+
return realMap.getEvictionCandidate(size, prioritizer, evictionAdvisor);
5555
}
5656

5757
@Override

‎ehcache-impl/src/test/java/org/ehcache/impl/internal/concurrent/ConcurrentHashMapTest.java‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import java.util.Collection;
2323
import java.util.Map;
2424
import java.util.Map.Entry;
25-
import java.util.Random;
2625

2726
import static org.ehcache.config.Eviction.noAdvice;
2827
import static org.hamcrest.MatcherAssert.assertThat;
@@ -118,21 +117,21 @@ public int compareTo(BadHashKey o) {
118117
@Test
119118
public void testRandomSampleOnEmptyMap() {
120119
ConcurrentHashMap<String, String> map = new ConcurrentHashMap<>();
121-
assertThat(map.getEvictionCandidate(new Random(), 1, null, noAdvice()), nullValue());
120+
assertThat(map.getEvictionCandidate(1, null, noAdvice()), nullValue());
122121
}
123122

124123
@Test
125124
public void testEmptyRandomSample() {
126125
ConcurrentHashMap<String, String> map = new ConcurrentHashMap<>();
127126
map.put("foo", "bar");
128-
assertThat(map.getEvictionCandidate(new Random(), 0, null, noAdvice()), nullValue());
127+
assertThat(map.getEvictionCandidate(0, null, noAdvice()), nullValue());
129128
}
130129

131130
@Test
132131
public void testOversizedRandomSample() {
133132
ConcurrentHashMap<String, String> map = new ConcurrentHashMap<>();
134133
map.put("foo", "bar");
135-
Entry<String, String> candidate = map.getEvictionCandidate(new Random(), 2, null, noAdvice());
134+
Entry<String, String> candidate = map.getEvictionCandidate(2, null, noAdvice());
136135
assertThat(candidate.getKey(), is("foo"));
137136
assertThat(candidate.getValue(), is("bar"));
138137
}
@@ -143,7 +142,7 @@ public void testUndersizedRandomSample() {
143142
for (int i = 0; i < 1000; i++) {
144143
map.put(Integer.toString(i), Integer.toString(i));
145144
}
146-
Entry<String, String> candidate = map.getEvictionCandidate(new Random(), 2, (t, t1) -> 0, noAdvice());
145+
Entry<String, String> candidate = map.getEvictionCandidate(2, (t, t1) -> 0, noAdvice());
147146
assertThat(candidate, notNullValue());
148147
}
149148

@@ -153,7 +152,7 @@ public void testFullyAdvisedAgainstEvictionRandomSample() {
153152
for (int i = 0; i < 1000; i++) {
154153
map.put(Integer.toString(i), Integer.toString(i));
155154
}
156-
Entry<String, String> candidate = map.getEvictionCandidate(new Random(), 2, null, (key, value) -> true);
155+
Entry<String, String> candidate = map.getEvictionCandidate(2, null, (key, value) -> true);
157156
assertThat(candidate, nullValue());
158157
}
159158

@@ -163,7 +162,7 @@ public void testSelectivelyAdvisedAgainstEvictionRandomSample() {
163162
for (int i = 0; i < 1000; i++) {
164163
map.put(Integer.toString(i), Integer.toString(i));
165164
}
166-
Entry<String, String> candidate = map.getEvictionCandidate(new Random(), 20, (t, t1) -> 0, (key, value) -> key.length() > 1);
165+
Entry<String, String> candidate = map.getEvictionCandidate(20, (t, t1) -> 0, (key, value) -> key.length() > 1);
167166
assertThat(candidate.getKey().length(), is(1));
168167
}
169168

‎ehcache-impl/src/unsafe/java/org/ehcache/impl/internal/concurrent/ConcurrentHashMap.java‎

Lines changed: 40 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import java.util.Arrays;
3737
import java.util.Collection;
3838
import java.util.Comparator;
39+
import java.util.Deque;
3940
import java.util.Enumeration;
4041
import java.util.HashMap;
4142
import java.util.Hashtable;
@@ -46,6 +47,7 @@
4647
import java.util.Random;
4748
import java.util.Set;
4849
import java.util.Spliterator;
50+
import java.util.concurrent.ConcurrentLinkedDeque;
4951
import java.util.concurrent.CountedCompleter;
5052
import java.util.concurrent.ForkJoinPool;
5153
import java.util.concurrent.atomic.AtomicReference;
@@ -6478,72 +6480,54 @@ else if (f.hash == MOVED)
64786480
return invalidated;
64796481
}
64806482

6481-
public Entry<K, V> getEvictionCandidate(Random rndm, int size, Comparator<? super V> prioritizer, EvictionAdvisor<? super K, ? super V> evictionAdvisor) {
6482-
Node<K,V>[] tab = table;
6483-
if (tab == null || size == 0) {
6484-
return null;
6485-
}
6483+
private final Deque<Traverser<K, V>> evictionTraversers = new ConcurrentLinkedDeque<>();
64866484

6487-
K maxKey = null;
6488-
V maxValue = null;
6485+
public Entry<K, V> getEvictionCandidate(int size, Comparator<? super V> prioritizer, EvictionAdvisor<? super K, ? super V> evictionAdvisor) {
6486+
if (size == 0) {
6487+
return null;
6488+
}
64896489

6490-
int n = tab.length;
6491-
int start = rndm.nextInt(n);
6490+
K maxKey = null;
6491+
V maxValue = null;
64926492

6493-
Traverser<K, V> t = new Traverser<>(tab, n, start, n);
6494-
for (Node<K, V> p; (p = t.advance()) != null;) {
6495-
K key = p.key;
6496-
V val = p.val;
6497-
if (!evictionAdvisor.adviseAgainstEviction(key, val)) {
6498-
if (maxKey == null || prioritizer.compare(val, maxValue) > 0) {
6499-
maxKey = key;
6500-
maxValue = val;
6501-
}
6502-
if (--size == 0) {
6503-
for (int terminalIndex = t.index; (p = t.advance()) != null && t.index == terminalIndex; ) {
6504-
key = p.key;
6505-
val = p.val;
6506-
if (!evictionAdvisor.adviseAgainstEviction(key, val) && prioritizer.compare(val, maxValue) > 0) {
6507-
maxKey = key;
6508-
maxValue = val;
6509-
}
6510-
}
6511-
return new MapEntry<>(maxKey, maxValue, this);
6512-
}
6513-
}
6493+
boolean exhaustive = false;
6494+
do {
6495+
Traverser<K, V> t = evictionTraversers.poll();
6496+
if (t == null) {
6497+
Node<K, V>[] tab;
6498+
int f = (tab = table) == null ? 0 : tab.length;
6499+
t = new Traverser<>(tab, f, 0, f);
6500+
exhaustive = true;
65146501
}
65156502

6516-
return getEvictionCandidateWrap(tab, start, size, maxKey, maxValue, prioritizer, evictionAdvisor);
6517-
}
6518-
6519-
private Entry<K, V> getEvictionCandidateWrap(Node<K,V>[] tab, int start, int size, K maxKey, V maxVal, Comparator<? super V> prioritizer, EvictionAdvisor<? super K, ? super V> evictionAdvisor) {
6520-
Traverser<K, V> t = new Traverser<>(tab, tab.length, 0, start);
6521-
for (Node<K, V> p; (p = t.advance()) != null;) {
6503+
boolean exhausted = false;
6504+
try {
6505+
for (Node<K, V> p; (p = t.advance()) != null; ) {
65226506
K key = p.key;
65236507
V val = p.val;
65246508
if (!evictionAdvisor.adviseAgainstEviction(key, val)) {
6525-
if (maxKey == null || prioritizer.compare(val, maxVal) > 0) {
6526-
maxKey = key;
6527-
maxVal = val;
6528-
}
6529-
if (--size == 0) {
6530-
for (int terminalIndex = t.index; (p = t.advance()) != null && t.index == terminalIndex; ) {
6531-
key = p.key;
6532-
val = p.val;
6533-
if (!evictionAdvisor.adviseAgainstEviction(key, val) && prioritizer.compare(val, maxVal) > 0) {
6534-
maxKey = key;
6535-
maxVal = val;
6536-
}
6537-
}
6538-
return new MapEntry<>(maxKey, maxVal, this);
6539-
}
6509+
if (maxKey == null || prioritizer.compare(val, maxValue) > 0) {
6510+
maxKey = key;
6511+
maxValue = val;
6512+
}
6513+
if (--size == 0) {
6514+
return new MapEntry<>(maxKey, maxValue, this);
6515+
}
65406516
}
6517+
}
6518+
exhausted = true;
6519+
} finally {
6520+
if (!exhausted) {
6521+
evictionTraversers.push(t);
6522+
}
65416523
}
6542-
if (maxKey == null) {
6543-
return null;
6544-
} else {
6545-
return new MapEntry<>(maxKey, maxVal, this);
6546-
}
6524+
} while (!exhaustive);
6525+
6526+
if (maxKey == null) {
6527+
return null;
6528+
} else {
6529+
return new MapEntry<>(maxKey, maxValue, this);
6530+
}
65476531
}
65486532
// END OF EHCACHE SPECIFIC
65496533
}

‎ehcache-impl/src/unsafe/java/org/ehcache/impl/internal/concurrent/EvictingConcurrentMap.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,12 @@ public interface EvictingConcurrentMap<K, V> extends ConcurrentMap<K, V>{
2929
/**
3030
* Return the preferred entry to evict based on a sample of entries taken from the map.
3131
*
32-
* @param rndm Random implementation used to determine the sample randomly
3332
* @param size Number of sampled entries
3433
* @param prioritizer Prioritizer used to determine the best entry to evict in the sample
3534
* @param evictionAdvisor Can veto against the eviction of an entry
3635
* @return Entry to evict or null is none was found
3736
*/
38-
Entry<K, V> getEvictionCandidate(Random rndm, int size, Comparator<? super V> prioritizer, EvictionAdvisor<? super K, ? super V> evictionAdvisor);
37+
Entry<K, V> getEvictionCandidate(int size, Comparator<? super V> prioritizer, EvictionAdvisor<? super K, ? super V> evictionAdvisor);
3938

4039
/**
4140
* Returns the number of mappings. This method should be used

0 commit comments

Comments
 (0)