Prevent stack overflow when reseting values to default

spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java

@@ -55,6 +55,7 @@
 import org.springframework.jmx.export.annotation.ManagedOperation;
 import org.springframework.jmx.export.annotation.ManagedResource;
 import org.springframework.stereotype.Component;
+import org.springframework.util.ObjectUtils;
 import org.springframework.util.StringUtils;
 
 /**
@@ -97,6 +98,17 @@ public class ConfigurationPropertiesRebinder
 
 	private final Set<String> neverResetNestedTypes;
 
+	private final Set<String> loggedResetFailures = ConcurrentHashMap.newKeySet();
+
+	/**
+	 * Safety net against unbounded recursion in {@link #resetProperties}. Cyclic graphs
+	 * are already broken by the identity-based {@code visited} set, and graphs built from
+	 * unstable (newly-allocated-per-call) instances are broken by
+	 * {@link #isStableInstance(BeanWrapper, String, Object)}; this limit only guards
+	 * against a gap in either of those checks, or a legitimately very deep object graph.
+	 */
+	private static final int MAX_RESET_DEPTH = 25;
+
 	public ConfigurationPropertiesRebinder(ConfigurationPropertiesBeans beans) {
 		this(beans, Collections.emptySet());
 	}
@@ -253,7 +265,7 @@ private void resetBeanToDefaults(Object bean) {
 					+ " for reset; skipping property reset", ex);
 			return;
 		}
