dkpro / dkpro-core

Collection of software components for natural language processing (NLP) based on the Apache UIMA framework.
https://dkpro.github.io/dkpro-core
Other
196 stars 67 forks source link

Fails to run the binary, with "noexec" /tmp #107

Closed reckart closed 9 years ago

reckart commented 9 years ago
What steps will reproduce the problem?
1. Run the TreeTagger wrapper in a linux system with /tmp mounted with "noexec". 
2. Tree tagger binary copied into the /tmp cannot be executed, and fails. 

A work-around: 
One can tell Java to use a different temporary directory, like followings. 

> java -Djava.io.tmpdir="somedir"   [the rest],
or in maven
> mvn -Djava.io.tmpdir="somedir" [build command] 

This makes Java to use a different directory, and it successfully runs. 

Possible Solution: 
A "fall back" code would be nice to end users; if the copy & call on /tmp fails, try
to make a /tmp/ folder on current (eh, this might be ugly but...) PWD and use it and
clean it ... 

Original issue reported on code.google.com by nohtaegil on 2013-01-07 10:53:23

reckart commented 9 years ago
(No text was entered with this change)

Original issue reported on code.google.com by richard.eckart on 2013-01-07 16:21:18

reckart commented 9 years ago
I think using PWD is a bad idea as that may be basically anything, but trying DKPRO_HOME
and the user's home should work.

Original issue reported on code.google.com by richard.eckart on 2013-01-07 16:22:41

reckart commented 9 years ago
At least on *nix systems, executable files should never be put in the system temp directory
by default, nor should they be put in some arbitrary location in the user's home directory.
 Instead we should follow the conventions of the XDG Base Directory Specification <http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html>
– that is, check for the presence of an $XDG_RUNTIME_DIR environment variable, and
if set, use that directory for the binary.  On systems implementing the XDG Base Directory
Specification (which is pretty much every modern *nix), this directory is guaranteed
to be readable, writable, and executable by the user (and only the user).  If this
environment variable is missing or empty, only then should the application try falling
back to the system temp directories.  On POSIX/FHS systems, this is $TMPDIR (or failing
that, /tmp).  Microsoft Windows probably has its own conventions but I don't know what
those are.

Original issue reported on code.google.com by tristan.miller@nothingisreal.com on 2013-01-09 11:16:30

reckart commented 9 years ago
An excellent idea. Thanks for pointing this out.

Original issue reported on code.google.com by richard.eckart on 2013-01-09 13:41:48

reckart commented 9 years ago
(No text was entered with this change)

Original issue reported on code.google.com by richard.eckart on 2013-02-16 11:01:26

reckart commented 9 years ago
(No text was entered with this change)

Original issue reported on code.google.com by pedrobssantos on 2013-04-26 13:00:29

reckart commented 9 years ago
/tmp directory is not used any more. Now it is checked if the
XDG_RUNTIME_DIR variable is set. If it is, this folder is used. Otherwise, it is
checked if the DKPRO_HOME environment is set. If it is, then it is used. If it
is not, then the USER.HOME folder is used.

Original issue reported on code.google.com by pedrobssantos on 2013-04-26 15:49:04

reckart commented 9 years ago
I think it's not quite fixed yet. E.g. there is no handling of DKPRO_HOME (or any of
the other variables) being set but not having been created as a directory yet. The
getClasspathAsFolder() isn't updated yet.

Original issue reported on code.google.com by richard.eckart on 2013-04-26 15:55:17

reckart commented 9 years ago
I tested it without having the directory created, and it created on the fly. At least
on ubuntu is not a problem. Do you know if it is problem in other OS? The getClasspathAsFolder()
method was updated, thanks for noticing this.

Original issue reported on code.google.com by pedrobssantos on 2013-04-26 17:05:05

reckart commented 9 years ago
Since we do not use /tmp anymore at all now, we get test failures on the build server,
because the home folder is actually not writable., e.g. in testGetUrlAsFile().

We also seem to have a problem that the temporary files created by getUrlAsFile can
actually conflict with each other. E.g. if getUrlAsFile("http://lala/test.txt") and
then getUrlAsFile("http://lolo/test.txt") is called, the temporary file created in
the first call appears to be overwritten.

Original issue reported on code.google.com by richard.eckart on 2013-04-26 18:09:42

reckart commented 9 years ago
The problem of conflicting file names was not there previously, because File.createTempFile(name,
"."+suffix) creates unique names. The ability to create a unique name for each call
of createTempFile() must be retained.

