Add jffi.extract.name for specific file

[headius]

Normally the library is extracted to a temp directory with an
appropriately-mangled temp name, but on platforms that cannot
delete the file before exiting the JVM this leaves those library
files to accumulate.

This change builds on the jffi.extract.dir property and adds
jffi.extract.name to specify a specific filename to use every
time, reusing any existing file at that location.

Depending on how these two properties are combined, the file will
be extracted as follows:

• Set neither: file will be extracted with a temp name under the
  default temp dir, with attempt to delete and no reuse.
• Set both: file will be the given name in the given path,
  reused if existing or extracted otherwise.
• Set only dir: file will be extracted with a temp name under the
  specified directory, with attempt to delete and no reuse.
• Set only name: file will be extracted with the given name under
  the default temp dir, reused if existing or extracted otherwise.

Note that this does not verify that the loaded file matches the
one bundled with jffi. This is left up to the user, since any
verification of the existing file will be subject to TOC-to-TOU
vulnerability. It is not possible to have the JVM atomically
verify and load the given file.

This partially addresses #97 by allowing Windows users to specify
a known file path to reuse across runs.

[headius]

I am unsure how important the missing verification might be.

In the extreme, if the directory used is not under tight control, a malicious application could exploit this by installing its own library at that location. However if the path is secure, the file should also be secure even if it unpacks lazily. The concern is whether users will take appropriate precautions and how much we should protect them from the worst case.

Verification would be tricky. A checksum or hash of the library would seem to be a natural mechanism, but the file could be modified between the time that its hash is calculated and the library is loaded into the JVM (a TOC-to-TOU exploit). We could call something to verify the library's identity after it has loaded, but by then the exploit is already in place and anything we call could be simulated by mimicking what our own binary does.

So, this may be a "buyer beware" sort of feature.

[david-bouyssie]

Thanks!

Do you think that a minimal check could be to verify the library size?

Also, regarding the retained solution I'm wondering if a boolean property won't be enough.
Something like jffi.extract.use-fixed-name or jffi.extract.use-bundled-name or jffi.extract.no-temp-name.
So anything that would tell JFFI to extract the bundled library without renaming it.

The described behaviors would be the same, but this would avoid the JFFI user to determine a library name and also a library extension according to the platform.

[headius]

Do you think that a minimal check could be to verify the library size?

You know, that's a good point, Just because there's a TOCTOU issue doesn't mean we shouldn't at least perform some minimum checks.

Also, regarding the retained solution I'm wondering if a boolean property won't be enough.

Well this is also a fair point, but I am reluctant to add a simple boolean (maybe jffi.extract.tempname=false?) and then a few weeks from now someone comes back and asks for a way to set the name. A few justifications for going all the way to a name property:

• Someone will want it eventually, so might as well add it now.
• System administrators may want a predictable name; the packaged libraries have platform and version information in the name that will change across platforms and releases.
• Two different JFFI-based apps may want different names in the same location?

I am stretching a bit on the last one but I still feel like having a simple name property is best. Perhaps to avoid the extension issue, we allow user to provide a simple name and add any platform-specific extension that is needed?

[enebo]

I believe so long as you specify the name and not only the dir you will have to fingerprint this. Not for security but to make sure you do not run two versions of JRuby which require different versions of jffi. If it is only the dir and the name is versioned then perhaps than can be omitted (unless you want a security check).

[enebo]

Obviously if you are using this as a library and not from JRuby the same concern will likely apply.

[headius]

I have pushed another commit that enhances this feature:

• If the jffi.extra.name property is set but empty (e.g. java -Djffi.extract.name -Djffi.extract.dir=/blah) then a default name of "jffi-#.#.ext" will be used with the jffi version and the platform's dynamic library extension.
• If a name is specified and does not end with the appropriate extension, it will be added to the name before unpacking.
• If the target file already exists it will be verified with SHA-256 hashing. This does not prevent TOCTOU issues but it is the best we can do.

[david-bouyssie]

LGTM, thanks.

I think the blank property is a good trick, and it should really simplify my use case.
However, it will require to be well documented because it may be not that obvious.

I also agree with your arguments against a single boolean field.
The retained solution is more flexible.

I can now test the PR in its current form. Will do that tomorrow.

[david-bouyssie]

So I tried but I'm facing an issue:

java.lang.UnsatisfiedLinkError: could not load FFI provider jnr.ffi.provider.jffi.Provider
        at jnr.ffi.provider.InvalidProvider$1.loadLibrary(InvalidProvider.java:48)
        at jnr.ffi.LibraryLoader.load(LibraryLoader.java:403)

I tried with:

System.setProperty("jffi.extract.dir","./lib")
System.setProperty("jffi.extract.name","jffi-1.2.dll")

and also

System.setProperty("jffi.extract.dir","./lib")
System.setProperty("jffi.extract.name","")

Note that I have mixed JFFI 1.3.2-SNAPSHOT with JNR-FFI 2.2.1.
Maybe this is explaining this current issue?

[headius]

This might be a commit I attempted and reverted for #93 which changed the version numbers we use to load the native libraries out of the jar. I have merged that change to this branch.

[headius]

I am running into some issues as well on MacOS and will get back to you once they are fixed.

[headius]

@david-bouyssie Ok my issues may only be with the jffi tests. jnr-ffi appears to be running green with the reverted commit. Could you give it another try (on this branch)?

[david-bouyssie]

Yes sure, will do that later this week-end.

[headius]

@david-bouyssie Were you able to give this a try?

[david-bouyssie]

Not yet, but should be able this evening.

[david-bouyssie]

So, the previous issue is now solved but now I have another unclear issue:
the JVM process is hanging, with one core at 100% CPU use.

If I stop the JVM and re-run my app (without deleting the jffi DLL), then it works fine.

So it looks like the file is extracted, but is not well loaded, like if it was stuck in a while loop.
I have not debugged the JVM execution because I was wondering if you could first spot the problem easily or not.

[headius]

@david-bouyssie Ok something must be wrong with how I am calculating the SHA. I will investigate (and add a test this time).

[headius]

I found the source of the bug (a bad refactoring on my part) and some additional small issues I will fix up.

[headius]

@david-bouyssie Thanks for your patience and sorry about having you test broken code.

The main bug was that I refactored out a call to InputStream.available used as a terminating condition in a loop, forgetting that it will update as the stream progresses until it reaches zero.

There were other minor issues with the verification of the file:

• It tried to verify when using a temp file, but broke on the size check because we always ask the JVM to create a zero-length tempfile before we write it. Fixed by only verifying target file if we know we are using a specific file path.
• Verification error did not propagate up to the UnsatisfiedLinkError eventually thrown.
• Digest mismatch was raw byte values rather than hex. I just removed it because it isn't really actionable information.

I have manually confirmed the digest hash check and various configurations and added two additional full test runs that use a temp file or a specific file.

Try again please? 🙇‍♂️

[david-bouyssie]

No problem, actually I'm glad to see it was not my mistake 😄
But more importantly very happy that you solved this second issue.

I'll try today.

[david-bouyssie]

LGTM 🚀