NPE in PluginsService when starting elasticsearch with a wrong user #5196
Conversation
When starting elasticsearch with a wrong linux user, it could generate a `NullPointerException` when `PluginsService` tries to list available plugins in `./plugins` dir. To reproduce: * create a plugins directory with `rwx` rights for root user only * launch elasticsearch from another account (elasticsearch for example) It was supposed to be fixed with elastic#4186, but sadly it's not :-( Closes elastic#5195.
| if (pluginsFile != null) { | ||
| for (File pluginFile : pluginsFiles) { | ||
| for (File pluginFile : pluginsFile.listFiles()) { |
There was a problem hiding this comment.
the below code still fails with an NPE... so maybe there is another null check for pluginsFIle.listFiles() needed?
File pluginsFile = new File("/tmp/does-not-existing");
if (pluginsFile != null) {
for (File pluginFile : pluginsFile.listFiles()) {
// NPE happens here, due to trying to iterate over null object
}
}
There was a problem hiding this comment.
Thanks @spinscale! Nice catch.
There was a problem hiding this comment.
provided tips too fast.. you might want to use file.isDirectory(), file.canRead() etc.. might make more sense than simple null checks :-)
There was a problem hiding this comment.
@spinscale Actually, we check exists() and isDirectory() some lines before.
So I guess what you described can not happen.
The more I think about it the more I think the NPE was not what I was thinking about but as you said pluginsFile.listFile() send a NPE if the directory is not readable.
Going to add a simple file.canRead() as you suggested in place of testing for something which can not be null!
| assert directory != null && logger != null; | ||
|
|
||
| if (!directory.exists()) { | ||
| logger.debug("plugins directory does not exist [{}].", directory.getAbsolutePath()); |
There was a problem hiding this comment.
this is more generic now, not plugins anymore... same for the lines below
|
codewise this is ok, but can you please test this with a couple of weird chmods (or even test it automatically?)? To me it looks like the plugins service can be even unittested, but changing permissions is not easily possible... seems like manual testing |
|
tested this on mac os with directories 'chmod'ed to 644 or 000 and no exception was thrown (this was what its all about, right?) LGTM |
|
Pushed in master and 1.x. |
clintongormley
added
>bug
v1.1.0
v2.0.0-beta1
:Core/Infra/Plugins
When starting elasticsearch with a wrong linux user, it could generate a
NullPointerExceptionwhenPluginsServicetries to list available plugins in./pluginsdir.To reproduce:
rwxrights for root user onlyIt was supposed to be fixed with #4186, but sadly it's not :-(
Closes #5195.