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

Kernel32LibraryTests creates JNA proxy classes #9802

Closed
rmuir opened this issue Feb 20, 2015 · 5 comments · Fixed by #9923
Closed

Kernel32LibraryTests creates JNA proxy classes #9802

rmuir opened this issue Feb 20, 2015 · 5 comments · Fixed by #9923
Assignees
Labels
>test Issues or PRs that are addressing/adding tests v1.5.0 v2.0.0-beta1

Comments

@rmuir
Copy link
Contributor

rmuir commented Feb 20, 2015

With the current tests policy in place, this hits SecurityException (e.g. http://build-us-00.elasticsearch.org/job/es_core_master_window-2012/1004/console).

Does the functionality or just the test need to create these proxies? Can we avoid it? Otherwise, we can add this permission, but it is a little bit strange that JNA for e.g. unix mlockall does not need to create proxies but on windows it does.

Caused by: java.security.AccessControlException: access denied ("java.lang.reflect.ReflectPermission" "newProxyInPackage.org.elasticsearch.common.jna")
1> java.security.AccessControlContext.checkPermission(AccessControlContext.java:457)
1> java.security.AccessController.checkPermission(AccessController.java:884)
1> java.lang.SecurityManager.checkPermission(SecurityManager.java:549)
1> [...java.lang.reflect.*]
1> com.sun.jna.Native.loadLibrary(Native.java:415)
1> com.sun.jna.Native.loadLibrary(Native.java:391)
1> org.elasticsearch.common.jna.Kernel32Library.(Kernel32Library.java:50)
1> org.elasticsearch.common.jna.Kernel32Library.(Kernel32Library.java:36)
1> org.elasticsearch.common.jna.Kernel32Library$Holder.(Kernel32Library.java:45)
1> org.elasticsearch.common.jna.Kernel32Library.getInstance(Kernel32Library.java:60)
1> org.elasticsearch.common.jna.Kernel32LibraryTests.testKernel32Library(Kernel32LibraryTests.java:74)

@rmuir rmuir added >test Issues or PRs that are addressing/adding tests v2.0.0-beta1 labels Feb 20, 2015
@s1monw
Copy link
Contributor

s1monw commented Feb 23, 2015

this is related to #8993

@s1monw
Copy link
Contributor

s1monw commented Feb 23, 2015

@tlrx can you comment on this. I really wonder if the native library we load here is worth the trouble?

@tlrx
Copy link
Member

tlrx commented Feb 23, 2015

@rmuir thanks for spotting it.

Does the functionality or just the test need to create these proxies? Can we avoid it? Otherwise, we can add this permission, but it is a little bit strange that JNA for e.g. unix mlockall does not need to create proxies but on windows it does.

The functionality makes a native call on Windows to add a console control handler. The current implementation uses proxification which is the best documented way to make a native call on Windows with the JNA library. The other way is to use Direct Mapping like what is done in the unix/mlockall function.

If these proxy creation is really annoying, I think I can change the code to use Direct Mapping in Kernel32Library too.

Besides this, I didn't see a test for the Native.tryMlockall() method. I added one locally and it fails with SecurityManager enabled and the current test policy:

Caused by: java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "createSecurityManager")

I really wonder if the native library we load here is worth the trouble?

#8993 relies on the SetConsoleCtrlHandler function and was added to ensure that a node is correctly shutdown on Windows when the user closes the console that starts Elasticsearch (either by clicking Close on the console window's window menu, or by clicking the End Task button command from Task Manager, or with a TASKILL command like a stop script would do). I really think that's very important to gracefully stop the node... Without this, it's like kill -9 the JVM.

So I suggest to keep the native library (it will also be used for the mlockall equivalent on Windows) and unify the way the native calls are made in CLibrary/Kernel32Libray and limit the permissions to grant in the policy file, and finally add a test for Native.tryMlockall().

@rmuir
Copy link
Contributor Author

rmuir commented Feb 23, 2015

So I suggest to keep the native library (it will also be used for the mlockall equivalent on Windows) and unify the way the native calls are made in CLibrary/Kernel32Libray and limit the permissions to grant in the policy file, and finally add a test for Native.tryMlockall().

+1 that sounds great.

@s1monw
Copy link
Contributor

s1monw commented Feb 23, 2015

cool stuff!!

@tlrx tlrx self-assigned this Feb 27, 2015
tlrx added a commit to tlrx/elasticsearch that referenced this issue Mar 2, 2015
This commit modifies the Kernel32Library to use direct mapping instead of a proxy class when doing native calls on Windows platforms. It also adds the "createSecurityManager" permission to the tests.policy file, and adds unit tests that should have failed when the Java security manager is enabled.

Closes elastic#9802
@tlrx tlrx closed this as completed in #9923 Mar 2, 2015
@tlrx tlrx added the v1.5.0 label Mar 2, 2015
tlrx added a commit that referenced this issue Mar 2, 2015
This commit modifies the Kernel32Library to use direct mapping instead of a proxy class when doing native calls on Windows platforms. It also adds the "createSecurityManager" permission to the tests.policy file, and adds unit tests that should have failed when the Java security manager is enabled.

Closes #9802
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>test Issues or PRs that are addressing/adding tests v1.5.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants