Improve Java version comparison in JarHell

This commit improves Java version comparison in JarHell.

The first improvement is the addition of a method to check the version
format of a target version string. This method will reject target
version strings that are not a sequence of nonnegative decimal integers
separated by “.”s, possibly with leading zeros (0*[0-9]+(\.[0-9]+)?).
This version format is the version format used for Java specification
versioning (cf. Java Product Versioning, 1.5.1 Specification Versioning
and the Javadocs for java.lang.Package.)

The second improvement is a clean method for checking that a target
version is compatible with the runtime version of the JVM. This is done
using the system property java.specification.version and comparing the
versions lexicograpically. This method of comparison has been tested on
JDK 9 builds that include JEP-220 (the Project Jigsaw JEP concerning
modular runtime images) and JEP-223 (the version string JEP). The class
that encapsulates the methods for parsing and comparing versions is
written in a way that can easily be converted to use the Version class
from JEP-223 if that class is ultimately incorporated into mainline JDK
9.

Closes #12441

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

@@ -25,6 +25,7 @@ import org.elasticsearch.common.io.PathUtils;
 import org.elasticsearch.common.logging.ESLogger;
 import org.elasticsearch.common.logging.Loggers;
 
+import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.net.URL;
 import java.net.URLClassLoader;
@@ -33,12 +34,7 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.SimpleFileVisitor;
 import java.nio.file.attribute.BasicFileAttributes;
-import java.util.Arrays;
+import java.util.*;
-import java.util.Enumeration;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
-import java.util.Set;
 import java.util.jar.JarEntry;
 import java.util.jar.JarFile;
 import java.util.jar.Manifest;
@@ -69,7 +65,7 @@ public class JarHell {
             logger.debug("sun.boot.class.path: {}", System.getProperty("sun.boot.class.path"));
             logger.debug("classloader urls: {}", Arrays.toString(((URLClassLoader)loader).getURLs()));
         }
