[CONFIGURATION-760] Properties file using cyclical includes cause a StackOverflowError instead of detecting the misconfiguration. (#35)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 149f142..5724ccc 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -72,6 +72,9 @@
<action dev="ggregory" type="update" issue="CONFIGURATION-759" due-to="Gary Gregory">
Update Spring from 4.3.24.RELEASE to 4.3.25.RELEASE.
</action>
+ <action dev="ggregory" type="fix" issue="CONFIGURATION-760" due-to="Gary Gregory">
+ Properties file using cyclical includes cause a StackOverflowError instead of detecting the misconfiguration.
+ </action>
</release>
<release version="2.5" date="2019-05-23"
description="Minor release with new features and updated dependencies.">
diff --git a/src/main/java/org/apache/commons/configuration2/PropertiesConfiguration.java b/src/main/java/org/apache/commons/configuration2/PropertiesConfiguration.java
index 438bba8..4df6827 100644
--- a/src/main/java/org/apache/commons/configuration2/PropertiesConfiguration.java
+++ b/src/main/java/org/apache/commons/configuration2/PropertiesConfiguration.java
@@ -27,6 +27,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
+import java.util.Deque;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -643,11 +644,12 @@
*
* @param key the property key
* @param value the property value
+ * @param seenStack the stack of seen include URLs
* @return a flag whether this is a normal property
* @throws ConfigurationException if an error occurs
* @since 1.3
*/
- boolean propertyLoaded(final String key, final String value)
+ boolean propertyLoaded(final String key, final String value, final Deque<URL> seenStack)
throws ConfigurationException
{
boolean result;
@@ -661,7 +663,7 @@
getListDelimiterHandler().split(value, true);
for (final String f : files)
{
- loadIncludeFile(interpolate(f), false);
+ loadIncludeFile(interpolate(f), false, seenStack);
}
}
result = false;
@@ -676,7 +678,7 @@
getListDelimiterHandler().split(value, true);
for (final String f : files)
{
- loadIncludeFile(interpolate(f), true);
+ loadIncludeFile(interpolate(f), true, seenStack);
}
}
result = false;
@@ -1853,9 +1855,11 @@
*
* @param fileName the name of the file to load
* @param optional whether or not the {@code fileName} is optional
+ * @param seenStack Stack of seen include URLs
* @throws ConfigurationException if loading fails
*/
- private void loadIncludeFile(final String fileName, final boolean optional) throws ConfigurationException
+ private void loadIncludeFile(final String fileName, final boolean optional, final Deque<URL> seenStack)
+ throws ConfigurationException
{
if (locator == null)
{
@@ -1881,11 +1885,8 @@
if (url == null)
{
- if (getIncludeListener() != null)
- {
- getIncludeListener().accept(new ConfigurationException(
- "Cannot resolve include file " + fileName, new FileNotFoundException(fileName)));
- }
+ getIncludeListener().accept(new ConfigurationException("Cannot resolve include file " + fileName,
+ new FileNotFoundException(fileName)));
}
else
{
@@ -1896,14 +1897,25 @@
{
try
{
- fh.load(url);
+ // Check for cycles
+ if (seenStack.contains(url))
+ {
+ throw new ConfigurationException(
+ String.format("Cycle detected loading %s, seen stack: %s", url, seenStack));
+ }
+ seenStack.add(url);
+ try
+ {
+ fh.load(url);
+ }
+ finally
+ {
+ seenStack.pop();
+ }
}
catch (ConfigurationException e)
{
- if (getIncludeListener() != null)
- {
- getIncludeListener().accept(e);
- }
+ getIncludeListener().accept(e);
}
}
finally
diff --git a/src/main/java/org/apache/commons/configuration2/PropertiesConfigurationLayout.java b/src/main/java/org/apache/commons/configuration2/PropertiesConfigurationLayout.java
index dffff50..4da001f 100644
--- a/src/main/java/org/apache/commons/configuration2/PropertiesConfigurationLayout.java
+++ b/src/main/java/org/apache/commons/configuration2/PropertiesConfigurationLayout.java
@@ -19,6 +19,8 @@
import java.io.IOException;
import java.io.Reader;
import java.io.Writer;
+import java.net.URL;
+import java.util.ArrayDeque;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
@@ -138,6 +140,9 @@
/** Stores the force single line flag. */
private boolean forceSingleLine;
+ /** Seen includes. */
+ private final ArrayDeque<URL> seenStack = new ArrayDeque<>();
+
/**
* Creates a new, empty instance of {@code PropertiesConfigurationLayout}.
*/
@@ -486,7 +491,7 @@
while (reader.nextProperty())
{
if (config.propertyLoaded(reader.getPropertyName(),
- reader.getPropertyValue()))
+ reader.getPropertyValue(), seenStack))
{
final boolean contained = layoutData.containsKey(reader
.getPropertyName());
@@ -653,6 +658,7 @@
*/
private void clear()
{
+ seenStack.clear();
layoutData.clear();
setHeaderComment(null);
setFooterComment(null);
diff --git a/src/test/java/org/apache/commons/configuration2/TestPropertiesConfiguration.java b/src/test/java/org/apache/commons/configuration2/TestPropertiesConfiguration.java
index bd1e532..5b5e8f5 100644
--- a/src/test/java/org/apache/commons/configuration2/TestPropertiesConfiguration.java
+++ b/src/test/java/org/apache/commons/configuration2/TestPropertiesConfiguration.java
@@ -49,6 +49,7 @@
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Paths;
+import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
@@ -73,8 +74,8 @@
import org.apache.commons.configuration2.io.FileHandler;
import org.apache.commons.configuration2.io.FileSystem;
import org.apache.commons.lang3.mutable.MutableObject;
+import org.junit.Assert;
import org.junit.Before;
-import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
@@ -1147,8 +1148,82 @@
}
@Test
- @Ignore("PropertiesConfiguration does NOT detect cyclical references.")
- public void testIncludeLoadAllCycliclaReference() throws Exception
+ public void testIncludeLoadCyclicalReferenceFail() throws Exception
+ {
+ final PropertiesConfiguration pc = new PropertiesConfiguration();
+ final FileHandler handler = new FileHandler(pc);
+ handler.setBasePath(testBasePath);
+ handler.setFileName("include-cyclical-reference.properties");
+ try {
+ handler.load();
+ Assert.fail("Expected " + Configuration.class.getCanonicalName());
+ } catch (ConfigurationException e) {
+ // expected
+ // e.printStackTrace();
+ }
+ assertNull(pc.getString("keyA"));
+ }
+
+ @Test
+ public void testIncludeLoadCyclicalMultiStepReferenceFail() throws Exception
+ {
+ final PropertiesConfiguration pc = new PropertiesConfiguration();
+ final FileHandler handler = new FileHandler(pc);
+ handler.setBasePath(testBasePath);
+ handler.setFileName("include-cyclical-root.properties");
+ try {
+ handler.load();
+ Assert.fail("Expected " + Configuration.class.getCanonicalName());
+ } catch (ConfigurationException e) {
+ // expected
+ // e.printStackTrace();
+ }
+ assertNull(pc.getString("keyA"));
+ }
+
+ @Test
+ public void testIncludeLoadCyclicalMultiStepReferenceIgnore() throws Exception
+ {
+ final PropertiesConfiguration pc = new PropertiesConfiguration();
+ pc.setIncludeListener(PropertiesConfiguration.NOOP_INCLUDE_LISTENER);
+ final FileHandler handler = new FileHandler(pc);
+ handler.setBasePath(testBasePath);
+ handler.setFileName("include-cyclical-root.properties");
+ handler.load();
+ assertEquals("valueA", pc.getString("keyA"));
+ }
+
+ @Test
+ public void testIncludeIncludeLoadCyclicalReferenceFail() throws Exception
+ {
+ final PropertiesConfiguration pc = new PropertiesConfiguration();
+ final FileHandler handler = new FileHandler(pc);
+ handler.setBasePath(testBasePath);
+ handler.setFileName("include-include-cyclical-reference.properties");
+ try {
+ handler.load();
+ Assert.fail("Expected " + Configuration.class.getCanonicalName());
+ } catch (ConfigurationException e) {
+ // expected
+ // e.printStackTrace();
+ }
+ assertNull(pc.getString("keyA"));
+ }
+
+ @Test
+ public void testIncludeIncludeLoadCyclicalReferenceIgnore() throws Exception
+ {
+ final PropertiesConfiguration pc = new PropertiesConfiguration();
+ pc.setIncludeListener(PropertiesConfiguration.NOOP_INCLUDE_LISTENER);
+ final FileHandler handler = new FileHandler(pc);
+ handler.setBasePath(testBasePath);
+ handler.setFileName("include-include-cyclical-reference.properties");
+ handler.load();
+ assertEquals("valueA", pc.getString("keyA"));
+ }
+
+ @Test
+ public void testIncludeLoadCyclicalReferenceIgnore() throws Exception
{
final PropertiesConfiguration pc = new PropertiesConfiguration();
pc.setIncludeListener(PropertiesConfiguration.NOOP_INCLUDE_LISTENER);
@@ -1231,7 +1306,7 @@
{
final DummyLayout layout = new DummyLayout();
conf.setLayout(layout);
- conf.propertyLoaded("layoutLoadedProperty", "yes");
+ conf.propertyLoaded("layoutLoadedProperty", "yes", null);
assertEquals("Layout's load() was called", 0, layout.loadCalls);
assertEquals("Property not added", "yes", conf.getString("layoutLoadedProperty"));
}
@@ -1244,7 +1319,8 @@
{
final DummyLayout layout = new DummyLayout();
conf.setLayout(layout);
- conf.propertyLoaded(PropertiesConfiguration.getInclude(), "testClasspath.properties,testEqual.properties");
+ conf.propertyLoaded(PropertiesConfiguration.getInclude(), "testClasspath.properties,testEqual.properties",
+ new ArrayDeque<>());
assertEquals("Layout's load() was not correctly called", 2, layout.loadCalls);
assertFalse("Property was added", conf.containsKey(PropertiesConfiguration.getInclude()));
}
@@ -1259,7 +1335,7 @@
final DummyLayout layout = new DummyLayout();
conf.setLayout(layout);
conf.setIncludesAllowed(false);
- conf.propertyLoaded(PropertiesConfiguration.getInclude(), "testClassPath.properties,testEqual.properties");
+ conf.propertyLoaded(PropertiesConfiguration.getInclude(), "testClassPath.properties,testEqual.properties", null);
assertEquals("Layout's load() was called", 0, layout.loadCalls);
assertFalse("Property was added", conf.containsKey(PropertiesConfiguration.getInclude()));
}
diff --git a/src/test/java/org/apache/commons/configuration2/TestPropertiesConfigurationLayout.java b/src/test/java/org/apache/commons/configuration2/TestPropertiesConfigurationLayout.java
index 6b63d33..f5912ac 100644
--- a/src/test/java/org/apache/commons/configuration2/TestPropertiesConfigurationLayout.java
+++ b/src/test/java/org/apache/commons/configuration2/TestPropertiesConfigurationLayout.java
@@ -26,6 +26,8 @@
import java.io.Reader;
import java.io.StringReader;
import java.io.StringWriter;
+import java.net.URL;
+import java.util.Deque;
import java.util.Iterator;
import org.apache.commons.configuration2.convert.DefaultListDelimiterHandler;
@@ -830,12 +832,12 @@
* load() call on the layout is invoked.
*/
@Override
- boolean propertyLoaded(final String key, final String value)
+ boolean propertyLoaded(final String key, final String value, Deque<URL> seenStack)
throws ConfigurationException
{
if (builder == null)
{
- return super.propertyLoaded(key, value);
+ return super.propertyLoaded(key, value, seenStack);
}
if (PropertiesConfiguration.getInclude().equals(key))
{
diff --git a/src/test/resources/include-cyclical-back-to-root.properties b/src/test/resources/include-cyclical-back-to-root.properties
new file mode 100644
index 0000000..71a5b8d
--- /dev/null
+++ b/src/test/resources/include-cyclical-back-to-root.properties
@@ -0,0 +1,16 @@
+# 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.
+include = src/test/resources/include-cyclical-root.properties
+keyA = valueA
diff --git a/src/test/resources/include-cyclical-root.properties b/src/test/resources/include-cyclical-root.properties
new file mode 100644
index 0000000..5e18ec4
--- /dev/null
+++ b/src/test/resources/include-cyclical-root.properties
@@ -0,0 +1,16 @@
+# 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.
+include = src/test/resources/include-cyclical-step.properties
+keyA = valueA
diff --git a/src/test/resources/include-cyclical-step.properties b/src/test/resources/include-cyclical-step.properties
new file mode 100644
index 0000000..edee5d5
--- /dev/null
+++ b/src/test/resources/include-cyclical-step.properties
@@ -0,0 +1,16 @@
+# 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.
+include = src/test/resources/include-cyclical-back-to-root.properties
+keyA = valueA
diff --git a/src/test/resources/include-include-cyclical-reference.properties b/src/test/resources/include-include-cyclical-reference.properties
new file mode 100644
index 0000000..7a5ccf4
--- /dev/null
+++ b/src/test/resources/include-include-cyclical-reference.properties
@@ -0,0 +1,16 @@
+# 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.
+include = src/test/resources/include-cyclical-reference.properties
+keyA = valueA
