Merge remote-tracking branch 'origin/master'

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()  {
+    static URL[] parseClassPath(String classPath) {
-        String elements[] = System.getProperty("java.class.path").split(System.getProperty("path.separator"));
+        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();

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"));
+        }
+    }
 }

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%"

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/*"
+# check in case a user was using this mechanism
-
+if [ "x$ES_CLASSPATH" != "x" ]; then
-if [ "x$ES_CLASSPATH" = "x" ]; then
+    cat >&2 << EOF
-    ES_CLASSPATH="$CORE_CLASSPATH"
+Error: Don't modify the classpath with ES_CLASSPATH. Best is to add
-else
+additional elements via the plugin mechanism, or if code must really be
-    ES_CLASSPATH="$ES_CLASSPATH:$CORE_CLASSPATH"
+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