-        checkJarHell(((URLClassLoader)loader).getURLs());
+        checkJarHell(((URLClassLoader) loader).getURLs());
     }
 
     /**
@@ -141,6 +137,7 @@ public class JarHell {
         // give a nice error if jar requires a newer java version
         String targetVersion = manifest.getMainAttributes().getValue("X-Compile-Target-JDK");
         if (targetVersion != null) {
+            checkVersionFormat(targetVersion);
             checkJavaVersion(jar.toString(), targetVersion);
         }
 
@@ -153,23 +150,34 @@ public class JarHell {
         }
     }
 
+    public static void checkVersionFormat(String targetVersion) {
+        if (!JavaVersion.isValid(targetVersion)) {
+            throw new IllegalStateException(
+                    String.format(
+                            Locale.ROOT,
+                            "version string must be a sequence of nonnegative decimal integers separated by \".\"'s and may have leading zeros but was %s",
+                            targetVersion
+                    )
+            );
+        }
+    }
+
     /**
      * Checks that the java specification version {@code targetVersion}
      * required by {@code resource} is compatible with the current installation.
      */
     public static void checkJavaVersion(String resource, String targetVersion) {
-        String systemVersion = System.getProperty("java.specification.version");
+        JavaVersion version = JavaVersion.parse(targetVersion);
-        float current = Float.POSITIVE_INFINITY;
+        if (JavaVersion.current().compareTo(version) < 0) {
-        float target = Float.NEGATIVE_INFINITY;
+            throw new IllegalStateException(
-        try {
+                    String.format(
-            current = Float.parseFloat(systemVersion);
+                            Locale.ROOT,
-            target = Float.parseFloat(targetVersion);
+                            "%s requires Java %s:, your system: %s",
-        } catch (NumberFormatException e) {
+                            resource,
-            // some spec changed, time for a more complex parser
+                            targetVersion,
-        }
+                            JavaVersion.current().toString()
-        if (current < target) {
+                    )
-            throw new IllegalStateException(resource + " requires Java " + targetVersion
+            );
-                    + ", your system: " + systemVersion);
         }
     }
 

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

@@ -0,0 +1,87 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.bootstrap;
+
+import org.elasticsearch.common.Strings;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+class JavaVersion implements Comparable<JavaVersion> {
+    private final List<Integer> version;
+
+    public List<Integer> getVersion() {
+        return Collections.unmodifiableList(version);
+    }
+
+    private JavaVersion(List<Integer> version) {
+        this.version = version;
+    }
+
+    public static JavaVersion parse(String value) {
+        if (value == null) {
+            throw new NullPointerException("value");
+        }
+        if ("".equals(value)) {
+            throw new IllegalArgumentException("value");
+        }
+
+        List<Integer> version = new ArrayList<>();
+        String[] components = value.split("\\.");
+        for (String component : components) {
+            version.add(Integer.valueOf(component));
+        }
+
+        return new JavaVersion(version);
+    }
+
+    public static boolean isValid(String value) {
+        if (!value.matches("^0*[0-9]+(\\.[0-9]+)*$")) {
+            return false;
+        }
+        return true;
+    }
+
+    private final static JavaVersion CURRENT = parse(System.getProperty("java.specification.version"));
+
+    public static JavaVersion current() {
+        return CURRENT;
+    }
+
+    @Override
+    public int compareTo(JavaVersion o) {
+        int len = Math.max(version.size(), o.version.size());
+        for (int i = 0; i < len; i++) {
+            int d = (i < version.size() ? version.get(i) : 0);
+            int s = (i < o.version.size() ? o.version.get(i) : 0);
+            if (s < d)
+                return 1;
+            if (s > d)
+                return -1;
+        }
+        return 0;
+    }
+
+    @Override
+    public String toString() {
+        return Strings.collectionToDelimitedString(version, ".");
+    }
+}

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

@@ -20,6 +20,7 @@
 package org.elasticsearch.bootstrap;
 
 import org.elasticsearch.Version;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.test.ESTestCase;
 
 import java.io.IOException;
@@ -27,6 +28,8 @@ import java.net.URL;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.jar.Attributes;
 import java.util.jar.JarOutputStream;
 import java.util.jar.Manifest;
@@ -153,22 +156,25 @@ public class JarHellTests extends ESTestCase {
 
     public void testRequiredJDKVersionTooOld() throws Exception {
         Path dir = createTempDir();
-        String previousJavaVersion = System.getProperty("java.specification.version");
+        List<Integer> current = JavaVersion.current().getVersion();
-        System.setProperty("java.specification.version", "1.7");
+        List<Integer> target = new ArrayList<>(current.size());
+        for (int i = 0; i < current.size(); i++) {
+            target.add(current.get(i) + 1);
+        }
+        JavaVersion targetVersion = JavaVersion.parse(Strings.collectionToDelimitedString(target, "."));
+
 
         Manifest manifest = new Manifest();
         Attributes attributes = manifest.getMainAttributes();
         attributes.put(Attributes.Name.MANIFEST_VERSION, "1.0.0");
-        attributes.put(new Attributes.Name("X-Compile-Target-JDK"), "1.8");
+        attributes.put(new Attributes.Name("X-Compile-Target-JDK"), targetVersion.toString());
         URL[] jars = {makeJar(dir, "foo.jar", manifest, "Foo.class")};
         try {
             JarHell.checkJarHell(jars);
             fail("did not get expected exception");
         } catch (IllegalStateException e) {
-            assertTrue(e.getMessage().contains("requires Java 1.8"));
+            assertTrue(e.getMessage().contains("requires Java " + targetVersion.toString()));
-            assertTrue(e.getMessage().contains("your system: 1.7"));
+            assertTrue(e.getMessage().contains("your system: " + JavaVersion.current().toString()));
-        } finally {
-            System.setProperty("java.specification.version", previousJavaVersion);
         }
     }
 
@@ -213,7 +219,12 @@ public class JarHellTests extends ESTestCase {
         attributes.put(Attributes.Name.MANIFEST_VERSION, "1.0.0");
         attributes.put(new Attributes.Name("X-Compile-Target-JDK"), "bogus");
         URL[] jars = {makeJar(dir, "foo.jar", manifest, "Foo.class")};
-        JarHell.checkJarHell(jars);
+        try {
+            JarHell.checkJarHell(jars);
+            fail("did not get expected exception");
+        } catch (IllegalStateException e) {
+            assertTrue(e.getMessage().equals("version string must be a sequence of nonnegative decimal integers separated by \".\"'s and may have leading zeros but was bogus"));
+        }
     }
 
     /** make sure if a plugin is compiled against the same ES version, it works */
@@ -242,4 +253,26 @@ public class JarHellTests extends ESTestCase {
             assertTrue(e.getMessage().contains("requires Elasticsearch 1.0-bogus"));
         }
     }
+
+    public void testValidVersions() {
+        String[] versions = new String[]{"1.7", "1.7.0", "0.1.7", "1.7.0.80"};
+        for (String version : versions) {
+            try {
+                JarHell.checkVersionFormat(version);
+            } catch (IllegalStateException e) {
+                fail(version + " should be accepted as a valid version format");
+            }
+        }
+    }
+
+    public void testInvalidVersions() {
+        String[] versions = new String[]{"", "1.7.0_80", "1.7."};
+        for (String version : versions) {
+            try {
+                JarHell.checkVersionFormat(version);
+                fail("\"" + version + "\"" + " should be rejected as an invalid version format");
+            } catch (IllegalStateException e) {
+            }
+        }
+    }
 }

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

