Skip to content

Cleanup some things after removal of joda-time hack #18959

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rmuir wants to merge 3 commits into from
Closed

Cleanup some things after removal of joda-time hack #18959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5747cc7
treat codebases as sets
rmuir Jun 18, 2016
e2d7a71
treat codebases/modules/plugins as unique
rmuir Jun 18, 2016
294bb66
track plugins/modules separate
rmuir Jun 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions core/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ dependencies {

// time handling, remove with java 8 time
compile 'joda-time:joda-time:2.9.4'
// joda 2.0 moved to using volatile fields for datetime
// When updating to a new version, make sure to update our copy of BaseDateTime
compile 'org.joda:joda-convert:1.2'

// json and yaml
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.security.Permissions;
import java.security.Policy;
import java.security.ProtectionDomain;
import java.util.Collections;
import java.util.Map;

/** custom policy for union of static and dynamic permissions */
Expand All @@ -49,7 +50,7 @@ final class ESPolicy extends Policy {

public ESPolicy(PermissionCollection dynamic, Map<String,Policy> 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 {
Expand Down
28 changes: 17 additions & 11 deletions core/src/main/java/org/elasticsearch/bootstrap/JarHell.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,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;
Expand Down Expand Up @@ -92,7 +94,7 @@ public static void checkJarHell() throws Exception {
* @return array of URLs
* @throws IllegalStateException if the classpath contains empty elements
*/
public static URL[] parseClassPath() {
public static Set<URL> parseClassPath() {
return parseClassPath(System.getProperty("java.class.path"));
}

Expand All @@ -103,11 +105,11 @@ 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<URL> 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];
Set<URL> urlElements = new LinkedHashSet<>(); // order is already lost, but some filesystems have it
for (int i = 0; i < elements.length; i++) {
String element = elements[i];
// Technically empty classpath element behaves like CWD.
Expand Down Expand Up @@ -135,21 +137,25 @@ 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() +
Copy link
Contributor

@s1monw s1monw Jun 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation is off here?

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

/**
* Checks the set of URLs for duplicate classes
* @throws IllegalStateException if jar hell was found
*/
@SuppressForbidden(reason = "needs JarFile for speed, just reading entries")
public static void checkJarHell(URL urls[]) throws Exception {
public static void checkJarHell(Set<URL> urls) throws Exception {
ESLogger 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
Expand All @@ -167,8 +173,8 @@ public static void checkJarHell(URL urls[]) throws Exception {
}
if (path.toString().endsWith(".jar")) {
if (!seenJars.add(path)) {
logger.debug("excluding duplicate classpath element: {}", path);
continue; // we can't fail because of sheistiness with joda-time
throw new IllegalStateException("jar hell!" + System.lineSeparator() +
"duplicate jar on classpath: " + path);
}
logger.debug("examining jar: {}", path);
try (JarFile file = new JarFile(path.toString())) {
Expand Down Expand Up @@ -197,7 +203,7 @@ public static void checkJarHell(URL urls[]) throws Exception {
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);
}
Expand Down Expand Up @@ -274,8 +280,8 @@ static void checkClass(Map<String,Path> clazzes, String clazz, Path jarpath) {
if (clazz.startsWith("org.apache.log4j")) {
return; // go figure, jar hell for what should be System.out.println...
}
if (clazz.equals("org.joda.time.base.BaseDateTime")) {
return; // apparently this is intentional... clean this up
if (clazz.equals("module-info")) {
return; // not really a java class: java 9 modular jar
}
throw new IllegalStateException("jar hell!" + System.lineSeparator() +
"class: " + clazz + System.lineSeparator() +
Expand Down
42 changes: 29 additions & 13 deletions core/src/main/java/org/elasticsearch/bootstrap/Security.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@
import java.security.Permissions;
import java.security.Policy;
import java.security.URIParameter;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;

/**
* Initializes SecurityManager with necessary permissions.
Expand Down Expand Up @@ -133,19 +133,23 @@ static void configure(Environment environment, boolean filterBadDefaults) throws
@SuppressForbidden(reason = "proper use of URL")
static Map<String,Policy> getPluginPermissions(Environment environment) throws IOException, NoSuchAlgorithmException {
Map<String,Policy> map = new HashMap<>();
// collect up lists of plugins and modules
List<Path> pluginsAndModules = new ArrayList<>();
// collect up set of plugins and modules by listing directories.
Set<Path> pluginsAndModules = new LinkedHashSet<>(); // order is already lost, but some filesystems have it
if (Files.exists(environment.pluginsFile())) {
try (DirectoryStream<Path> 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<Path> 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);
}
}
}
}
Expand All @@ -155,15 +159,18 @@ static Map<String,Policy> 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<URL> codebases = new ArrayList<>();
Set<URL> codebases = new LinkedHashSet<>(); // order is already lost, but some filesystems have it
try (DirectoryStream<Path> 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) {
Expand All @@ -181,24 +188,33 @@ static Map<String,Policy> getPluginPermissions(Environment environment) throws I
/**
* Reads and returns the specified {@code policyFile}.
* <p>
* Resources (e.g. jar files and directories) listed in {@code codebases} location
* Jar files listed in {@code codebases} location
* will be provided to the policy file via a system property of the short name:
* e.g. <code>${codebase.joda-convert-1.2.jar}</code> 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<URL> 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) {
Copy link
Contributor

@s1monw s1monw Jun 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we set some system property or so that we can use to at least asssert that we are running tests? I mean it would be nice if somebody tries to sneak in here if we can fail? just a suggestion can also be a followup?

Copy link
Contributor Author

@rmuir rmuir Jun 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is absolutely nothing I can do here. Otherwise it will break IDEs running these shitty tests, where no sysprop is set :( Thats always been the problem with so much of this stuff!

continue; // tests :(
}
String previous = System.setProperty("codebase." + shortName, url.toString());
if (previous != null) {
throw new IllegalStateException("codebase properly 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);
}
}
Expand Down
9 changes: 5 additions & 4 deletions core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,22 +406,23 @@ 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<URL> jars = new ArrayList<>();
jars.addAll(Arrays.asList(JarHell.parseClassPath()));
final Set<URL> jars = new HashSet<>(JarHell.parseClassPath());

// read existing bundles. this does some checks on the installation too.
PluginsService.getPluginBundles(pluginsDir);

// 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);
}

/**
Expand Down
Loading