From ba1032191248866fa59e770a8c9c1ee9a457661f Mon Sep 17 00:00:00 2001 From: David Roberts Date: Mon, 15 Jan 2018 09:52:02 +0000 Subject: [PATCH 1/2] Enforce that java.io.tmpdir exists on startup If the default java.io.tmpdir is used then the startup script creates it, but if a custom java.io.tmpdir is used then the user must ensure it exists before running Elasticsearch. If they forget then it can cause errors that are hard to understand, so this change adds an explicit check early in the bootstrap and reports a clear error if java.io.tmpdir is not an accessible directory. --- .../elasticsearch/bootstrap/Bootstrap.java | 6 +++++ .../org/elasticsearch/env/Environment.java | 20 ++++++++++++++- .../elasticsearch/env/EnvironmentTests.java | 25 +++++++++++++++++-- 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java index 0a3d7f675c234..ea2da4d624d5c 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java @@ -288,6 +288,12 @@ static void init( } catch (IOException e) { throw new BootstrapException(e); } + // a misconfigured java.io.tmpdir can cause hard-to-diagnose problems later, so reject it immediately + try { + environment.validateTmpFile(); + } catch (IOException e) { + throw new BootstrapException(e); + } if (environment.pidFile() != null) { try { PidFile.create(environment.pidFile(), true); diff --git a/server/src/main/java/org/elasticsearch/env/Environment.java b/server/src/main/java/org/elasticsearch/env/Environment.java index 2433ccf6e8ede..1f4940007afda 100644 --- a/server/src/main/java/org/elasticsearch/env/Environment.java +++ b/server/src/main/java/org/elasticsearch/env/Environment.java @@ -27,6 +27,7 @@ import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; +import java.io.FileNotFoundException; import java.io.IOException; import java.net.MalformedURLException; import java.net.URISyntaxException; @@ -87,9 +88,14 @@ public class Environment { private final Path pidFile; /** Path to the temporary file directory used by the JDK */ - private final Path tmpFile = PathUtils.get(System.getProperty("java.io.tmpdir")); + private final Path tmpFile; public Environment(final Settings settings, final Path configPath) { + this(settings, configPath, PathUtils.get(System.getProperty("java.io.tmpdir"))); + } + + // Should only be called directly by this class's unit tests + Environment(final Settings settings, final Path configPath, final Path tmpPath) { final Path homeFile; if (PATH_HOME_SETTING.exists(settings)) { homeFile = PathUtils.get(PATH_HOME_SETTING.get(settings)).normalize(); @@ -103,6 +109,8 @@ public Environment(final Settings settings, final Path configPath) { configFile = homeFile.resolve("config"); } + tmpFile = Objects.requireNonNull(tmpPath); + pluginsFile = homeFile.resolve("plugins"); List dataPaths = PATH_DATA_SETTING.get(settings); @@ -302,6 +310,16 @@ public Path tmpFile() { return tmpFile; } + /** Ensure the configured temp directory is a valid directory */ + public void validateTmpFile() throws IOException { + if (Files.exists(tmpFile) == false) { + throw new FileNotFoundException("Temporary file directory [" + tmpFile + "] does not exist or is not accessible"); + } + if (Files.isDirectory(tmpFile) == false) { + throw new IOException("Configured temporary file directory [" + tmpFile + "] is not a directory"); + } + } + public static FileStore getFileStore(final Path path) throws IOException { return new ESFileStore(Files.getFileStore(path)); } diff --git a/server/src/test/java/org/elasticsearch/env/EnvironmentTests.java b/server/src/test/java/org/elasticsearch/env/EnvironmentTests.java index 5ca3f4dc6a591..5ada31b612941 100644 --- a/server/src/test/java/org/elasticsearch/env/EnvironmentTests.java +++ b/server/src/test/java/org/elasticsearch/env/EnvironmentTests.java @@ -21,6 +21,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.test.ESTestCase; +import java.io.FileNotFoundException; import java.io.IOException; import java.net.URL; import java.nio.file.Path; @@ -28,6 +29,7 @@ import static org.hamcrest.CoreMatchers.endsWith; import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.CoreMatchers.nullValue; +import static org.hamcrest.CoreMatchers.startsWith; import static org.hamcrest.Matchers.arrayWithSize; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -37,11 +39,11 @@ * Simple unit-tests for Environment.java */ public class EnvironmentTests extends ESTestCase { - public Environment newEnvironment() throws IOException { + public Environment newEnvironment() { return newEnvironment(Settings.EMPTY); } - public Environment newEnvironment(Settings settings) throws IOException { + public Environment newEnvironment(Settings settings) { Settings build = Settings.builder() .put(settings) .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toAbsolutePath()) @@ -146,4 +148,23 @@ public void testNodeDoesNotRequireLocalStorageButHasPathData() { assertThat(e, hasToString(containsString("node does not require local storage yet path.data is set to [" + pathData + "]"))); } + public void testNonExistentTempPathValidation() { + Settings build = Settings.builder() + .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir()) + .build(); + Environment environment = new Environment(build, null, createTempDir().resolve("this_does_not_exist")); + FileNotFoundException e = expectThrows(FileNotFoundException.class, environment::validateTmpFile); + assertThat(e.getMessage(), startsWith("Temporary file directory [")); + assertThat(e.getMessage(), endsWith("this_does_not_exist] does not exist or is not accessible")); + } + + public void testTempPathValidationWhenRegularFile() throws IOException { + Settings build = Settings.builder() + .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir()) + .build(); + Environment environment = new Environment(build, null, createTempFile("something", ".test")); + IOException e = expectThrows(IOException.class, environment::validateTmpFile); + assertThat(e.getMessage(), startsWith("Configured temporary file directory [")); + assertThat(e.getMessage(), endsWith(".test] is not a directory")); + } } From 1bd060424d7ae354d04d16a3e3690705f08f257b Mon Sep 17 00:00:00 2001 From: David Roberts Date: Wed, 14 Mar 2018 10:53:46 +0000 Subject: [PATCH 2/2] Move tmpdir check to Elasticsearch class --- .../main/java/org/elasticsearch/bootstrap/Bootstrap.java | 6 ------ .../java/org/elasticsearch/bootstrap/Elasticsearch.java | 7 +++++++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java index 117171a61876b..79e5e87251a90 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java @@ -292,12 +292,6 @@ static void init( } catch (IOException e) { throw new BootstrapException(e); } - // a misconfigured java.io.tmpdir can cause hard-to-diagnose problems later, so reject it immediately - try { - environment.validateTmpFile(); - } catch (IOException e) { - throw new BootstrapException(e); - } if (environment.pidFile() != null) { try { PidFile.create(environment.pidFile(), true); diff --git a/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java b/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java index 1538f0cdf0003..a0646288b1ad0 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java @@ -108,6 +108,13 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th final Path pidFile = pidfileOption.value(options); final boolean quiet = options.has(quietOption); + // a misconfigured java.io.tmpdir can cause hard-to-diagnose problems later, so reject it immediately + try { + env.validateTmpFile(); + } catch (IOException e) { + throw new UserException(ExitCodes.CONFIG, e.getMessage()); + } + try { init(daemonize, pidFile, quiet, env); } catch (NodeValidationException e) {