From 3b7546bdb1ed9d3448b4eeb5ccb4d28aa2758c9f Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 15 Mar 2017 12:53:30 -0700 Subject: [PATCH 1/2] Packaging: Remove classpath ordering hack After the removal of the joda time hack we used to have, we can cleanup the codebase handling in security, jarhell and plugins to be more picky about uniqueness. This was originally in #18959 which was never merged. closes #18959 --- .../org/elasticsearch/bootstrap/ESPolicy.java | 3 +- .../org/elasticsearch/bootstrap/JarHell.java | 27 ++-- .../org/elasticsearch/bootstrap/Security.java | 44 +++++-- .../plugins/InstallPluginCommand.java | 9 +- .../elasticsearch/plugins/PluginsService.java | 123 ++++++++++++------ .../elasticsearch/bootstrap/JarHellTests.java | 51 ++++---- .../main/resources/bin/elasticsearch.in.bat | 2 +- .../main/resources/bin/elasticsearch.in.sh | 2 +- .../ingest/attachment/TikaImpl.java | 11 +- .../bootstrap/BootstrapForTesting.java | 16 ++- 10 files changed, 179 insertions(+), 109 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java b/core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java index e8538daec56b0..74fa7e0c1d5ac 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java @@ -31,6 +31,7 @@ import java.security.Permissions; import java.security.Policy; import java.security.ProtectionDomain; +import java.util.Collections; import java.util.Map; import java.util.function.Predicate; @@ -50,7 +51,7 @@ final class ESPolicy extends Policy { ESPolicy(PermissionCollection dynamic, Map plugins, boolean filterBadDefaults) { this.template = Security.readPolicy(getClass().getResource(POLICY_RESOURCE), JarHell.parseClassPath()); - this.untrusted = Security.readPolicy(getClass().getResource(UNTRUSTED_RESOURCE), new URL[0]); + this.untrusted = Security.readPolicy(getClass().getResource(UNTRUSTED_RESOURCE), Collections.emptySet()); if (filterBadDefaults) { this.system = new SystemPolicy(Policy.getPolicy()); } else { diff --git a/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java b/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java index 22ba936d9030f..cdf6e516e33a8 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java @@ -36,9 +36,11 @@ import java.nio.file.SimpleFileVisitor; import java.nio.file.attribute.BasicFileAttributes; import java.util.Arrays; +import java.util.Collections; import java.util.Enumeration; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.Locale; import java.util.Map; import java.util.Set; @@ -93,7 +95,7 @@ public static void checkJarHell() throws IOException, URISyntaxException { * @return array of URLs * @throws IllegalStateException if the classpath contains empty elements */ - public static URL[] parseClassPath() { + public static Set parseClassPath() { return parseClassPath(System.getProperty("java.class.path")); } @@ -104,13 +106,12 @@ public static URL[] parseClassPath() { * @throws IllegalStateException if the classpath contains empty elements */ @SuppressForbidden(reason = "resolves against CWD because that is how classpaths work") - static URL[] parseClassPath(String classPath) { + static Set parseClassPath(String classPath) { String pathSeparator = System.getProperty("path.separator"); String fileSeparator = System.getProperty("file.separator"); String elements[] = classPath.split(pathSeparator); - URL urlElements[] = new URL[elements.length]; - for (int i = 0; i < elements.length; i++) { - String element = elements[i]; + Set urlElements = new LinkedHashSet<>(); // order is already lost, but some filesystems have it + for (String element : elements) { // 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: @@ -136,13 +137,17 @@ static URL[] parseClassPath(String classPath) { } // now just parse as ordinary file try { - urlElements[i] = PathUtils.get(element).toUri().toURL(); + URL url = PathUtils.get(element).toUri().toURL(); + if (urlElements.add(url) == false) { + throw new IllegalStateException("jar hell!" + System.lineSeparator() + + "duplicate jar on classpath: " + classPath); + } } catch (MalformedURLException e) { // should not happen, as we use the filesystem API throw new RuntimeException(e); } } - return urlElements; + return Collections.unmodifiableSet(urlElements); } /** @@ -150,7 +155,7 @@ static URL[] parseClassPath(String classPath) { * @throws IllegalStateException if jar hell was found */ @SuppressForbidden(reason = "needs JarFile for speed, just reading entries") - public static void checkJarHell(URL urls[]) throws URISyntaxException, IOException { + public static void checkJarHell(Set urls) throws URISyntaxException, IOException { Logger logger = Loggers.getLogger(JarHell.class); // we don't try to be sneaky and use deprecated/internal/not portable stuff // like sun.boot.class.path, and with jigsaw we don't yet have a way to get @@ -168,8 +173,8 @@ public static void checkJarHell(URL urls[]) throws URISyntaxException, IOExcepti } if (path.toString().endsWith(".jar")) { if (!seenJars.add(path)) { - logger.debug("excluding duplicate classpath element: {}", path); - continue; + throw new IllegalStateException("jar hell!" + System.lineSeparator() + + "duplicate jar on classpath: " + path); } logger.debug("examining jar: {}", path); try (JarFile file = new JarFile(path.toString())) { @@ -198,7 +203,7 @@ public static void checkJarHell(URL urls[]) throws URISyntaxException, IOExcepti public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { String entry = root.relativize(file).toString(); if (entry.endsWith(".class")) { - // normalize with the os separator + // normalize with the os separator, remove '.class' entry = entry.replace(sep, ".").substring(0, entry.length() - 6); checkClass(clazzes, entry, path); } diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Security.java b/core/src/main/java/org/elasticsearch/bootstrap/Security.java index 3b59f235b1cf4..de16bbe76aa42 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Security.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Security.java @@ -48,8 +48,10 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Set; /** * Initializes SecurityManager with necessary permissions. @@ -127,19 +129,23 @@ static void configure(Environment environment, boolean filterBadDefaults) throws @SuppressForbidden(reason = "proper use of URL") static Map getPluginPermissions(Environment environment) throws IOException, NoSuchAlgorithmException { Map map = new HashMap<>(); - // collect up lists of plugins and modules - List pluginsAndModules = new ArrayList<>(); + // collect up set of plugins and modules by listing directories. + Set pluginsAndModules = new LinkedHashSet<>(); // order is already lost, but some filesystems have it if (Files.exists(environment.pluginsFile())) { try (DirectoryStream stream = Files.newDirectoryStream(environment.pluginsFile())) { for (Path plugin : stream) { - pluginsAndModules.add(plugin); + if (pluginsAndModules.add(plugin) == false) { + throw new IllegalStateException("duplicate plugin: " + plugin); + } } } } if (Files.exists(environment.modulesFile())) { try (DirectoryStream stream = Files.newDirectoryStream(environment.modulesFile())) { - for (Path plugin : stream) { - pluginsAndModules.add(plugin); + for (Path module : stream) { + if (pluginsAndModules.add(module) == false) { + throw new IllegalStateException("duplicate module: " + module); + } } } } @@ -149,15 +155,18 @@ static Map getPluginPermissions(Environment environment) throws I if (Files.exists(policyFile)) { // first get a list of URLs for the plugins' jars: // we resolve symlinks so map is keyed on the normalize codebase name - List codebases = new ArrayList<>(); + Set codebases = new LinkedHashSet<>(); // order is already lost, but some filesystems have it try (DirectoryStream jarStream = Files.newDirectoryStream(plugin, "*.jar")) { for (Path jar : jarStream) { - codebases.add(jar.toRealPath().toUri().toURL()); + URL url = jar.toRealPath().toUri().toURL(); + if (codebases.add(url) == false) { + throw new IllegalStateException("duplicate module/plugin: " + url); + } } } // parse the plugin's policy file into a set of permissions - Policy policy = readPolicy(policyFile.toUri().toURL(), codebases.toArray(new URL[codebases.size()])); + Policy policy = readPolicy(policyFile.toUri().toURL(), codebases); // consult this policy for each of the plugin's jars: for (URL url : codebases) { @@ -175,24 +184,33 @@ static Map getPluginPermissions(Environment environment) throws I /** * Reads and returns the specified {@code policyFile}. *

