Nuke ES_CLASSPATH appending, JarHell fail on empty classpath elements

Closes #13880

Squashed commit of the following:

commit 316a328
Author: Robert Muir <rmuir@apache.org>
Date:   Wed Sep 30 16:57:47 2015 -0400

    windows is terrible

commit 0406b56
Author: Robert Muir <rmuir@apache.org>
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 415d897
Author: Robert Muir <rmuir@apache.org>
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

core/src/main/java/org/elasticsearch/bootstrap/JarHell.java

@@ -88,17 +88,35 @@ public static void checkJarHell() throws Exception {
     }
 
     /**
-     * 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
      */
-    @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"));
+        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")
+    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();

core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java

@@ -275,4 +275,64 @@ public void testInvalidVersions() {
             }
         }
     }
+
+    // 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/*"
-
-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