From 8e268987ff3ce3f5e6d30edf296c8d6f84716e76 Mon Sep 17 00:00:00 2001
From: Phillip Webb <pwebb@pivotal.io>
Date: Wed, 29 May 2019 16:05:24 -0700
Subject: [PATCH] Improve thread safety in property source cache

Update `SpringIterableConfigurationPropertySource` so that they cache
and cache key are not stored in different fields. Prior to this commit
it was possible that the an incorrect cache could be returned from
because the key and cache were out of sync.

This commit also allows more lenient handling of ConcurrentModification
exceptions if they are thrown during cache retrieval.

Closes gh-17017
See gh-17013
---
 ...ngIterableConfigurationPropertySource.java | 34 ++++++++----
 ...rableConfigurationPropertySourceTests.java | 52 +++++++++++++++++++
 2 files changed, 77 insertions(+), 9 deletions(-)

diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java
index fb82b6877b..b8ad0cdc5d 100644
--- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java
+++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java
@@ -18,6 +18,7 @@ package org.springframework.boot.context.properties.source;
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.ConcurrentModificationException;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
@@ -44,8 +45,6 @@ import org.springframework.util.ObjectUtils;
 class SpringIterableConfigurationPropertySource extends SpringConfigurationPropertySource
 		implements IterableConfigurationPropertySource {
 
-	private volatile Object cacheKey;
-
 	private volatile Cache cache;
 
 	SpringIterableConfigurationPropertySource(EnumerablePropertySource<?> propertySource,
@@ -131,16 +130,23 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope
 	}
 
 	private Cache getCache() {
-		CacheKey cacheKey = CacheKey.get(getPropertySource());
-		if (cacheKey == null) {
+		CacheKey key = CacheKey.get(getPropertySource());
+		if (key == null) {
 			return null;
 		}
-		if (ObjectUtils.nullSafeEquals(cacheKey, this.cacheKey)) {
-			return this.cache;
+		Cache cache = this.cache;
+		try {
+			if (cache != null && cache.hasKeyEqualTo(key)) {
+				return cache;
+			}
+			cache = new Cache(key.copy());
+			this.cache = cache;
+			return cache;
+		}
+		catch (ConcurrentModificationException ex) {
+			// Not fatal at this point, we can continue without a cache
+			return null;
 		}
-		this.cache = new Cache();
-		this.cacheKey = cacheKey.copy();
-		return this.cache;
 	}
 
 	@Override
@@ -150,10 +156,20 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope
 
 	private static class Cache {
 
+		private final CacheKey key;
+
 		private List<ConfigurationPropertyName> names;
 
 		private PropertyMapping[] mappings;
 
+		Cache(CacheKey key) {
+			this.key = key;
+		}
+
+		public boolean hasKeyEqualTo(CacheKey key) {
+			return this.key.equals(key);
+		}
+
 		public List<ConfigurationPropertyName> getNames() {
 			return this.names;
 		}
diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java
index 0ff04edee0..edbe8c7899 100644
--- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java
+++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java
@@ -16,8 +16,12 @@
 
 package org.springframework.boot.context.properties.source;
 
+import java.util.ConcurrentModificationException;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
 import java.util.Map;
+import java.util.Set;
 
 import org.junit.Test;
 
@@ -169,6 +173,21 @@ public class SpringIterableConfigurationPropertySourceTests {
 		assertThat(adapter.stream().count()).isEqualTo(3);
 	}
 
+	@Test
+	public void concurrentModificationExceptionInvalidatesCache() {
+		// gh-17013
+		ConcurrentModificationThrowingMap<String, Object> map = new ConcurrentModificationThrowingMap<>();
+		map.put("key1", "value1");
+		map.put("key2", "value2");
+		EnumerablePropertySource<?> source = new MapPropertySource("test", map);
+		SpringIterableConfigurationPropertySource adapter = new SpringIterableConfigurationPropertySource(
+				source, DefaultPropertyMapper.INSTANCE);
+		assertThat(adapter.stream().count()).isEqualTo(2);
+		map.setThrowException(true);
+		map.put("key3", "value3");
+		assertThat(adapter.stream().count()).isEqualTo(3);
+	}
+
 	/**
 	 * Test {@link PropertySource} that's also an {@link OriginLookup}.
 	 */
@@ -206,4 +225,37 @@ public class SpringIterableConfigurationPropertySourceTests {
 
 	}
 
+	private static class ConcurrentModificationThrowingMap<K, V>
+			extends LinkedHashMap<K, V> {
+
+		private boolean throwException;
+
+		public void setThrowException(boolean throwException) {
+			this.throwException = throwException;
+		}
+
+		@Override
+		public Set<K> keySet() {
+			return new KeySet(super.keySet());
+		}
+
+		private class KeySet extends LinkedHashSet<K> {
+
+			KeySet(Set<K> keySet) {
+				super(keySet);
+			}
+
+			@Override
+			public Iterator<K> iterator() {
+				if (ConcurrentModificationThrowingMap.this.throwException) {
+					ConcurrentModificationThrowingMap.this.throwException = false;
+					throw new ConcurrentModificationException();
+				}
+				return super.iterator();
+			}
+
+		}
+
+	}
+
 }