From 2ea1d2507fa753b615a64a359437dc7331b94832 Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Tue, 29 Sep 2026 11:43:52 -0400 Subject: [PATCH] Prevent stack overflow, log exceptions as warnings, reset only when necessary Improvements to reset to defaults logic. Prevent stack overflow when resetting creates new objects. When Maps and Collections throw exceptions log them as warnings. Do not reset values that have not changed. --- .../ConfigurationPropertiesRebinder.java | 101 +++++++- ...ebinderUnstableNestedIntegrationTests.java | 234 ++++++++++++++++++ ...CheckServiceInstanceListSupplierTests.java | 2 +- 3 files changed, 323 insertions(+), 14 deletions(-) create mode 100644 spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderUnstableNestedIntegrationTests.java diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java b/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java index 48fd3c3527..eb6f037048 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java +++ b/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 neverResetNestedTypes; + private final Set 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 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 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 diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderUnstableNestedIntegrationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderUnstableNestedIntegrationTests.java new file mode 100644 index 0000000000..aae4221eed --- /dev/null +++ b/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 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 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 findTestProperties() { + for (PropertySource source : this.environment.getPropertySources()) { + if (source.getName().toLowerCase().contains("test")) { + @SuppressWarnings("unchecked") + Map map = (Map) 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 labels = new LinkedHashMap<>(); + + // Read-only: exposes an unmodifiable view over the mutable backing map. + public Map getLabels() { + return Collections.unmodifiableMap(this.labels); + } + + public void setLabel(Map 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(); + } + + } + +} diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplierTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplierTests.java index e5aa4a7bd9..ae8d78b413 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplierTests.java +++ b/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 healthCheckFunctions() {