From f4774d17a6cb906f35b866b8ca5b3b4752d87dad Mon Sep 17 00:00:00 2001
From: Jason Tedor <jason@tedor.me>
Date: Fri, 21 Aug 2015 10:43:48 -0400
Subject: [PATCH] Do not swallow exceptions thrown while parsing settings

This commit fixes an issue that was causing Elasticsearch to silently
ignore settings files that contain garbage. The underlying issue was
swallowing an SettingsException under the assumption that the only
reason that an exception could be throw was due to the settings file
not existing (in this case the IOException would be the cause of the
swallowed SettingsException). This assumption is mistaken as an
IOException could also be thrown due to an access error or a read
error. Additionally, a SettingsException could be thrown exactly
because garbage was found in the settings file. We should instead
explicitly check that the settings file exists, and bomb on an
exception thrown for any reason.

Closes #13028
---
 .../node/internal/InternalSettingsPreparer.java |  7 +++----
 .../internal/InternalSettingsPreparerTests.java | 17 +++++++++++++++--
 .../test/resources/config/garbage/garbage.yml   |  7 +++++++
 3 files changed, 25 insertions(+), 6 deletions(-)
 create mode 100644 core/src/test/resources/config/garbage/garbage.yml

diff --git a/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java b/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java
index 0f460274fc1..3b61502f723 100644
--- a/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java
+++ b/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java
@@ -114,10 +114,9 @@ public class InternalSettingsPreparer {
             }
             if (loadFromEnv) {
                 for (String allowedSuffix : ALLOWED_SUFFIXES) {
-                    try {
-                        settingsBuilder.loadFromPath(environment.configFile().resolve("elasticsearch" + allowedSuffix));
-                    } catch (SettingsException e) {
-                        // ignore
+                    Path path = environment.configFile().resolve("elasticsearch" + allowedSuffix);
+                    if (Files.exists(path)) {
+                        settingsBuilder.loadFromPath(path);
                     }
                 }
             }
diff --git a/core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java b/core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java
index f868c04d6f2..9d4324d8ce4 100644
--- a/core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java
+++ b/core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java
@@ -23,15 +23,15 @@ import org.elasticsearch.common.cli.CliToolTestCase;
 import org.elasticsearch.common.cli.Terminal;
 import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.settings.SettingsException;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.test.ESTestCase;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.io.IOException;
 import java.io.InputStream;
-import java.net.URL;
-import java.net.URLClassLoader;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
@@ -235,4 +235,17 @@ public class InternalSettingsPreparerTests extends ESTestCase {
         assertThat(settings.get("name"), is("prompted name 0"));
         assertThat(settings.get("node.name"), is("prompted name 0"));
     }
+
+    @Test(expected = SettingsException.class)
+    public void testGarbageIsNotSwallowed() throws IOException {
+        InputStream garbage = getClass().getResourceAsStream("/config/garbage/garbage.yml");
+        Path home = createTempDir();
+        Path config = home.resolve("config");
+        Files.createDirectory(config);
+        Files.copy(garbage, config.resolve("elasticsearch.yml"));
+        InternalSettingsPreparer.prepareSettings(settingsBuilder()
+                .put("config.ignore_system_properties", true)
+                .put("path.home", home)
+                .build(), true);
+    }
 }
diff --git a/core/src/test/resources/config/garbage/garbage.yml b/core/src/test/resources/config/garbage/garbage.yml
new file mode 100644
index 00000000000..36c5fdb52a2
--- /dev/null
+++ b/core/src/test/resources/config/garbage/garbage.yml
@@ -0,0 +1,7 @@
+SKDFLK@$#L%@KL#%L#@$#@L$ #L$@$ #L@K$#L $L $K#L#@L $#L
+!!@!@$(#%#)(@)% #(%)
+#(%#@)%@#)% (@#%()
+()#%@#% (@ )%@%(@#)% @( %)@ %(@)
+)(%)@()(%)()(#%)@#
+
+node.name: "Hiro Takachiho"