Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
On 23/07/2019 19:51, Mandy Chung wrote: (Coming back to this patch and ready to push this change later today) Here is the updated webrev: http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.02/ This includes the change that Peter suggested accepting m2 == null. The updated isModuleAccessible looks okay to me. -Alan
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
(Coming back to this patch and ready to push this change later today) Here is the updated webrev: http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.02/ This includes the change that Peter suggested accepting m2 == null. Mandy On 7/5/19 12:29 PM, Peter Levart wrote: Hi Mandy, Yes, either way will avoid double canRead/isExported calls against the same module. Although perhaps, an API with null prevModule would be more consistent with other API methods that use null prevLookupClass etc... Regards, Peter
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
Hi Mandy,
Yes, either way will avoid double canRead/isExported calls against the
same module. Although perhaps, an API with null prevModule would be more
consistent with other API methods that use null prevLookupClass etc...
Regards, Peter
On 7/5/19 8:19 PM, Mandy Chung wrote:
On 7/5/19 8:24 AM, Peter Levart wrote:
Hi Mandy,
On 7/2/19 7:20 PM, Mandy Chung wrote:
Webrev updated:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.01/
I just skimmed across code and I think there's a little optimization
possible in VerifyAccess:
:
...instead of seting prevLookupModule to lookupModule in case
prevLookupClass is null (common case) causing double canRead and
isExported checks against the same module in isModuleAccessible()
(for positive outcome - the case where we want the check to be quick):
alternatively, keep prevLookupModule = lookupModule if prevLookupClass
is null
:
...and then check against null in isModuleAccessible():
and skip for the case where m1 == m2 like:
public static boolean isModuleAccessible(Class refc, Module m1,
Module m2) {
Module refModule = refc.getModule();
assert refModule != m1 || refModule != m2;
int mods = getClassModifiers(refc);
if (isPublic(mods)) {
if (m1.canRead(refModule) && (m1 == m2 ||
m2.canRead(refModule))) {
String pn = refc.getPackageName();
// refc is exported package to at least both m1 and m2
if (refModule.isExported(pn, m1) && (m1 == m2 ||
refModule.isExported(pn, m2)))
return true;
}
}
return false;
}
Mandy
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
On 7/5/19 8:24 AM, Peter Levart wrote:
Hi Mandy,
On 7/2/19 7:20 PM, Mandy Chung wrote:
Webrev updated:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.01/
I just skimmed across code and I think there's a little optimization
possible in VerifyAccess:
:
...instead of seting prevLookupModule to lookupModule in case
prevLookupClass is null (common case) causing double canRead and
isExported checks against the same module in isModuleAccessible() (for
positive outcome - the case where we want the check to be quick):
alternatively, keep prevLookupModule = lookupModule if prevLookupClass
is null
:
...and then check against null in isModuleAccessible():
and skip for the case where m1 == m2 like:
public static boolean isModuleAccessible(Class refc, Module m1,
Module m2) {
Module refModule = refc.getModule();
assert refModule != m1 || refModule != m2;
int mods = getClassModifiers(refc);
if (isPublic(mods)) {
if (m1.canRead(refModule) && (m1 == m2 ||
m2.canRead(refModule))) {
String pn = refc.getPackageName();
// refc is exported package to at least both m1 and m2
if (refModule.isExported(pn, m1) && (m1 == m2 ||
refModule.isExported(pn, m2)))
return true;
}
}
return false;
}
Mandy
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
Hi Mandy,
On 7/2/19 7:20 PM, Mandy Chung wrote:
Webrev updated:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.01/
I just skimmed across code and I think there's a little optimization
possible in VerifyAccess:
224 // cross-module access
225 // 1. refc is in different module from lookupModule, or
226 // 2. refc is in lookupModule and a different module
from prevLookupModule
227 Module prevLookupModule = prevLookupClass != null ?
prevLookupClass.getModule()
228 :
lookupModule;
229 assert refModule != lookupModule || refModule !=
prevLookupModule;
230 if (isModuleAccessible(refc, lookupModule,
prevLookupModule))
231 return true;
...instead of seting prevLookupModule to lookupModule in case
prevLookupClass is null (common case) causing double canRead and
isExported checks against the same module in isModuleAccessible() (for
positive outcome - the case where we want the check to be quick):
250 public static boolean isModuleAccessible(Class refc,
Module m1, Module m2) {
251 Module refModule = refc.getModule();
252 assert refModule != m1 || refModule != m2;
253 int mods = getClassModifiers(refc);
254 if (isPublic(mods)) {
255 if (m1.canRead(refModule) && m2.canRead(refModule)) {
256 String pn = refc.getPackageName();
257
258 // refc is exported package to at least both m1 and m2
259 if (refModule.isExported(pn, m1) &&
refModule.isExported(pn, m2))
260 return true;
261 }
262 }
263 return false;
264 }
...you could set prevLookupModule to null if prevLookupClass is null:
Module prevLookupModule = prevLookupClass != null
?
prevLookupClass.getModule()
: null;
...and then check against null in isModuleAccessible():
public static boolean isModuleAccessible(Class refc, Module m1,
Module m2) {
Module refModule = refc.getModule();
assert refModule != m1 || refModule != m2;
int mods = getClassModifiers(refc);
if (isPublic(mods)) {
if (m1.canRead(refModule) && (m2 == null ||
m2.canRead(refModule))) {
String pn = refc.getPackageName();
// refc is exported package to at least both m1 and m2
if (refModule.isExported(pn, m1) && (m2 == null ||
refModule.isExported(pn, m2)))
return true;
}
}
return false;
}
I think this would be faster since canRead and isExported are not trivial...
Regards, Peter
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
On 02/07/2019 18:20, Mandy Chung wrote: : I've taken several passes over the changes and I don't see anything obviously wrong. One minor nit is that the @throws IllegalAccessException in accessClass(Class) needs to be updated to allow for it to thrown when not accessible from the previous lookup class. Also VerifyAccess::isModuleAccessible shouldn't need to if the package is exported unconditionally as it is covered by isExported(pn, m1) && isExported(pn, m2). Good point. Fixed. Webrev updated: http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.01/ The updated @throws IAE in accessClass and the updated VerifyAccess::isModuleAccessible looks good. -Alan.
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
On 7/2/19 6:59 AM, Alan Bateman wrote: This is really good work and fixes several issues left over from JDK 9. The compatibility issues (mostly small/advanced) are unfortunate but necessary and I hope it won't impact too many things. It will need a detail release note once the CSR is done and the changes are in. The existing usage of `privateLookupIn` I am aware of is using a private lookup object and do deep reflection in one module. I expect that the compatibility risk should be low while the library developers/maintainers are encouraged to test with EA early. I've taken several passes over the changes and I don't see anything obviously wrong. One minor nit is that the @throws IllegalAccessException in accessClass(Class) needs to be updated to allow for it to thrown when not accessible from the previous lookup class. Also VerifyAccess::isModuleAccessible shouldn't need to if the package is exported unconditionally as it is covered by isExported(pn, m1) && isExported(pn, m2). Good point. Fixed. Webrev updated: http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.01/ Mandy
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
On Tue, 2 Jul 2019 at 16:51, Alan Bateman wrote: > > Are you using privateLookupIn and dropping PRIVATE access? Are you > teleporting in Lookup.in? It might be that you don't have to change > anything. I don't really have any control over how the Lookup object is created. It is provide by users of the library. And I just use the "raw" version (this might change at some point). /Kasper /Kasper
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
Hi Daniel,
Thanks for catching these.
On 7/2/19 8:03 AM, Daniel Fuchs wrote:
Hi Mandy,
Don't count me as reviewer; this is only editorial:
In MethodHandles.java:
165 * A caller, specified as a {@code Lookup} object, is in
module {@code M1} is
166 * allowed to do deep reflection on module {@code M2} and
package of the target class
167 * if and only if all of the following conditions are {@code
true}:
There are two 'is' in this sentence, which makes it a bit difficult
to parse. Should the first be removed?
Yes the first "is" is removed.
* A caller, specified as a {@code Lookup} object, in module {@code M1}
is ...
169 * If there is a security manager, its {@code
checkPermission} method is
170 * called to check {@code
ReflectPermission("suppressAccessChecks")} and
171 * that must return normally.
172 *
173 * If there is a security manager, its {@code
checkPermission} method is
174 * called to check {@code
ReflectPermission("suppressAccessChecks")}.
175 * This method must return normally.
Probably one of these two bullets should be removed: they look
the same to me.
Removed. Thanks for catching it.
=
585 * A {@code Lookup} with {@link #PUBLIC} mode and a lookup
class in {@code M1}
586 * can access public types in a module {@code M2} when {@code
M2} is readable to
587 * {@code M1} when the type is in a package of {@code M2}
that is exported to at least {@code M1}.
I believe there is "and" missing here:
<<... when M2 is readable to M1 *and* when the type >>
Fixed.
===
Is the specdiff report incomplete? I was hoping to see the table
added to the Lookup class level API under
582 * Cross-module lookups
but it seems to be missing from the specdiff?
The specdiff tool is broken. Sigh... I workaround to generate the
method/field summary but hit another bug that it does not generate the
class summary.
BTW: is the right header? I thought was reserved for the
class name. John Gibbon will know more ;-)
It should be . Updated. This was written prior to that change.
thanks
Mandy
best regards,
-- daniel
On 02/07/2019 03:47, Mandy Chung wrote:
On 7/1/19 12:51 PM, Mandy Chung wrote:
This is an enhancement to |`Lookup::in`| and
|`MethodHandles::privateLookupIn`| API
for cross module teleporting. A `Lookup` object will record the
previous
lookup class from which this |Lookup| object was teleported such that
the access check will use both the previous lookup class and the
current
|lookup| context (current lookup class and allowed modes) to
determine if
a type is accessible to this `Lookup` object or not.
In a nutshell, `T` in M2 is accessible to a `Lookup` object on `C`
(lookup class in M1) and `PLC` (previous lookup class in M0) if and
only if
1. both M0 and M1 can read M2
2. T is in a package that is exported from M2 at least to both M0
and M1
Detailed specification is in Lookup class spec and `accessClass`
javadoc.
The relevant spec about cross-module teleporting is in the Lookup class
spec and `Lookup::in` and `MethodHandles::privateLookupIn`.
CSR: https://bugs.openjdk.java.net/browse/JDK-8226916
webrev:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.00
javadoc:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.Lookup.html
http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.html#privateLookupIn(java.lang.Class,java.lang.invoke.MethodHandles.Lookup)
I have yet to generate the spec diff. The tool is currently broken
due to javadoc change. I'll try to workaround it and post the
spec diff soon.
specdiff:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/specdiff/overview-summary.html
Mandy
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
On 02/07/2019 16:35, Kasper Nielsen wrote: Right now, I'm using a cache of ClassValue> with lookupClass and allowedModes as parameters [3]. This will obviously break with these changes. Are you using privateLookupIn and dropping PRIVATE access? Are you teleporting in Lookup.in? It might be that you don't have to change anything. -Alan
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
Hi Mandy, Are there any plans to make functionality from VerifyAccess public in any way? Changes to the access model like these are extremely frustrating because the access check mechanisms in VerifyAccess are not available outside of the JDK. I've posted about this before [1][2], But if you are a library developer there are basically no way to cache a MethodHandle and allow it to be used against multiple different Lookup objects because the access checks are so complicated. Another really useful addition would be a LookupValue class (similar to ClassValue) which would allow you to cache access check information for a given lookup object. Right now, I'm using a cache of ClassValue> with lookupClass and allowedModes as parameters [3]. This will obviously break with these changes. A simple use case is implement a simple serialization library with the following signature String serializeToString(Class clazz, MethodHandles.Lookup lookup). Trying to implement something like this that is both performant, secure and classloader friendly is really difficult. Creating varhandles/methodhandles for every call to 'serializeToString ' is not performant for obvious reasons. So you need to cache it in some way, which is currently non-trivial. /Kasper [1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2018-October/014012.html [2] https://bugs.openjdk.java.net/browse/JDK-8213251 [3] https://gist.github.com/kaspernielsen/540aacaf0581e7be80b0ebe3348f4a24 On Mon, 1 Jul 2019 at 20:51, Mandy Chung wrote: > > This is an enhancement to |`Lookup::in`| and > |`MethodHandles::privateLookupIn`| API > for cross module teleporting. A `Lookup` object will record the previous > lookup class from which this |Lookup| object was teleported such that > the access check will use both the previous lookup class and the current > |lookup| context (current lookup class and allowed modes) to determine if > a type is accessible to this `Lookup` object or not. > > In a nutshell, `T` in M2 is accessible to a `Lookup` object on `C` > (lookup class in M1) and `PLC` (previous lookup class in M0) if and only if > 1. both M0 and M1 can read M2 > 2. T is in a package that is exported from M2 at least to both M0 and M1 > > Detailed specification is in Lookup class spec and `accessClass` javadoc. > The relevant spec about cross-module teleporting is in the Lookup class > spec and `Lookup::in` and `MethodHandles::privateLookupIn`. > > CSR: https://bugs.openjdk.java.net/browse/JDK-8226916 > > webrev: > http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.00 > > javadoc: > http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.Lookup.html > > http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.html#privateLookupIn(java.lang.Class,java.lang.invoke.MethodHandles.Lookup) > > I have yet to generate the spec diff. The tool is currently broken > due to javadoc change. I'll try to workaround it and post the > spec diff soon. > > thanks > Mandy
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
Hi Mandy,
Don't count me as reviewer; this is only editorial:
In MethodHandles.java:
165 * A caller, specified as a {@code Lookup} object, is in
module {@code M1} is
166 * allowed to do deep reflection on module {@code M2} and
package of the target class
167 * if and only if all of the following conditions are {@code
true}:
There are two 'is' in this sentence, which makes it a bit difficult
to parse. Should the first be removed?
* A caller, specified as a {@code Lookup} object, in module {@code M1}
is ...
169 * If there is a security manager, its {@code
checkPermission} method is
170 * called to check {@code
ReflectPermission("suppressAccessChecks")} and
171 * that must return normally.
172 *
173 * If there is a security manager, its {@code checkPermission}
method is
174 * called to check {@code
ReflectPermission("suppressAccessChecks")}.
175 * This method must return normally.
Probably one of these two bullets should be removed: they look
the same to me.
=
585 * A {@code Lookup} with {@link #PUBLIC} mode and a lookup
class in {@code M1}
586 * can access public types in a module {@code M2} when {@code
M2} is readable to
587 * {@code M1} when the type is in a package of {@code M2} that
is exported to at least {@code M1}.
I believe there is "and" missing here:
<<... when M2 is readable to M1 *and* when the type >>
===
Is the specdiff report incomplete? I was hoping to see the table
added to the Lookup class level API under
582 * Cross-module lookups
but it seems to be missing from the specdiff?
BTW: is the right header? I thought was reserved for the
class name. John Gibbon will know more ;-)
best regards,
-- daniel
On 02/07/2019 03:47, Mandy Chung wrote:
On 7/1/19 12:51 PM, Mandy Chung wrote:
This is an enhancement to |`Lookup::in`| and
|`MethodHandles::privateLookupIn`| API
for cross module teleporting. A `Lookup` object will record the previous
lookup class from which this |Lookup| object was teleported such that
the access check will use both the previous lookup class and the current
|lookup| context (current lookup class and allowed modes) to determine if
a type is accessible to this `Lookup` object or not.
In a nutshell, `T` in M2 is accessible to a `Lookup` object on `C`
(lookup class in M1) and `PLC` (previous lookup class in M0) if and
only if
1. both M0 and M1 can read M2
2. T is in a package that is exported from M2 at least to both M0 and M1
Detailed specification is in Lookup class spec and `accessClass` javadoc.
The relevant spec about cross-module teleporting is in the Lookup class
spec and `Lookup::in` and `MethodHandles::privateLookupIn`.
CSR: https://bugs.openjdk.java.net/browse/JDK-8226916
webrev:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.00
javadoc:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.Lookup.html
http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.html#privateLookupIn(java.lang.Class,java.lang.invoke.MethodHandles.Lookup)
I have yet to generate the spec diff. The tool is currently broken
due to javadoc change. I'll try to workaround it and post the
spec diff soon.
specdiff:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/specdiff/overview-summary.html
Mandy
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
On 01/07/2019 20:51, Mandy Chung wrote: This is an enhancement to |`Lookup::in`| and |`MethodHandles::privateLookupIn`| API for cross module teleporting. A `Lookup` object will record the previous lookup class from which this |Lookup| object was teleported such that the access check will use both the previous lookup class and the current |lookup| context (current lookup class and allowed modes) to determine if a type is accessible to this `Lookup` object or not. In a nutshell, `T` in M2 is accessible to a `Lookup` object on `C` (lookup class in M1) and `PLC` (previous lookup class in M0) if and only if 1. both M0 and M1 can read M2 2. T is in a package that is exported from M2 at least to both M0 and M1 Detailed specification is in Lookup class spec and `accessClass` javadoc. The relevant spec about cross-module teleporting is in the Lookup class spec and `Lookup::in` and `MethodHandles::privateLookupIn`. CSR: https://bugs.openjdk.java.net/browse/JDK-8226916 webrev: http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.00 javadoc: http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.Lookup.html http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.html#privateLookupIn(java.lang.Class,java.lang.invoke.MethodHandles.Lookup) This is really good work and fixes several issues left over from JDK 9. The compatibility issues (mostly small/advanced) are unfortunate but necessary and I hope it won't impact too many things. It will need a detail release note once the CSR is done and the changes are in. I've taken several passes over the changes and I don't see anything obviously wrong. One minor nit is that the @throws IllegalAccessException in accessClass(Class) needs to be updated to allow for it to thrown when not accessible from the previous lookup class. Also VerifyAccess::isModuleAccessible shouldn't need to if the package is exported unconditionally as it is covered by isExported(pn, m1) && isExported(pn, m2). -Alan
Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
On 7/1/19 12:51 PM, Mandy Chung wrote: This is an enhancement to |`Lookup::in`| and |`MethodHandles::privateLookupIn`| API for cross module teleporting. A `Lookup` object will record the previous lookup class from which this |Lookup| object was teleported such that the access check will use both the previous lookup class and the current |lookup| context (current lookup class and allowed modes) to determine if a type is accessible to this `Lookup` object or not. In a nutshell, `T` in M2 is accessible to a `Lookup` object on `C` (lookup class in M1) and `PLC` (previous lookup class in M0) if and only if 1. both M0 and M1 can read M2 2. T is in a package that is exported from M2 at least to both M0 and M1 Detailed specification is in Lookup class spec and `accessClass` javadoc. The relevant spec about cross-module teleporting is in the Lookup class spec and `Lookup::in` and `MethodHandles::privateLookupIn`. CSR: https://bugs.openjdk.java.net/browse/JDK-8226916 webrev: http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.00 javadoc: http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.Lookup.html http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.html#privateLookupIn(java.lang.Class,java.lang.invoke.MethodHandles.Lookup) I have yet to generate the spec diff. The tool is currently broken due to javadoc change. I'll try to workaround it and post the spec diff soon. specdiff: http://cr.openjdk.java.net/~mchung/jdk14/8173978/specdiff/overview-summary.html Mandy
Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access
This is an enhancement to |`Lookup::in`| and |`MethodHandles::privateLookupIn`| API for cross module teleporting. A `Lookup` object will record the previous lookup class from which this |Lookup| object was teleported such that the access check will use both the previous lookup class and the current |lookup| context (current lookup class and allowed modes) to determine if a type is accessible to this `Lookup` object or not. In a nutshell, `T` in M2 is accessible to a `Lookup` object on `C` (lookup class in M1) and `PLC` (previous lookup class in M0) if and only if 1. both M0 and M1 can read M2 2. T is in a package that is exported from M2 at least to both M0 and M1 Detailed specification is in Lookup class spec and `accessClass` javadoc. The relevant spec about cross-module teleporting is in the Lookup class spec and `Lookup::in` and `MethodHandles::privateLookupIn`. CSR: https://bugs.openjdk.java.net/browse/JDK-8226916 webrev: http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.00 javadoc: http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.Lookup.html http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.html#privateLookupIn(java.lang.Class,java.lang.invoke.MethodHandles.Lookup) I have yet to generate the spec diff. The tool is currently broken due to javadoc change. I'll try to workaround it and post the spec diff soon. thanks Mandy