@@ -0,0 +1,79 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.bootstrap;
+
+import org.elasticsearch.test.ESTestCase;
+import org.junit.Test;
+
+import java.util.List;
+
+import static org.hamcrest.CoreMatchers.is;
+
+public class JavaVersionTests extends ESTestCase {
+    @Test
+    public void testParse() {
+        JavaVersion javaVersion = JavaVersion.parse("1.7.0");
+        List<Integer> version = javaVersion.getVersion();
+        assertThat(3, is(version.size()));
+        assertThat(1, is(version.get(0)));
+        assertThat(7, is(version.get(1)));
+        assertThat(0, is(version.get(2)));
+    }
+
+    @Test
+    public void testToString() {
+        JavaVersion javaVersion = JavaVersion.parse("1.7.0");
+        assertThat("1.7.0", is(javaVersion.toString()));
+    }
+
+    @Test
+    public void testCompare() {
+        JavaVersion onePointSix = JavaVersion.parse("1.6");
+        JavaVersion onePointSeven = JavaVersion.parse("1.7");
+        JavaVersion onePointSevenPointZero = JavaVersion.parse("1.7.0");
+        JavaVersion onePointSevenPointOne = JavaVersion.parse("1.7.1");
+        JavaVersion onePointSevenPointTwo = JavaVersion.parse("1.7.2");
+        JavaVersion onePointSevenPointOnePointOne = JavaVersion.parse("1.7.1.1");
+        JavaVersion onePointSevenPointTwoPointOne = JavaVersion.parse("1.7.2.1");
+
+        assertTrue(onePointSix.compareTo(onePointSeven) < 0);
+        assertTrue(onePointSeven.compareTo(onePointSix) > 0);
+        assertTrue(onePointSix.compareTo(onePointSix) == 0);
+        assertTrue(onePointSeven.compareTo(onePointSevenPointZero) == 0);
+        assertTrue(onePointSevenPointOnePointOne.compareTo(onePointSevenPointOne) > 0);
+        assertTrue(onePointSevenPointTwo.compareTo(onePointSevenPointTwoPointOne) < 0);
+    }
+
+    @Test
+    public void testValidVersions() {
+        String[] versions = new String[]{"1.7", "1.7.0", "0.1.7", "1.7.0.80"};
+        for (String version : versions) {
+            assertTrue(JavaVersion.isValid(version));
+        }
+    }
+
+    @Test
+    public void testInvalidVersions() {
+        String[] versions = new String[]{"", "1.7.0_80", "1.7."};
+        for (String version : versions) {
+            assertFalse(JavaVersion.isValid(version));
+        }
+    }
+}