Skip to content
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.

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

NPE in PluginsService when starting elasticsearch with a wrong user #5196

Closed
wants to merge 9 commits into from
22 changes: 22 additions & 0 deletions src/main/java/org/elasticsearch/common/io/FileSystemUtils.java
Expand Up @@ -228,6 +228,28 @@ public static void copyFile(File sourceFile, File destinationFile) throws IOExce
}
}

/**
* Check that a directory exists, is a directory and is readable
* by the current user
*/
public static boolean isAccessibleDirectory(File directory, ESLogger logger) {
assert directory != null && logger != null;

if (!directory.exists()) {
logger.debug("plugins directory does not exist [{}].", directory.getAbsolutePath());
Copy link
Contributor

Choose a reason for hiding this comment

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

this is more generic now, not plugins anymore... same for the lines below

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed! :)

return false;
}
if (!directory.isDirectory()) {
logger.debug("plugins directory is not a directory [{}].", directory.getAbsolutePath());
return false;
}
if (!directory.canRead()) {
logger.debug("plugins directory is not readable [{}].", directory.getAbsolutePath());
return false;
}
return true;
}

private FileSystemUtils() {

}
Expand Down
67 changes: 32 additions & 35 deletions src/main/java/org/elasticsearch/plugins/PluginsService.java
Expand Up @@ -30,6 +30,7 @@
import org.elasticsearch.common.component.AbstractComponent;
import org.elasticsearch.common.component.LifecycleComponent;
import org.elasticsearch.common.inject.Module;
import org.elasticsearch.common.io.FileSystemUtils;
import org.elasticsearch.common.settings.ImmutableSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
Expand Down Expand Up @@ -316,11 +317,8 @@ synchronized public PluginsInfo info() {
}

private void loadPluginsIntoClassLoader() {
File pluginsFile = environment.pluginsFile();
if (!pluginsFile.exists()) {
return;
}
if (!pluginsFile.isDirectory()) {
File pluginsDirectory = environment.pluginsFile();
if (!FileSystemUtils.isAccessibleDirectory(pluginsDirectory, logger)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

static import?

return;
}

Expand All @@ -342,40 +340,39 @@ private void loadPluginsIntoClassLoader() {
return;
}

File[] pluginsFiles = pluginsFile.listFiles();
if (pluginsFile != null) {
for (File pluginFile : pluginsFiles) {
if (pluginFile.isDirectory()) {
if (logger.isTraceEnabled()) {
logger.trace("--- adding plugin [" + pluginFile.getAbsolutePath() + "]");
}
try {
// add the root
addURL.invoke(classLoader, pluginFile.toURI().toURL());
// gather files to add
List<File> libFiles = Lists.newArrayList();
if (pluginFile.listFiles() != null) {
libFiles.addAll(Arrays.asList(pluginFile.listFiles()));
}
File libLocation = new File(pluginFile, "lib");
if (libLocation.exists() && libLocation.isDirectory() && libLocation.listFiles() != null) {
libFiles.addAll(Arrays.asList(libLocation.listFiles()));
}
for (File plugin : pluginsDirectory.listFiles()) {
// We check that subdirs are directories and readable
if (!FileSystemUtils.isAccessibleDirectory(plugin, logger)) {
continue;
}

// if there are jars in it, add it as well
for (File libFile : libFiles) {
if (!(libFile.getName().endsWith(".jar") || libFile.getName().endsWith(".zip"))) {
continue;
}
addURL.invoke(classLoader, libFile.toURI().toURL());
}
} catch (Throwable e) {
logger.warn("failed to add plugin [" + pluginFile + "]", e);
if (logger.isTraceEnabled()) {
logger.trace("--- adding plugin [{}]", plugin.getAbsolutePath());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove the if statement now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? You don't want to trace that we are actually adding a new plugin? It could help I think.

}

try {
// add the root
addURL.invoke(classLoader, plugin.toURI().toURL());
// gather files to add
List<File> libFiles = Lists.newArrayList();
if (plugin.listFiles() != null) {
libFiles.addAll(Arrays.asList(plugin.listFiles()));
}
File libLocation = new File(plugin, "lib");
if (libLocation.exists() && libLocation.isDirectory() && libLocation.listFiles() != null) {
libFiles.addAll(Arrays.asList(libLocation.listFiles()));
}

// if there are jars in it, add it as well
for (File libFile : libFiles) {
if (!(libFile.getName().endsWith(".jar") || libFile.getName().endsWith(".zip"))) {
continue;
}
addURL.invoke(classLoader, libFile.toURI().toURL());
}
} catch (Throwable e) {
logger.warn("failed to add plugin [" + plugin + "]", e);
}
} else {
logger.debug("failed to list plugins from {}. Check your right access.", pluginsFile.getAbsolutePath());
}
}

Expand Down