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

Only check existence for absolute paths in env.resolveConfig() #10854

Closed
wants to merge 1 commit into from

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Apr 28, 2015

When resolving a path using env.resolveConfig(String), it first checks for the path existence in the current working directory. This can fails when the security manager is enabled and elasticsearch not started from the ES_HOME directory like with init.d/systemd scripts or also with ./path/to/elasticsearch/bin/elasticsearch: Files.exist() resolves against the working directory and fails with a SecurityException.

I'm not sure we always need to set "user.dir" so I add a check for absolute path. This couldn't hurt much since the following f1.toUri().toURL() expects an absolute path.

What do you think?

When resolving a path using env.resolveConfig(String), it first checks for the path existence in the current working directory. This can fails when the security manager is enabled and elasticsearch not started from the ES_HOME directory like with init.d/systemd scripts or also with ./path/to/elasticsearch/bin/elasticsearch: Files.exist() resolves against the working directory and fails with a SecurityException.

I'm not sure we always need to set "user.dir" so I add a check for absolute path. This couldn't hurt much since f1.toUri().toURL() expects an absolute path.
@tlrx
Copy link
Member Author

tlrx commented Apr 28, 2015

More background: with ES installed with an RPM, creating a new index failed because the MapperService resolves env.resolveConfig("default-mapping.json") which calls Files.exists("default-mapping.json") and fails with a SecurityException.

@rjernst
Copy link
Member

rjernst commented Apr 29, 2015

Why wouldn't we make the paths relative to ES_HOME if they are not absolute?

@tlrx
Copy link
Member Author

tlrx commented Apr 29, 2015

When ES is installed with an RPM or Deb package, the configuration dir is not necessarily relative to ES_HOME, so we can't resolve path relative to ES_HOME.

@s1monw
Copy link
Contributor

s1monw commented Apr 29, 2015

hmm I think this look good to me - this is actually more intuitive than what we did before? @rjernst

@rjernst
Copy link
Member

rjernst commented Apr 29, 2015

Ok, I was confused about what we were resolving here, and after looking at the rest of the method I am now more confused. :)

I'm not sure this entire try/catch that is being modified should exist at all. It does not appear to be useful. What I see is the following logic:

  1. Try to resolve the path on its own (only as an absolute path with your change)
  2. Remove "/" from the beginning of path, making it relative (but this should no longer be possible with your change)
  3. Take the path (which may have been absolute and made relative, at least on a nix system), and try to resolve it against the config dir.

It seems to me 2 is no longer valid (and was pretty bogus in the first place) and that 1 can actually be rolled into 3 because configFile.resolve(path) will just return path if it is absolute.

@tlrx
Copy link
Member Author

tlrx commented May 7, 2015

Better fix has been merged in #10923, closing this one.

@tlrx tlrx closed this May 7, 2015
@kevinkluge kevinkluge removed the review label May 7, 2015
@tlrx tlrx deleted the only-check-absolute-paths branch May 18, 2015 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants