Merge pull request #13010 from jasontedor/feature/improve-java-version-comparison

Improve java version comparison and explicitly enforce a version format

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.Enumeration;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;
 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");
-        float current = Float.POSITIVE_INFINITY;
-        float target = Float.NEGATIVE_INFINITY;
-        try {
-            current = Float.parseFloat(systemVersion);
-            target = Float.parseFloat(targetVersion);
-        } catch (NumberFormatException e) {
-            // some spec changed, time for a more complex parser
-        }
-        if (current < target) {
-            throw new IllegalStateException(resource + " requires Java " + targetVersion
-                    + ", your system: " + systemVersion);
+        JavaVersion version = JavaVersion.parse(targetVersion);
+        if (JavaVersion.current().compareTo(version) < 0) {
+            throw new IllegalStateException(
+                    String.format(
+                            Locale.ROOT,
+                            "%s requires Java %s:, your system: %s",
+                            resource,
+                            targetVersion,
+                            JavaVersion.current().toString()
+                    )
+            );
         }
     }
 

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/main/java/org/elasticsearch/plugins/PluginInfo.java

@@ -120,6 +120,7 @@ public class PluginInfo implements Streamable, ToXContent {
             if (javaVersionString == null) {
                 throw new IllegalArgumentException("Property [java.version] is missing for jvm plugin [" + name + "]");
             }
+            JarHell.checkVersionFormat(javaVersionString);
             JarHell.checkJavaVersion(name, javaVersionString);
             isolated = Boolean.parseBoolean(props.getProperty("isolated", "true"));
             classname = props.getProperty("classname");

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");
-        System.setProperty("java.specification.version", "1.7");
+        List<Integer> current = JavaVersion.current().getVersion();
+        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("your system: 1.7"));
-        } finally {
-            System.setProperty("java.specification.version", previousJavaVersion);
+            assertTrue(e.getMessage().contains("requires Java " + targetVersion.toString()));
+            assertTrue(e.getMessage().contains("your system: " + JavaVersion.current().toString()));
         }
     }
 
@@ -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));
+        }
+    }
+}

core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java

@@ -176,6 +176,25 @@ public class PluginInfoTests extends ESTestCase {
         }
     }
 
+    public void testReadFromPropertiesBadJavaVersionFormat() throws Exception {
+        String pluginName = "fake-plugin";
+        Path pluginDir = createTempDir().resolve(pluginName);
+        writeProperties(pluginDir,
+                "description", "fake desc",
+                "name", pluginName,
+                "elasticsearch.version", Version.CURRENT.toString(),
+                "java.version", "1.7.0_80",
+                "classname", "FakePlugin",
+                "version", "1.0",
+                "jvm", "true");
+        try {
+            PluginInfo.readFromProperties(pluginDir);
+            fail("expected bad java version format exception");
+        } catch (IllegalStateException e) {
+            assertTrue(e.getMessage(), e.getMessage().equals("version string must be a sequence of nonnegative decimal integers separated by \".\"'s and may have leading zeros but was 1.7.0_80"));
+        }
+    }
+
     public void testReadFromPropertiesBogusElasticsearchVersion() throws Exception {
         Path pluginDir = createTempDir().resolve("fake-plugin");
         writeProperties(pluginDir,

dev-tools/src/main/resources/plugin-metadata/plugin-descriptor.properties

@@ -58,6 +58,9 @@ jvm=${elasticsearch.plugin.jvm}
 classname=${elasticsearch.plugin.classname}
 #
 # 'java.version' version of java the code is built against
+# use the system property java.specification.version
+# version string must be a sequence of nonnegative decimal integers
+# separated by "."'s and may have leading zeros
 java.version=${maven.compiler.target}
 #
 # 'elasticsearch.version' version of elasticsearch compiled against