-		resetProperties(bean, freshInstance, Collections.newSetFromMap(new IdentityHashMap<>()));
+		resetProperties(bean, freshInstance, Collections.newSetFromMap(new IdentityHashMap<>()), 0);
 	}
 
 	/**
@@ -278,9 +290,11 @@ private boolean hasDefaultConstructor(Class<?> type) {
 		return constructors.length == 1 && constructors[0].getParameterCount() == 0;
 	}
 
-	private void resetProperties(Object bean, Object defaults, Set<Object> visited) {
-		// Guard against cyclic object graphs so that recursion always terminates.
-		if (bean == null || !visited.add(bean)) {
+	private void resetProperties(Object bean, Object defaults, Set<Object> visited, int depth) {
+		// Guard against cyclic object graphs and pathologically deep ones so that
+		// recursion
+		// always terminates; see MAX_RESET_DEPTH.
+		if (bean == null || depth > MAX_RESET_DEPTH || !visited.add(bean)) {
 			return;
 		}
 		BeanWrapper target = new BeanWrapperImpl(bean);
@@ -303,25 +317,50 @@ && isNeverReset(target.getPropertyValue(propertyName))) {
 						continue;
 					}
 					Object defaultValue = defaultsWrapper.getPropertyValue(propertyName);
-					target.setPropertyValue(propertyName, defaultValue);
+					Object currentValue = target.isReadableProperty(propertyName)
+							? target.getPropertyValue(propertyName) : null;
+					// Skip the setter call entirely when there's nothing to change: this
+					// avoids needlessly invoking setters that don't tolerate being called
+					// again with the same value, and setters that reject null are only
+					// ever
+					// invoked when the property actually needs to become null.
+					if (!ObjectUtils.nullSafeEquals(currentValue, defaultValue)) {
+						try {
+							target.setPropertyValue(propertyName, defaultValue);
+						}
+						catch (Exception ex) {
+							warnCannotReset(bean, propertyName, ex);
+						}
+					}
 				}
 				else if (target.isReadableProperty(propertyName) && defaultsWrapper.isReadableProperty(propertyName)) {
 					Object value = target.getPropertyValue(propertyName);
 					Object defaultValue = defaultsWrapper.getPropertyValue(propertyName);
 					if (value instanceof Collection collection) {
-						collection.clear();
-						if (defaultValue instanceof Collection defaultCollection) {
-							collection.addAll(defaultCollection);
+						try {
+							collection.clear();
+							if (defaultValue instanceof Collection defaultCollection) {
+								collection.addAll(defaultCollection);
+							}
+						}
+						catch (UnsupportedOperationException ex) {
+							warnCannotReset(bean, propertyName, ex);
 						}
 					}
 					else if (value instanceof Map map) {
-						map.clear();
-						if (defaultValue instanceof Map defaultMap) {
-							map.putAll(defaultMap);
+						try {
+							map.clear();
+							if (defaultValue instanceof Map defaultMap) {
+								map.putAll(defaultMap);
+							}
+						}
+						catch (UnsupportedOperationException ex) {
+							warnCannotReset(bean, propertyName, ex);
 						}
 					}
-					else if (value != null && defaultValue != null && isResettableNestedType(value.getClass())) {
-						resetProperties(value, defaultValue, visited);
+					else if (value != null && defaultValue != null && isResettableNestedType(value.getClass())
+							&& isStableInstance(target, propertyName, value)) {
+						resetProperties(value, defaultValue, visited, depth + 1);
 					}
 				}
 			}
@@ -334,6 +373,42 @@ else if (value != null && defaultValue != null && isResettableNestedType(value.g
 		}
 	}
 
+	/**
+	 * Whether re-reading the given read-only property returns the very same instance. A
+	 * getter that instead returns a new object on every call cannot be holding any state
+	 * that a reset would need to affect, so recursing into such a value would only ever
+	 * walk a throw-away object graph - one that, for record-style/fluent accessors, may
+	 * not even terminate (see gh-1750).
+	 */
+	private boolean isStableInstance(BeanWrapper target, String propertyName, Object value) {
+		try {
+			return target.getPropertyValue(propertyName) == value;
+		}
+		catch (Exception ex) {
+			return false;
+		}
+	}
+
+	/**
+	 * Log, at most once per bean type and property, that a property could not be reset to
+	 * its default value and may therefore retain a stale value after this and future
+	 * refreshes. The full exception is only logged at DEBUG, on every occurrence, to
+	 * avoid spamming the log at the default level while still making the failure
+	 * discoverable.
+	 */
+	private void warnCannotReset(Object bean, String propertyName, Exception ex) {
+		String targetClassName = AopUtils.getTargetClass(bean).getName();
+		if (this.loggedResetFailures.add(targetClassName + "#" + propertyName)) {
+			logger.warn("Cannot reset property '" + propertyName + "' on " + targetClassName
+					+ " to its default value; it may retain its previous value after this and future refreshes. "
+					+ "Enable DEBUG logging for " + ConfigurationPropertiesRebinder.class.getName()
+					+ " for the full exception.");
+		}
+		if (logger.isDebugEnabled()) {
+			logger.debug("Failed to reset property '" + propertyName + "' on " + targetClassName, ex);
+		}
+	}
+
 	/**
 	 * Determine whether a nested property value should be recursively reset. Only
 	 * user-defined types are descended into. Recursing into JDK or standard API types

spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderUnstableNestedIntegrationTests.java

@@ -0,0 +1,234 @@
+/*
+ * Copyright 2012-present the original author or authors.
+ *
+ * Licensed 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
+ *
+ *      https://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.springframework.cloud.context.properties;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.autoconfigure.context.PropertyPlaceholderAutoConfiguration;
+import org.springframework.boot.context.properties.ConfigurationProperties;
+import org.springframework.boot.context.properties.EnableConfigurationProperties;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.boot.test.system.CapturedOutput;
+import org.springframework.boot.test.system.OutputCaptureExtension;
+import org.springframework.cloud.autoconfigure.ConfigurationPropertiesRebinderAutoConfiguration;
+import org.springframework.cloud.autoconfigure.RefreshAutoConfiguration;
+import org.springframework.cloud.context.properties.ConfigurationPropertiesRebinderUnstableNestedIntegrationTests.TestConfiguration;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Import;
+import org.springframework.core.env.ConfigurableEnvironment;
+import org.springframework.core.env.PropertySource;
+import org.springframework.test.annotation.DirtiesContext;
+
+import static org.assertj.core.api.BDDAssertions.then;
+import static org.assertj.core.data.MapEntry.entry;
+
+/**
+ * Reproduces the three problems reported in gh-1750 against
+ * {@code ConfigurationPropertiesRebinder.resetProperties}: a {@link StackOverflowError}
+ * for read-only getters that return a new instance on every call, silently-skipped resets
+ * for unmodifiable collections/maps, and silently-skipped resets for setters that reject
+ * {@code null}.
+ *
+ * @author Ryan Baxter
+ */
+@SpringBootTest(classes = TestConfiguration.class,
+		properties = { "test.readonly.label.a=1", "test.readonly.label.b=2",
+				"test.nullrejecting.endpoint=https://old.example.com" })
+@ExtendWith(OutputCaptureExtension.class)
+public class ConfigurationPropertiesRebinderUnstableNestedIntegrationTests {
+
+	@Autowired
+	private TestProperties properties;
+
+	@Autowired
+	private ConfigurationPropertiesRebinder rebinder;
+
+	@Autowired
+	private ConfigurableEnvironment environment;
+
+	@Test
+	@DirtiesContext
+	public void rebindDoesNotStackOverflowForFluentAccessorReturningNewInstance() {
+		// FluentProperties.getSettings() returns a new instance on every call, and
+		// Settings.protect() does the same, so a naive recursive reset never terminates.
+		// The rebind must complete rather than throw StackOverflowError.
+		this.rebinder.rebind();
+		then(this.properties.getFluent().getName()).isEqualTo("default");
+	}
+
+	@Test
+	@DirtiesContext
+	public void removedEntryLogsWarningForUnmodifiableView(CapturedOutput output) {
+		then(this.properties.getReadonly().getLabels()).containsExactly(entry("a", "1"), entry("b", "2"));
+		Map<String, Object> map = findTestProperties();
+		map.remove("test.readonly.label.b");
+		this.rebinder.rebind();
+		// getLabels() only exposes an unmodifiable view over the private backing map, so
+		// the
+		// removed entry can't actually be cleared through the bean's public API - but the
+		// failure must now be visible instead of silently swallowed at DEBUG.
+		then(output).contains("Cannot reset property 'labels'");
+	}
+
+	@Test
+	@DirtiesContext
+	public void removedPropertyLogsWarningForNullRejectingSetter(CapturedOutput output) {
+		then(this.properties.getNullrejecting().getEndpoint()).isEqualTo("https://old.example.com");
+		Map<String, Object> map = findTestProperties();
+		map.remove("test.nullrejecting.endpoint");
+		this.rebinder.rebind();
+		// The setter rejects null, so the stale value cannot be reset away, but the
+		// failure must now be visible instead of silently swallowed at DEBUG.
+		then(output).contains("Cannot reset property 'endpoint'");
+	}
+
+	private Map<String, Object> findTestProperties() {
+		for (PropertySource<?> source : this.environment.getPropertySources()) {
+			if (source.getName().toLowerCase().contains("test")) {
+				@SuppressWarnings("unchecked")
+				Map<String, Object> map = (Map<String, Object>) source.getSource();
+				return map;
+			}
+		}
+		throw new IllegalStateException("Could not find test property source");
+	}
+
+	@Configuration(proxyBeanMethods = false)
+	@EnableConfigurationProperties(RefreshAutoConfiguration.RefreshProperties.class)
+	@Import({ RefreshConfiguration.RebinderConfiguration.class, PropertyPlaceholderAutoConfiguration.class })
+	protected static class TestConfiguration {
+
+		@Bean
+		protected TestProperties testProperties() {
+			return new TestProperties();
+		}
+
+	}
+
+	// Hack out a protected inner class for testing
+	protected static class RefreshConfiguration extends RefreshAutoConfiguration {
+
+		@Configuration(proxyBeanMethods = false)
+		protected static class RebinderConfiguration extends ConfigurationPropertiesRebinderAutoConfiguration {
+
+			public RebinderConfiguration(ApplicationContext context) {
+				super(context);
+			}
+
+		}
+
+	}
+
+	@ConfigurationProperties("test")
+	protected static class TestProperties {
+
+		private final FluentProperties fluent = new FluentProperties();
+
+		private final ReadOnlyViewProperties readonly = new ReadOnlyViewProperties();
+
+		private final NullRejectingSetterProperties nullrejecting = new NullRejectingSetterProperties();
+
+		public FluentProperties getFluent() {
+			return this.fluent;
+		}
+
+		public ReadOnlyViewProperties getReadonly() {
+			return this.readonly;
+		}
+
+		public NullRejectingSetterProperties getNullrejecting() {
+			return this.nullrejecting;
+		}
+
+	}
+
+	protected static class FluentProperties {
+
+		private String name = "default";
+
+		public String getName() {
+			return this.name;
+		}
+
+		public void setName(String name) {
+			this.name = name;
+		}
+
+		// Read-only: returns a new instance on every call.
+		public Settings getSettings() {
+			return new Settings();
+		}
+
+		protected static class Settings {
+
+			private boolean protect;
+
+			public boolean isProtect() {
+				return this.protect;
+			}
+
+			// Record-style accessor (no "get" prefix, matches the "protect" field) that
+			// returns a new, protected copy on every call - exactly like
+			// org.infinispan.commons.configuration.attributes.AttributeSet#protect().
+			public Settings protect() {
+				Settings copy = new Settings();
+				copy.protect = true;
+				return copy;
+			}
+
+		}
+
+	}
+
+	protected static class ReadOnlyViewProperties {
+
+		private final Map<String, String> labels = new LinkedHashMap<>();
+
+		// Read-only: exposes an unmodifiable view over the mutable backing map.
+		public Map<String, String> getLabels() {
+			return Collections.unmodifiableMap(this.labels);
+		}
+
+		public void setLabel(Map<String, String> label) {
+			this.labels.putAll(label);
+		}
+
+	}
+
+	protected static class NullRejectingSetterProperties {
+
+		private String endpoint;
+
+		public String getEndpoint() {
+			return this.endpoint;
+		}
+
+		public void setEndpoint(String endpoint) {
+			this.endpoint = endpoint.strip();
+		}
+
+	}
+
+}

spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplierTests.java

@@ -718,7 +718,7 @@ void shouldCheckUserProvidedPortForHealthCheckRequest() {
 			listSupplier.isAlive(serviceInstance).block();
 		});
 
-		assertThat(exception).hasMessageContaining("Connection refused: /127.0.0.1:8888");
+		assertThat(exception).hasMessageContaining("Connection refused");
 	}
 
 	private static Stream<Arguments> healthCheckFunctions() {