Original issue reported on code.google.com by richard.eckart on 2013-04-26 18:14:25

reckart commented 9 years ago
I think your test did not what you think it did. I just ran a manual test pointing XDG_RUNTIME_DIR
to a folder that does not exist and ran the testGetUrlAsFile(). The result was a FileNotFoundException
because FileOutputStream could not write a file to a non-existing folder.

So we have several problems now:

- /tmp is not used anymore at all, even if it could be used
- createTempFile() does not create unique names like File.createTempFile() does
- the case that the temporary directory does not exist is not handled
- also the case that a temporary directory does exist but is not writable that it cannot
be used for executable files is not handled

I'd suggest the following:

- revert getUrlAsFile() and getClasspathAsFolder() to the previous state and document
that they must not be used for executable binaries
- create a new getUrlAsExecutable() method which makes sure that the temporary file
is created in a location which permits executables
- getUrlAsExecutable() should try to create the executable file in the methods and
order: 
-- File.createTempFile() then try to make temporary file executable
-- if this fails try creating the executable in XDG_RUNTIME_DIR under a unique name.
Make sure no file which by accident may already have that name in that location is
overwritten
-- if this fails, try DKPRO_HOME, but according to policy not DKPRO_HOME directly a
subdirectory, I suggest DKPRO_HOME/temp. Again be careful about the unique name and
about not overwriting
-- if this fails, try the users' home. Again not the home directly, but user.home/.dkpro/temp.
(unique name/overwriting/yaddayadda)
-- if this also fails, produce an IOException with a useful error message explaining
what was tried, what failed, why and what is suggested to fix the situation (e.g. make
sure that user.home/dkpro/temp is writable and can be used for executables)

Making sure that the created file really has a unique name component may the the most
difficult part. Apparrently the JDK uses a SecureRandom.nextLong() for this. Not fool-proof,
but probably good enough for our purposes.

Original issue reported on code.google.com by richard.eckart on 2013-04-26 18:43:24

reckart commented 9 years ago
It's probably also a good idea to use FileSystem.getFileSystem().createFileExcusively(file)
to create the temporary file, like File.createTempFile() does. Best take a look at
the sources of File.createTempFile() but don't just copy it ;)

Original issue reported on code.google.com by richard.eckart on 2013-04-26 18:49:30

reckart commented 9 years ago
Apparrently FileSystem is an internal class of the JDK that we cannot use, so we'll
probably have to resort to a simple loop which repeatedly tests if there already is
a file with the unique name, try the next unique name if there is and tries to create
the file if it's not there. Then we pray that the situation that two threads or processes
try to create a file by the same name in almost the same moment is so rare that it
is no problem.

Original issue reported on code.google.com by richard.eckart on 2013-04-26 19:09:24

reckart commented 9 years ago
(No text was entered with this change)

Original issue reported on code.google.com by pedrobssantos on 2013-05-03 13:48:41

reckart commented 9 years ago
I'm having trouble following what the problem is here.  You say there's a possibility
of conflicting file names and in Comment #14 propose some sort of polling loop as a
workaround.  What's wrong with just using File.createTempFile(String prefix, String
suffix, File directory)?

Original issue reported on code.google.com by tristan.miller@nothingisreal.com on 2013-05-06 10:50:22

reckart commented 9 years ago
Hum, inability to read the documentation on my side I guess ;) I didn't know of that
signature.

Original issue reported on code.google.com by richard.eckart on 2013-05-06 10:52:14

reckart commented 9 years ago
OK.  As a follow-up question, though, how do you intend to make the file executable?
 DKPro uses Java 6, but support for changing POSIX file permissions first appears in
Java 7.

Original issue reported on code.google.com by tristan.miller@nothingisreal.com on 2013-05-06 14:26:03

reckart commented 9 years ago
If Java 7, then we can use setExecutable(). Otherwise, there are some provisions in
the PlatformDetector class to help, in particular getChmodCmd(). I don't know if this
is used anywhere right now.

Original issue reported on code.google.com by richard.eckart on 2013-05-06 15:38:41

reckart commented 9 years ago
Actually, Java 6 already supports File.setExecutable(true). The provisions in PlatformDetector
had been used when the stuff was still supporting Java 5.

Original issue reported on code.google.com by richard.eckart on 2013-05-06 15:43:04

reckart commented 9 years ago
(No text was entered with this change)

Original issue reported on code.google.com by pedrobssantos on 2013-05-08 08:20:17