- * Resources (e.g. jar files and directories) listed in {@code codebases} location - * will be provided to the policy file via a system property of the short name: - * e.g. ${codebase.joda-convert-1.2.jar} would map to full URL. + * Jar files listed in {@code codebases} location will be provided to the policy file via + * a system property of the short name: e.g. ${codebase.joda-convert-1.2.jar} + * would map to full URL. */ @SuppressForbidden(reason = "accesses fully qualified URLs to configure security") - static Policy readPolicy(URL policyFile, URL codebases[]) { + static Policy readPolicy(URL policyFile, Set codebases) { try { try { // set codebase properties for (URL url : codebases) { String shortName = PathUtils.get(url.toURI()).getFileName().toString(); - System.setProperty("codebase." + shortName, url.toString()); + if (shortName.endsWith(".jar") == false) { + continue; // tests :( + } + String previous = System.setProperty("codebase." + shortName, url.toString()); + if (previous != null) { + throw new IllegalStateException("codebase property already set: " + shortName + "->" + previous); + } } return Policy.getInstance("JavaPolicy", new URIParameter(policyFile.toURI())); } finally { // clear codebase properties for (URL url : codebases) { String shortName = PathUtils.get(url.toURI()).getFileName().toString(); + if (shortName.endsWith(".jar") == false) { + continue; // tests :( + } System.clearProperty("codebase." + shortName); } } diff --git a/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java b/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java index be78dd927ff32..a5905e191689a 100644 --- a/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java +++ b/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java @@ -455,8 +455,7 @@ private PluginInfo verify(Terminal terminal, Path pluginRoot, boolean isBatch, E /** check a candidate plugin for jar hell before installing it */ void jarHellCheck(Path candidate, Path pluginsDir) throws Exception { // create list of current jars in classpath - final List jars = new ArrayList<>(); - jars.addAll(Arrays.asList(JarHell.parseClassPath())); + final Set jars = new HashSet<>(JarHell.parseClassPath()); // read existing bundles. this does some checks on the installation too. PluginsService.getPluginBundles(pluginsDir); @@ -464,13 +463,15 @@ void jarHellCheck(Path candidate, Path pluginsDir) throws Exception { // add plugin jars to the list Path pluginJars[] = FileSystemUtils.files(candidate, "*.jar"); for (Path jar : pluginJars) { - jars.add(jar.toUri().toURL()); + if (jars.add(jar.toUri().toURL()) == false) { + throw new IllegalStateException("jar hell! duplicate plugin jar: " + jar); + } } // TODO: no jars should be an error // TODO: verify the classname exists in one of the jars! // check combined (current classpath + new jars to-be-added) - JarHell.checkJarHell(jars.toArray(new URL[jars.size()])); + JarHell.checkJarHell(jars); } /** diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java index 7d9887370588e..9295c6c38d872 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -58,8 +58,10 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; @@ -107,16 +109,16 @@ public PluginsService(Settings settings, Path modulesDirectory, Path pluginsDire pluginsList.add(pluginInfo); } + Set seenBundles = new LinkedHashSet<>(); List modulesList = new ArrayList<>(); // load modules if (modulesDirectory != null) { try { - List bundles = getModuleBundles(modulesDirectory); - List> loaded = loadBundles(bundles); - pluginsLoaded.addAll(loaded); - for (Tuple module : loaded) { - modulesList.add(module.v1()); + Set modules = getModuleBundles(modulesDirectory); + for (Bundle bundle : modules) { + modulesList.add(bundle.plugin); } + seenBundles.addAll(modules); } catch (IOException ex) { throw new IllegalStateException("Unable to initialize modules", ex); } @@ -125,17 +127,19 @@ public PluginsService(Settings settings, Path modulesDirectory, Path pluginsDire // now, find all the ones that are in plugins/ if (pluginsDirectory != null) { try { - List bundles = getPluginBundles(pluginsDirectory); - List> loaded = loadBundles(bundles); - pluginsLoaded.addAll(loaded); - for (Tuple plugin : loaded) { - pluginsList.add(plugin.v1()); + Set plugins = getPluginBundles(pluginsDirectory); + for (Bundle bundle : plugins) { + pluginsList.add(bundle.plugin); } + seenBundles.addAll(plugins); } catch (IOException ex) { throw new IllegalStateException("Unable to initialize plugins", ex); } } + List> loaded = loadBundles(seenBundles); + pluginsLoaded.addAll(loaded); + this.info = new PluginsAndModules(pluginsList, modulesList); this.plugins = Collections.unmodifiableList(pluginsLoaded); @@ -234,48 +238,70 @@ public PluginsAndModules info() { // a "bundle" is a group of plugins in a single classloader // really should be 1-1, but we are not so fortunate static class Bundle { - List plugins = new ArrayList<>(); - List urls = new ArrayList<>(); + final PluginInfo plugin; + final Set urls; + + Bundle(PluginInfo plugin, Set urls) { + this.plugin = Objects.requireNonNull(plugin); + this.urls = Objects.requireNonNull(urls); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Bundle bundle = (Bundle) o; + return Objects.equals(plugin, bundle.plugin); + } + + @Override + public int hashCode() { + return Objects.hash(plugin); + } } // similar in impl to getPluginBundles, but DO NOT try to make them share code. // we don't need to inherit all the leniency, and things are different enough. - static List getModuleBundles(Path modulesDirectory) throws IOException { + static Set getModuleBundles(Path modulesDirectory) throws IOException { // damn leniency if (Files.notExists(modulesDirectory)) { - return Collections.emptyList(); + return Collections.emptySet(); } - List bundles = new ArrayList<>(); + Set bundles = new LinkedHashSet<>(); try (DirectoryStream stream = Files.newDirectoryStream(modulesDirectory)) { for (Path module : stream) { if (FileSystemUtils.isHidden(module)) { continue; // skip over .DS_Store etc } PluginInfo info = PluginInfo.readFromProperties(module); - Bundle bundle = new Bundle(); - bundle.plugins.add(info); + Set urls = new LinkedHashSet<>(); // gather urls for jar files try (DirectoryStream jarStream = Files.newDirectoryStream(module, "*.jar")) { for (Path jar : jarStream) { // normalize with toRealPath to get symlinks out of our hair - bundle.urls.add(jar.toRealPath().toUri().toURL()); + URL url = jar.toRealPath().toUri().toURL(); + if (urls.add(url) == false) { + throw new IllegalStateException("duplicate codebase: " + url); + } } } - bundles.add(bundle); + if (bundles.add(new Bundle(info, urls)) == false) { + throw new IllegalStateException("duplicate module: " + info); + } } } return bundles; } - static List getPluginBundles(Path pluginsDirectory) throws IOException { + static Set getPluginBundles(Path pluginsDirectory) throws IOException { Logger logger = Loggers.getLogger(PluginsService.class); // TODO: remove this leniency, but tests bogusly rely on it if (!isAccessibleDirectory(pluginsDirectory, logger)) { - return Collections.emptyList(); + return Collections.emptySet(); } - List bundles = new ArrayList<>(); + Set bundles = new LinkedHashSet<>(); try (DirectoryStream stream = Files.newDirectoryStream(pluginsDirectory)) { for (Path plugin : stream) { @@ -292,47 +318,58 @@ static List getPluginBundles(Path pluginsDirectory) throws IOException { + plugin.getFileName() + "]. Was the plugin built before 2.0?", e); } - List urls = new ArrayList<>(); + Set urls = new LinkedHashSet<>(); try (DirectoryStream jarStream = Files.newDirectoryStream(plugin, "*.jar")) { for (Path jar : jarStream) { // normalize with toRealPath to get symlinks out of our hair - urls.add(jar.toRealPath().toUri().toURL()); + URL url = jar.toRealPath().toUri().toURL(); + if (urls.add(url) == false) { + throw new IllegalStateException("duplicate codebase: " + url); + } } } - final Bundle bundle = new Bundle(); - bundles.add(bundle); - bundle.plugins.add(info); - bundle.urls.addAll(urls); + if (bundles.add(new Bundle(info, urls)) == false) { + throw new IllegalStateException("duplicate plugin: " + info); + } } } return bundles; } - private List> loadBundles(List bundles) { + private List> loadBundles(Set bundles) { List> plugins = new ArrayList<>(); for (Bundle bundle : bundles) { // jar-hell check the bundle against the parent classloader // pluginmanager does it, but we do it again, in case lusers mess with jar files manually try { - final List jars = new ArrayList<>(); - jars.addAll(Arrays.asList(JarHell.parseClassPath())); - jars.addAll(bundle.urls); - JarHell.checkJarHell(jars.toArray(new URL[0])); + Set classpath = JarHell.parseClassPath(); + // check we don't have conflicting codebases + Set intersection = new HashSet<>(classpath); + intersection.retainAll(bundle.urls); + if (intersection.isEmpty() == false) { + throw new IllegalStateException("jar hell! duplicate codebases between" + + " plugin and core: " + intersection); + } + // check we don't have conflicting classes + Set union = new HashSet<>(classpath); + union.addAll(bundle.urls); + JarHell.checkJarHell(union); } catch (Exception e) { - throw new IllegalStateException("failed to load bundle " + bundle.urls + " due to jar hell", e); + throw new IllegalStateException("failed to load plugin " + bundle.plugin + + " due to jar hell", e); } - // create a child to load the plugins in this bundle - ClassLoader loader = URLClassLoader.newInstance(bundle.urls.toArray(new URL[0]), getClass().getClassLoader()); - for (PluginInfo pluginInfo : bundle.plugins) { - // reload lucene SPI with any new services from the plugin - reloadLuceneSPI(loader); - final Class pluginClass = loadPluginClass(pluginInfo.getClassname(), loader); - final Plugin plugin = loadPlugin(pluginClass, settings); - plugins.add(new Tuple<>(pluginInfo, plugin)); - } + // create a child to load the plugin in this bundle + ClassLoader loader = URLClassLoader.newInstance(bundle.urls.toArray(new URL[0]), + getClass().getClassLoader()); + // reload lucene SPI with any new services from the plugin + reloadLuceneSPI(loader); + final Class pluginClass = + loadPluginClass(bundle.plugin.getClassname(), loader); + final Plugin plugin = loadPlugin(pluginClass, settings); + plugins.add(new Tuple<>(bundle.plugin, plugin)); } return Collections.unmodifiableList(plugins); diff --git a/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java b/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java index d38d346d6c14f..6321e97ddaedb 100644 --- a/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java +++ b/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java @@ -30,7 +30,9 @@ import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.util.ArrayList; +import java.util.Collections; import java.util.List; +import java.util.Set; import java.util.jar.Attributes; import java.util.jar.JarOutputStream; import java.util.jar.Manifest; @@ -62,7 +64,8 @@ URL makeFile(Path dir, String name) throws IOException { public void testDifferentJars() throws Exception { Path dir = createTempDir(); - URL[] jars = {makeJar(dir, "foo.jar", null, "DuplicateClass.class"), makeJar(dir, "bar.jar", null, "DuplicateClass.class")}; + Set jars = asSet(makeJar(dir, "foo.jar", null, "DuplicateClass.class"), + makeJar(dir, "bar.jar", null, "DuplicateClass.class")); try { JarHell.checkJarHell(jars); fail("did not get expected exception"); @@ -74,17 +77,11 @@ public void testDifferentJars() throws Exception { } } - public void testDuplicateClasspathLeniency() throws Exception { - Path dir = createTempDir(); - URL jar = makeJar(dir, "foo.jar", null, "Foo.class"); - URL[] jars = {jar, jar}; - JarHell.checkJarHell(jars); - } - public void testDirsOnClasspath() throws Exception { Path dir1 = createTempDir(); Path dir2 = createTempDir(); - URL[] dirs = {makeFile(dir1, "DuplicateClass.class"), makeFile(dir2, "DuplicateClass.class")}; + Set dirs = asSet(makeFile(dir1, "DuplicateClass.class"), + makeFile(dir2, "DuplicateClass.class")); try { JarHell.checkJarHell(dirs); fail("did not get expected exception"); @@ -99,7 +96,8 @@ public void testDirsOnClasspath() throws Exception { public void testDirAndJar() throws Exception { Path dir1 = createTempDir(); Path dir2 = createTempDir(); - URL[] dirs = {makeJar(dir1, "foo.jar", null, "DuplicateClass.class"), makeFile(dir2, "DuplicateClass.class")}; + Set dirs = asSet(makeJar(dir1, "foo.jar", null, "DuplicateClass.class"), + makeFile(dir2, "DuplicateClass.class")); try { JarHell.checkJarHell(dirs); fail("did not get expected exception"); @@ -114,7 +112,7 @@ public void testDirAndJar() throws Exception { public void testWithinSingleJar() throws Exception { // the java api for zip file does not allow creating duplicate entries (good!) so // this bogus jar had to be constructed with ant - URL[] jars = {JarHellTests.class.getResource("duplicate-classes.jar")}; + Set jars = Collections.singleton(JarHellTests.class.getResource("duplicate-classes.jar")); try { JarHell.checkJarHell(jars); fail("did not get expected exception"); @@ -127,7 +125,7 @@ public void testWithinSingleJar() throws Exception { } public void testXmlBeansLeniency() throws Exception { - URL[] jars = {JarHellTests.class.getResource("duplicate-xmlbeans-classes.jar")}; + Set jars = Collections.singleton(JarHellTests.class.getResource("duplicate-xmlbeans-classes.jar")); JarHell.checkJarHell(jars); } @@ -145,7 +143,7 @@ public void testRequiredJDKVersionTooOld() throws Exception { Attributes attributes = manifest.getMainAttributes(); attributes.put(Attributes.Name.MANIFEST_VERSION, "1.0.0"); attributes.put(new Attributes.Name("X-Compile-Target-JDK"), targetVersion.toString()); - URL[] jars = {makeJar(dir, "foo.jar", manifest, "Foo.class")}; + Set jars = Collections.singleton(makeJar(dir, "foo.jar", manifest, "Foo.class")); try { JarHell.checkJarHell(jars); fail("did not get expected exception"); @@ -161,7 +159,7 @@ public void testBadJDKVersionInJar() throws Exception { Attributes attributes = manifest.getMainAttributes(); 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")}; + Set jars = Collections.singleton(makeJar(dir, "foo.jar", manifest, "Foo.class")); try { JarHell.checkJarHell(jars); fail("did not get expected exception"); @@ -176,8 +174,7 @@ public void testRequiredJDKVersionIsOK() throws Exception { Attributes attributes = manifest.getMainAttributes(); attributes.put(Attributes.Name.MANIFEST_VERSION, "1.0.0"); attributes.put(new Attributes.Name("X-Compile-Target-JDK"), "1.7"); - URL[] jars = {makeJar(dir, "foo.jar", manifest, "Foo.class")}; - + Set jars = Collections.singleton(makeJar(dir, "foo.jar", manifest, "Foo.class")); JarHell.checkJarHell(jars); } @@ -188,7 +185,7 @@ public void testGoodESVersionInJar() throws Exception { Attributes attributes = manifest.getMainAttributes(); attributes.put(Attributes.Name.MANIFEST_VERSION, "1.0.0"); attributes.put(new Attributes.Name("X-Compile-Elasticsearch-Version"), Version.CURRENT.toString()); - URL[] jars = {makeJar(dir, "foo.jar", manifest, "Foo.class")}; + Set jars = Collections.singleton(makeJar(dir, "foo.jar", manifest, "Foo.class")); JarHell.checkJarHell(jars); } @@ -199,7 +196,7 @@ public void testBadESVersionInJar() throws Exception { Attributes attributes = manifest.getMainAttributes(); attributes.put(Attributes.Name.MANIFEST_VERSION, "1.0.0"); attributes.put(new Attributes.Name("X-Compile-Elasticsearch-Version"), "1.0-bogus"); - URL[] jars = {makeJar(dir, "foo.jar", manifest, "Foo.class")}; + Set jars = Collections.singleton(makeJar(dir, "foo.jar", manifest, "Foo.class")); try { JarHell.checkJarHell(jars); fail("did not get expected exception"); @@ -242,8 +239,8 @@ public void testParseClassPathUnix() throws Exception { Path element1 = createTempDir(); Path element2 = createTempDir(); - URL expected[] = { element1.toUri().toURL(), element2.toUri().toURL() }; - assertArrayEquals(expected, JarHell.parseClassPath(element1.toString() + ":" + element2.toString())); + Set expected = asSet(element1.toUri().toURL(), element2.toUri().toURL()); + assertEquals(expected, JarHell.parseClassPath(element1.toString() + ":" + element2.toString())); } /** @@ -271,8 +268,8 @@ public void testParseClassPathWindows() throws Exception { Path element1 = createTempDir(); Path element2 = createTempDir(); - URL expected[] = { element1.toUri().toURL(), element2.toUri().toURL() }; - assertArrayEquals(expected, JarHell.parseClassPath(element1.toString() + ";" + element2.toString())); + Set expected = asSet(element1.toUri().toURL(), element2.toUri().toURL()); + assertEquals(expected, JarHell.parseClassPath(element1.toString() + ";" + element2.toString())); } /** @@ -298,13 +295,13 @@ public void testCrazyEclipseClassPathWindows() 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"))); - URL expected[] = { + Set expected = asSet( PathUtils.get("c:\\element1").toUri().toURL(), PathUtils.get("c:\\element2").toUri().toURL(), PathUtils.get("c:\\element3").toUri().toURL(), - PathUtils.get("c:\\element 4").toUri().toURL(), - }; - URL actual[] = JarHell.parseClassPath("c:\\element1;c:\\element2;/c:/element3;/c:/element 4"); - assertArrayEquals(expected, actual); + PathUtils.get("c:\\element 4").toUri().toURL() + ); + Set actual = JarHell.parseClassPath("c:\\element1;c:\\element2;/c:/element3;/c:/element 4"); + assertEquals(expected, actual); } } diff --git a/distribution/src/main/resources/bin/elasticsearch.in.bat b/distribution/src/main/resources/bin/elasticsearch.in.bat index 98b6a16316c2d..a25008338729c 100644 --- a/distribution/src/main/resources/bin/elasticsearch.in.bat +++ b/distribution/src/main/resources/bin/elasticsearch.in.bat @@ -15,7 +15,7 @@ for %%I in ("%SCRIPT_DIR%..") do set ES_HOME=%%~dpfI REM check in case a user was using this mechanism if "%ES_CLASSPATH%" == "" ( -set ES_CLASSPATH=!ES_HOME!/lib/elasticsearch-${project.version}.jar;!ES_HOME!/lib/* +set ES_CLASSPATH=!ES_HOME!/lib/* ) else ( 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 diff --git a/distribution/src/main/resources/bin/elasticsearch.in.sh b/distribution/src/main/resources/bin/elasticsearch.in.sh index 58b26a2d6ebc7..2d22439225697 100644 --- a/distribution/src/main/resources/bin/elasticsearch.in.sh +++ b/distribution/src/main/resources/bin/elasticsearch.in.sh @@ -10,4 +10,4 @@ EOF exit 1 fi -ES_CLASSPATH="$ES_HOME/lib/elasticsearch-${project.version}.jar:$ES_HOME/lib/*" +ES_CLASSPATH="$ES_HOME/lib/*" diff --git a/plugins/ingest-attachment/src/main/java/org/elasticsearch/ingest/attachment/TikaImpl.java b/plugins/ingest-attachment/src/main/java/org/elasticsearch/ingest/attachment/TikaImpl.java index c7ffe4f287f0c..3c0f3b0433c0c 100644 --- a/plugins/ingest-attachment/src/main/java/org/elasticsearch/ingest/attachment/TikaImpl.java +++ b/plugins/ingest-attachment/src/main/java/org/elasticsearch/ingest/attachment/TikaImpl.java @@ -47,7 +47,9 @@ import java.security.PrivilegedExceptionAction; import java.security.ProtectionDomain; import java.security.SecurityPermission; +import java.util.Arrays; import java.util.Collections; +import java.util.LinkedHashSet; import java.util.PropertyPermission; import java.util.Set; @@ -128,7 +130,12 @@ static PermissionCollection getRestrictedPermissions() { addReadPermissions(perms, JarHell.parseClassPath()); // plugin jars if (TikaImpl.class.getClassLoader() instanceof URLClassLoader) { - addReadPermissions(perms, ((URLClassLoader)TikaImpl.class.getClassLoader()).getURLs()); + URL[] urls = ((URLClassLoader)TikaImpl.class.getClassLoader()).getURLs(); + Set set = new LinkedHashSet<>(Arrays.asList(urls)); + if (set.size() != urls.length) { + throw new AssertionError("duplicate jars: " + Arrays.toString(urls)); + } + addReadPermissions(perms, set); } // jvm's java.io.tmpdir (needs read/write) perms.add(new FilePermission(System.getProperty("java.io.tmpdir") + System.getProperty("file.separator") + "-", @@ -145,7 +152,7 @@ static PermissionCollection getRestrictedPermissions() { // add resources to (what is typically) a jar, but might not be (e.g. in tests/IDE) @SuppressForbidden(reason = "adds access to jar resources") - static void addReadPermissions(Permissions perms, URL resources[]) { + static void addReadPermissions(Permissions perms, Set resources) { try { for (URL url : resources) { Path path = PathUtils.get(url.toURI()); diff --git a/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java b/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java index fe833470ad255..4ab45d8c1a62b 100644 --- a/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java +++ b/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java @@ -182,7 +182,7 @@ static Map getPluginPermissions() throws Exception { } // compute classpath minus obvious places, all other jars will get the permission. - Set codebases = new HashSet<>(Arrays.asList(parseClassPathWithSymlinks())); + Set codebases = new HashSet<>(parseClassPathWithSymlinks()); Set excluded = new HashSet<>(Arrays.asList( // es core Bootstrap.class.getProtectionDomain().getCodeSource().getLocation(), @@ -200,7 +200,7 @@ static Map getPluginPermissions() throws Exception { // parse each policy file, with codebase substitution from the classpath final List policies = new ArrayList<>(); for (URL policyFile : pluginPolicies) { - policies.add(Security.readPolicy(policyFile, codebases.toArray(new URL[codebases.size()]))); + policies.add(Security.readPolicy(policyFile, codebases)); } // consult each policy file for those codebases @@ -227,10 +227,14 @@ public boolean implies(ProtectionDomain domain, Permission permission) { * this is for matching the toRealPath() in the code where we have a proper plugin structure */ @SuppressForbidden(reason = "does evil stuff with paths and urls because devs and jenkins do evil stuff with paths and urls") - static URL[] parseClassPathWithSymlinks() throws Exception { - URL raw[] = JarHell.parseClassPath(); - for (int i = 0; i < raw.length; i++) { - raw[i] = PathUtils.get(raw[i].toURI()).toRealPath().toUri().toURL(); + static Set parseClassPathWithSymlinks() throws Exception { + Set raw = JarHell.parseClassPath(); + Set cooked = new HashSet<>(raw.size()); + for (URL url : raw) { + boolean added = cooked.add(PathUtils.get(url.toURI()).toRealPath().toUri().toURL()); + if (added == false) { + throw new IllegalStateException("Duplicate in classpath after resolving symlinks: " + url); + } } return raw; } From f0d6df76c65fe65aa9097779c8b860df2949b608 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 21 Mar 2017 12:11:44 -0700 Subject: [PATCH 2/2] address comments --- core/src/main/java/org/elasticsearch/bootstrap/JarHell.java | 2 +- .../src/test/java/org/elasticsearch/bootstrap/JarHellTests.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java b/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java index cdf6e516e33a8..c5346bf243d90 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java @@ -204,7 +204,7 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO String entry = root.relativize(file).toString(); if (entry.endsWith(".class")) { // normalize with the os separator, remove '.class' - entry = entry.replace(sep, ".").substring(0, entry.length() - 6); + entry = entry.replace(sep, ".").substring(0, entry.length() - ".class".length()); checkClass(clazzes, entry, path); } return super.visitFile(file, attrs); diff --git a/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java b/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java index 6321e97ddaedb..7003ef3d81efe 100644 --- a/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java +++ b/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java @@ -111,7 +111,7 @@ public void testDirAndJar() throws Exception { public void testWithinSingleJar() throws Exception { // the java api for zip file does not allow creating duplicate entries (good!) so - // this bogus jar had to be constructed with ant + // this bogus jar had to be with https://github.com/jasontedor/duplicate-classes Set jars = Collections.singleton(JarHellTests.class.getResource("duplicate-classes.jar")); try { JarHell.checkJarHell(jars);