Re: AWT Dev Review Request: 7164376 Replace use of sun.security.action.LoadLibraryAction
Hi, Mandy,
the client part of the fix looks fine. Let me ask a naive question, though.
From your explanation, I see that System.loadLibrary() is now aware of
modules. What prevents us to change LoadLibraryAction the same way?
FROM code looks much more elegant than the new (the old?) TO one.
Thanks,
Artem
On 4/26/2012 10:49 PM, Mandy Chung wrote:
7164376 Replace use of sun.security.action.LoadLibraryAction
with direct call of System.loadLibrary
Webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7164376/webrev.00/
This change is required for jdk modularization. High level summary:
it replaces the use of LoadLibraryAction:
FROM:
java.security.AccessController.doPrivileged(new LoadLibraryAction(net));
TO:
AccessController.doPrivileged(
new java.security.PrivilegedActionVoid() {
public Void run() {
System.loadLibrary(net);
return null;
}
});
It touches files in awt, security and serviceability area (cc'ed).
For this type of simple change, I think 1-2 reviewers can
review all files (simpler to review jdk.patch) and no need
for all teams to do the reviews.
System.loadLibrary and Runtime.loadLibrary loads a system library of the
given
library name that requires RuntimePermission(loadLibrary.+lib)
permission.
Many places in the JDK code loading a system native library is using the
sun.security.action.LoadLibraryAction convenient class that will load the
system library as a privileged action:
java.security.AccessController.doPrivileged(new LoadLibraryAction(net));
The search path of native libraries are coupled with an associated class
loader.
For example, the application class loader uses the path specified in the
java.library.path system property for native library lookup. The
loadLibrary
implementation uses the caller's class loader for finding the native
library being
requested. For system libraries, the class loader is null and the system
library
lookup is handled as a special case. When the
sun.security.action.LoadLibraryAction
class is used that is the caller of System.loadLibrary, the caller's
class loader
in this case is null loader and thus it always finds the native
library from
the system library path.
In a modular world, JDK modules may be loaded by multiple different
module class
loader. The following code would not work if it is expected to load a
native
library from a module which is not the module where the
sun.security.action.LoadLibraryAction lives.
For example, the management module is trying to load libmanagement.so.
Calling
the following will fail to find libmanagement.so because the caller of
System.loadLibrary is the LoadLibraryAction which is in the base module
and search the library from the base module only. To prepare for jdk
modularization, the use of LoadLibraryAction should be replaced with
a direct call of System.loadLibrary.
This patch also removes sun.security.action.LoadLibraryAction
class to avoid regression.
Thanks
Mandy
Re: AWT Dev Review Request: 7164376 Replace use of sun.security.action.LoadLibraryAction
On 5/2/2012 11:09 AM, Artem Ananiev wrote:
Hi, Mandy,
the client part of the fix looks fine.
Thanks for the review. I have pushed the changeset and hope you don't
mind I couldn't fix the changeset comment to add you as a reviewer.
Let me ask a naive question, though.
From your explanation, I see that System.loadLibrary() is now aware of
modules. What prevents us to change LoadLibraryAction the same way?
FROM code looks much more elegant than the new (the old?) TO one.
I would say the old code is little compact than the new one and both
versions look elegant to me.
The fix here is to get the caller of System.loadLibrary be in the same
class loader as the native library being loaded (see
ClassLoader.findLibrary).
Another alternative is that each component can have its own copy of
LoadLibraryAction (e.g. sun.awt.LoadLibraryAction) and so you can modify
the old code like this:
java.security.AccessController.doPrivileged(new
sun.awt.LoadLibraryAction(awt));
provided that sun.awt.LoadLibraryAction is loaded by the same class
loader with whom awt.dll is associated. Adding one copy of this simple
LoadLibraryAction utility class for each component seems overkill and
also error-prone e.g. refactoring a module into two modules require
adding another copy of this per-module utility class if both have native
libraries. This is certainly an option if the component team prefers to
use the utility class.
Mandy
Thanks,
Artem
On 4/26/2012 10:49 PM, Mandy Chung wrote:
7164376 Replace use of sun.security.action.LoadLibraryAction
with direct call of System.loadLibrary
Webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7164376/webrev.00/
This change is required for jdk modularization. High level summary:
it replaces the use of LoadLibraryAction:
FROM:
java.security.AccessController.doPrivileged(new
LoadLibraryAction(net));
TO:
AccessController.doPrivileged(
new java.security.PrivilegedActionVoid() {
public Void run() {
System.loadLibrary(net);
return null;
}
});
It touches files in awt, security and serviceability area (cc'ed).
For this type of simple change, I think 1-2 reviewers can
review all files (simpler to review jdk.patch) and no need
for all teams to do the reviews.
System.loadLibrary and Runtime.loadLibrary loads a system library of the
given
library name that requires RuntimePermission(loadLibrary.+lib)
permission.
Many places in the JDK code loading a system native library is using the
sun.security.action.LoadLibraryAction convenient class that will load
the
system library as a privileged action:
java.security.AccessController.doPrivileged(new
LoadLibraryAction(net));
The search path of native libraries are coupled with an associated class
loader.
For example, the application class loader uses the path specified in the
java.library.path system property for native library lookup. The
loadLibrary
implementation uses the caller's class loader for finding the native
library being
requested. For system libraries, the class loader is null and the system
library
lookup is handled as a special case. When the
sun.security.action.LoadLibraryAction
class is used that is the caller of System.loadLibrary, the caller's
class loader
in this case is null loader and thus it always finds the native
library from
the system library path.
In a modular world, JDK modules may be loaded by multiple different
module class
loader. The following code would not work if it is expected to load a
native
library from a module which is not the module where the
sun.security.action.LoadLibraryAction lives.
For example, the management module is trying to load libmanagement.so.
Calling
the following will fail to find libmanagement.so because the caller of
System.loadLibrary is the LoadLibraryAction which is in the base module
and search the library from the base module only. To prepare for jdk
modularization, the use of LoadLibraryAction should be replaced with
a direct call of System.loadLibrary.
This patch also removes sun.security.action.LoadLibraryAction
class to avoid regression.
Thanks
Mandy
Re: Review Request: 7164376 Replace use of sun.security.action.LoadLibraryAction
On 26/04/2012 19:49, Mandy Chung wrote: 7164376 Replace use of sun.security.action.LoadLibraryAction with direct call of System.loadLibrary Webrev: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7164376/webrev.00/ This change is required for jdk modularization. I went through the patch file and it looks good to me. I also agree with removing sun.security.action.LoadLibraryAction to ensure that new cases don't appear. -Alan
Review Request: 7164376 Replace use of sun.security.action.LoadLibraryAction
7164376 Replace use of sun.security.action.LoadLibraryAction
with direct call of System.loadLibrary
Webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7164376/webrev.00/
This change is required for jdk modularization. High level summary:
it replaces the use of LoadLibraryAction:
FROM:
java.security.AccessController.doPrivileged(new LoadLibraryAction(net));
TO:
AccessController.doPrivileged(
new java.security.PrivilegedActionVoid() {
public Void run() {
System.loadLibrary(net);
return null;
}
});
It touches files in awt, security and serviceability area (cc'ed).
For this type of simple change, I think 1-2 reviewers can
review all files (simpler to review jdk.patch) and no need
for all teams to do the reviews.
System.loadLibrary and Runtime.loadLibrary loads a system library of the given
library name that requires RuntimePermission(loadLibrary.+lib) permission.
Many places in the JDK code loading a system native library is using the
sun.security.action.LoadLibraryAction convenient class that will load the
system library as a privileged action:
java.security.AccessController.doPrivileged(new LoadLibraryAction(net));
The search path of native libraries are coupled with an associated class loader.
For example, the application class loader uses the path specified in the
java.library.path system property for native library lookup. The loadLibrary
implementation uses the caller's class loader for finding the native library
being
requested. For system libraries, the class loader is null and the system
library
lookup is handled as a special case. When the
sun.security.action.LoadLibraryAction
class is used that is the caller of System.loadLibrary, the caller's class
loader
in this case is null loader and thus it always finds the native library from
the system library path.
In a modular world, JDK modules may be loaded by multiple different module class
loader. The following code would not work if it is expected to load a native
library from a module which is not the module where the
sun.security.action.LoadLibraryAction lives.
For example, the management module is trying to load libmanagement.so. Calling
the following will fail to find libmanagement.so because the caller of
System.loadLibrary is the LoadLibraryAction which is in the base module
and search the library from the base module only. To prepare for jdk
modularization, the use of LoadLibraryAction should be replaced with
a direct call of System.loadLibrary.
This patch also removes sun.security.action.LoadLibraryAction
class to avoid regression.
Thanks
Mandy
Re: Review Request: 7164376 Replace use of sun.security.action.LoadLibraryAction
Looks fine, just a couple of nits.
src/macosx/classes/com/apple/concurrent/LibDispatchNative.java,
- the closing static brace is not indented the same as the open brace.
src/solaris/classes/sun/management/FileSystemImpl.java
src/windows/classes/sun/management/FileSystemImpl.java
- line-break coding style is different from all others; probably
better to be consistent
--Sean
On 04/26/2012 02:49 PM, Mandy Chung wrote:
7164376 Replace use of sun.security.action.LoadLibraryAction with
direct call of System.loadLibrary
Webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7164376/webrev.00/
This change is required for jdk modularization. High level summary:
it replaces the use of LoadLibraryAction:
FROM: java.security.AccessController.doPrivileged(new
LoadLibraryAction(net));
TO: AccessController.doPrivileged( new
java.security.PrivilegedActionVoid() { public Void run() {
System.loadLibrary(net); return null; } });
It touches files in awt, security and serviceability area (cc'ed).
For this type of simple change, I think 1-2 reviewers can review all
files (simpler to review jdk.patch) and no need for all teams to do
the reviews.
System.loadLibrary and Runtime.loadLibrary loads a system library of
the given library name that requires
RuntimePermission(loadLibrary.+lib) permission. Many places in the
JDK code loading a system native library is using the
sun.security.action.LoadLibraryAction convenient class that will load
the system library as a privileged action:
java.security.AccessController.doPrivileged(new
LoadLibraryAction(net));
The search path of native libraries are coupled with an associated
class loader. For example, the application class loader uses the path
specified in the java.library.path system property for native
library lookup. The loadLibrary implementation uses the caller's
class loader for finding the native library being requested. For
system libraries, the class loader is null and the system library
lookup is handled as a special case. When the
sun.security.action.LoadLibraryAction class is used that is the
caller of System.loadLibrary, the caller's class loader in this case
is null loader and thus it always finds the native library from the
system library path.
In a modular world, JDK modules may be loaded by multiple different
module class loader. The following code would not work if it is
expected to load a native library from a module which is not the
module where the sun.security.action.LoadLibraryAction lives.
For example, the management module is trying to load
libmanagement.so. Calling the following will fail to find
libmanagement.so because the caller of System.loadLibrary is the
LoadLibraryAction which is in the base module and search the library
from the base module only. To prepare for jdk modularization, the use
of LoadLibraryAction should be replaced with a direct call of
System.loadLibrary.
This patch also removes sun.security.action.LoadLibraryAction class
to avoid regression.
Thanks Mandy
Re: Review Request: 7164376 Replace use of sun.security.action.LoadLibraryAction
Thanks, Sean. I have fixed the 3 files per your comment.
Mandy
On 4/26/2012 1:59 PM, Sean Mullan wrote:
Looks fine, just a couple of nits.
src/macosx/classes/com/apple/concurrent/LibDispatchNative.java,
- the closing static brace is not indented the same as the open brace.
src/solaris/classes/sun/management/FileSystemImpl.java
src/windows/classes/sun/management/FileSystemImpl.java
- line-break coding style is different from all others; probably
better to be consistent
--Sean
On 04/26/2012 02:49 PM, Mandy Chung wrote:
7164376 Replace use of sun.security.action.LoadLibraryAction with
direct call of System.loadLibrary
Webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7164376/webrev.00/
This change is required for jdk modularization. High level summary:
it replaces the use of LoadLibraryAction:
FROM: java.security.AccessController.doPrivileged(new
LoadLibraryAction(net));
TO: AccessController.doPrivileged( new
java.security.PrivilegedActionVoid() { public Void run() {
System.loadLibrary(net); return null; } });
It touches files in awt, security and serviceability area (cc'ed).
For this type of simple change, I think 1-2 reviewers can review all
files (simpler to review jdk.patch) and no need for all teams to do
the reviews.
System.loadLibrary and Runtime.loadLibrary loads a system library of
the given library name that requires
RuntimePermission(loadLibrary.+lib) permission. Many places in the
JDK code loading a system native library is using the
sun.security.action.LoadLibraryAction convenient class that will load
the system library as a privileged action:
java.security.AccessController.doPrivileged(new
LoadLibraryAction(net));
The search path of native libraries are coupled with an associated
class loader. For example, the application class loader uses the path
specified in the java.library.path system property for native
library lookup. The loadLibrary implementation uses the caller's
class loader for finding the native library being requested. For
system libraries, the class loader is null and the system library
lookup is handled as a special case. When the
sun.security.action.LoadLibraryAction class is used that is the
caller of System.loadLibrary, the caller's class loader in this case
is null loader and thus it always finds the native library from the
system library path.
In a modular world, JDK modules may be loaded by multiple different
module class loader. The following code would not work if it is
expected to load a native library from a module which is not the
module where the sun.security.action.LoadLibraryAction lives.
For example, the management module is trying to load
libmanagement.so. Calling the following will fail to find
libmanagement.so because the caller of System.loadLibrary is the
LoadLibraryAction which is in the base module and search the library
from the base module only. To prepare for jdk modularization, the use
of LoadLibraryAction should be replaced with a direct call of
System.loadLibrary.
This patch also removes sun.security.action.LoadLibraryAction class
to avoid regression.
Thanks Mandy
Re: Review Request: 7164376 Replace use of sun.security.action.LoadLibraryAction
Thanks, Phil. FYI. I plan to use TL gate for the entire changeset.
The change will show up in the awt/2D repos when this gets integrated in
the next promoted build.
Mandy
On 4/26/2012 3:51 PM, Phil Race wrote:
All looks good to me. Compiler won't spot misspelled library names so I
did try to check all those are still the same.
-phil.
On 4/26/2012 2:20 PM, Mandy Chung wrote:
Thanks, Sean. I have fixed the 3 files per your comment.
Mandy
On 4/26/2012 1:59 PM, Sean Mullan wrote:
Looks fine, just a couple of nits.
src/macosx/classes/com/apple/concurrent/LibDispatchNative.java,
- the closing static brace is not indented the same as the open
brace.
src/solaris/classes/sun/management/FileSystemImpl.java
src/windows/classes/sun/management/FileSystemImpl.java
- line-break coding style is different from all others; probably
better to be consistent
--Sean
On 04/26/2012 02:49 PM, Mandy Chung wrote:
7164376 Replace use of sun.security.action.LoadLibraryAction with
direct call of System.loadLibrary
Webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7164376/webrev.00/
This change is required for jdk modularization. High level summary:
it replaces the use of LoadLibraryAction:
FROM: java.security.AccessController.doPrivileged(new
LoadLibraryAction(net));
TO: AccessController.doPrivileged( new
java.security.PrivilegedActionVoid() { public Void run() {
System.loadLibrary(net); return null; } });
It touches files in awt, security and serviceability area (cc'ed).
For this type of simple change, I think 1-2 reviewers can review all
files (simpler to review jdk.patch) and no need for all teams to do
the reviews.
System.loadLibrary and Runtime.loadLibrary loads a system library of
the given library name that requires
RuntimePermission(loadLibrary.+lib) permission. Many places in the
JDK code loading a system native library is using the
sun.security.action.LoadLibraryAction convenient class that will load
the system library as a privileged action:
java.security.AccessController.doPrivileged(new
LoadLibraryAction(net));
The search path of native libraries are coupled with an associated
class loader. For example, the application class loader uses the path
specified in the java.library.path system property for native
library lookup. The loadLibrary implementation uses the caller's
class loader for finding the native library being requested. For
system libraries, the class loader is null and the system library
lookup is handled as a special case. When the
sun.security.action.LoadLibraryAction class is used that is the
caller of System.loadLibrary, the caller's class loader in this case
is null loader and thus it always finds the native library from the
system library path.
In a modular world, JDK modules may be loaded by multiple different
module class loader. The following code would not work if it is
expected to load a native library from a module which is not the
module where the sun.security.action.LoadLibraryAction lives.
For example, the management module is trying to load
libmanagement.so. Calling the following will fail to find
libmanagement.so because the caller of System.loadLibrary is the
LoadLibraryAction which is in the base module and search the library
from the base module only. To prepare for jdk modularization, the use
of LoadLibraryAction should be replaced with a direct call of
System.loadLibrary.
This patch also removes sun.security.action.LoadLibraryAction class
to avoid regression.
Thanks Mandy
