From 8c4bc7d10b798ed4b1a5554f5308d4a4b9068190 Mon Sep 17 00:00:00 2001 From: Robert Muir Date: Wed, 30 Sep 2015 18:58:25 -0400 Subject: [PATCH] Nuke ES_CLASSPATH appending, JarHell fail on empty classpath elements Closes #13880 Squashed commit of the following: commit 316a328e5032e580ba840db993d907631334aac0 Author: Robert Muir Date: Wed Sep 30 16:57:47 2015 -0400 windows is terrible commit 0406b560c58bf833f8d77af9c7cf3386771dd9c5 Author: Robert Muir Date: Wed Sep 30 16:43:09 2015 -0400 Nuke ES_CLASSPATH appending Out of box, ES expects its stuff to be in particular places. We should not be appending to ES_CLASSPATH, allowing users to specify stuff there, like we do in elasticsearch.bin.sh If the user sets it, its not going to work out of box. Closes #13812 commit 415d8972df28eddec322bb6d70100a1993fa95f6 Author: Robert Muir Date: Wed Sep 30 16:26:35 2015 -0400 Fail hard on empty classpath elements. This can happen easily, if somehow old 1.x shellscripts survive and try to launch 2.x code. I have the feeling this happens maybe because of packaging upgrades or something. Either way: we can just fail hard and clear in this situation, rather than the current situation where CWD might be /, and we might traverse the entire filesystem until we hit an error... Relates to #13864 --- .../org/elasticsearch/bootstrap/JarHell.java | 28 +++++++-- .../elasticsearch/bootstrap/JarHellTests.java | 60 +++++++++++++++++++ .../main/resources/bin/elasticsearch.in.bat | 9 ++- .../main/resources/bin/elasticsearch.in.sh | 16 +++-- 4 files changed, 99 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java b/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java index a5c71e3aa67..53652f1be78 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java @@ -88,17 +88,35 @@ public class JarHell { } /** - * Parses the classpath into a set of URLs + * Parses the classpath into an array of URLs + * @return array of URLs + * @throws IllegalStateException if the classpath contains empty elements + */ + public static URL[] parseClassPath() { + return parseClassPath(System.getProperty("java.class.path")); + } + + /** + * Parses the classpath into a set of URLs. For testing. + * @param classPath classpath to parse (typically the system property {@code java.class.path}) + * @return array of URLs + * @throws IllegalStateException if the classpath contains empty elements */ @SuppressForbidden(reason = "resolves against CWD because that is how classpaths work") - public static URL[] parseClassPath() { - String elements[] = System.getProperty("java.class.path").split(System.getProperty("path.separator")); + static URL[] parseClassPath(String classPath) { + String elements[] = classPath.split(System.getProperty("path.separator")); URL urlElements[] = new URL[elements.length]; for (int i = 0; i < elements.length; i++) { String element = elements[i]; - // empty classpath element behaves like CWD. + // Technically empty classpath element behaves like CWD. + // So below is the "correct" code, however in practice with ES, this is usually just a misconfiguration, + // from old shell scripts left behind or something: + // if (element.isEmpty()) { + // element = System.getProperty("user.dir"); + // } + // Instead we just throw an exception, and keep it clean. if (element.isEmpty()) { - element = System.getProperty("user.dir"); + throw new IllegalStateException("Classpath should not contain empty elements! (outdated shell script from a previous version?) classpath='" + classPath + "'"); } try { urlElements[i] = PathUtils.get(element).toUri().toURL(); diff --git a/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java b/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java index 2d825674dbe..8005b14ebe7 100644 --- a/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java +++ b/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java @@ -275,4 +275,64 @@ public class JarHellTests extends ESTestCase { } } } + + // classpath testing is system specific, so we just write separate tests for *nix and windows cases + + /** + * Parse a simple classpath with two elements on unix + */ + public void testParseClassPathUnix() throws Exception { + assumeTrue("test is designed for unix-like systems only", ":".equals(System.getProperty("path.separator"))); + assumeTrue("test is designed for unix-like systems only", "/".equals(System.getProperty("file.separator"))); + + Path element1 = createTempDir(); + Path element2 = createTempDir(); + + URL expected[] = { element1.toUri().toURL(), element2.toUri().toURL() }; + assertArrayEquals(expected, JarHell.parseClassPath(element1.toString() + ":" + element2.toString())); + } + + /** + * Make sure an old unix classpath with an empty element (implicitly CWD: i'm looking at you 1.x ES scripts) fails + */ + public void testEmptyClassPathUnix() throws Exception { + assumeTrue("test is designed for unix-like systems only", ":".equals(System.getProperty("path.separator"))); + assumeTrue("test is designed for unix-like systems only", "/".equals(System.getProperty("file.separator"))); + + try { + JarHell.parseClassPath(":/element1:/element2"); + fail("should have hit exception"); + } catch (IllegalStateException expected) { + assertTrue(expected.getMessage().contains("should not contain empty elements")); + } + } + + /** + * Parse a simple classpath with two elements on windows + */ + public void testParseClassPathWindows() throws Exception { + assumeTrue("test is designed for windows-like systems only", ";".equals(System.getProperty("path.separator"))); + assumeTrue("test is designed for windows-like systems only", "\\".equals(System.getProperty("file.separator"))); + + Path element1 = createTempDir(); + Path element2 = createTempDir(); + + URL expected[] = { element1.toUri().toURL(), element2.toUri().toURL() }; + assertArrayEquals(expected, JarHell.parseClassPath(element1.toString() + ";" + element2.toString())); + } + + /** + * Make sure an old windows classpath with an empty element (implicitly CWD: i'm looking at you 1.x ES scripts) fails + */ + public void testEmptyClassPathWindows() throws Exception { + assumeTrue("test is designed for windows-like systems only", ";".equals(System.getProperty("path.separator"))); + assumeTrue("test is designed for windows-like systems only", "\\".equals(System.getProperty("file.separator"))); + + try { + JarHell.parseClassPath(";c:\\element1;c:\\element2"); + fail("should have hit exception"); + } catch (IllegalStateException expected) { + assertTrue(expected.getMessage().contains("should not contain empty elements")); + } + } } diff --git a/distribution/src/main/resources/bin/elasticsearch.in.bat b/distribution/src/main/resources/bin/elasticsearch.in.bat index 47cf727c7b4..4e88d22f1dd 100644 --- a/distribution/src/main/resources/bin/elasticsearch.in.bat +++ b/distribution/src/main/resources/bin/elasticsearch.in.bat @@ -91,10 +91,13 @@ set JAVA_OPTS=%JAVA_OPTS% -Dfile.encoding=UTF-8 REM Use our provided JNA always versus the system one set JAVA_OPTS=%JAVA_OPTS% -Djna.nosys=true -set CORE_CLASSPATH=%ES_HOME%/lib/${project.build.finalName}.jar;%ES_HOME%/lib/* +REM check in case a user was using this mechanism if "%ES_CLASSPATH%" == "" ( -set ES_CLASSPATH=%CORE_CLASSPATH% +set ES_CLASSPATH=%ES_HOME%/lib/${project.build.finalName}.jar;%ES_HOME%/lib/* ) else ( -set ES_CLASSPATH=%ES_CLASSPATH%;%CORE_CLASSPATH% +ECHO Error: Don't modify the classpath with ES_CLASSPATH, Best is to add 1>&2 +ECHO additional elements via the plugin mechanism, or if code must really be 1>&2 +ECHO added to the main classpath, add jars to lib\, unsupported 1>&2 +EXIT /B 1 ) set ES_PARAMS=-Delasticsearch -Des-foreground=yes -Des.path.home="%ES_HOME%" diff --git a/distribution/src/main/resources/bin/elasticsearch.in.sh b/distribution/src/main/resources/bin/elasticsearch.in.sh index 2661544cb52..5ac0025fe29 100644 --- a/distribution/src/main/resources/bin/elasticsearch.in.sh +++ b/distribution/src/main/resources/bin/elasticsearch.in.sh @@ -1,13 +1,17 @@ #!/bin/sh -CORE_CLASSPATH="$ES_HOME/lib/${project.build.finalName}.jar:$ES_HOME/lib/*" - -if [ "x$ES_CLASSPATH" = "x" ]; then - ES_CLASSPATH="$CORE_CLASSPATH" -else - ES_CLASSPATH="$ES_CLASSPATH:$CORE_CLASSPATH" +# check in case a user was using this mechanism +if [ "x$ES_CLASSPATH" != "x" ]; then + cat >&2 << EOF +Error: Don't modify the classpath with ES_CLASSPATH. Best is to add +additional elements via the plugin mechanism, or if code must really be +added to the main classpath, add jars to lib/ (unsupported). +EOF + exit 1 fi +ES_CLASSPATH="$ES_HOME/lib/${project.build.finalName}.jar:$ES_HOME/lib/*" + if [ "x$ES_MIN_MEM" = "x" ]; then ES_MIN_MEM=${packaging.elasticsearch.heap.min} fi