Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
Hi Brent,
The updated webrev looks good to me.
At least, I do not see any issues.
Thanks,
Serguei
On 9/5/19 17:09, Brent Christian wrote:
Hi, David
On 9/5/19 12:52 AM, David Holmes wrote:
Good to see this all come together :)
:)
So to clarify for others any current caller to
find_class_from_class_loader that passes true for "init" was already
implicitly asking for a linked class (as you must be linked before
you can be initialized). With that in mind I would suggest that in
find_class_from_class_loader you add an assert:
! jclass find_class_from_class_loader(JNIEnv* env, Symbol* name,
jboolean init, jboolean link,
Handle loader, Handle
protection_domain,
jboolean throwError, TRAPS) {
+assert((init && link) || !init, "incorrect use of init/link
arguments");
just to ensure the callers understand this.
I'm good with adding an assert to check for coherent arguments.
(Another interpretation is that if 'init' is set, then the 'link'
argument is ignored, since linking is implied).
Aside: in looking at this I've spotted another existing bug. JNI
FindClass is not specified to perform class initialization, but the
implementation passes init=true!
OK, thanks. I've noted this in JDK-8226540.
src/java.base/share/classes/java/lang/invoke/MethodHandles.java
I don't see the need for the new note given it already has
* @throws LinkageError if the linkage fails
(Discussed in the CSR)
src/java.base/share/classes/sun/launcher/LauncherHelper.java
... > Is AccessControlException the only exception, that is not a
LinkageError, that might be thrown when linking? I would think a
number of others are possible - in particular IllegalAccessError
might occur during verification. Other than the fact a test obviously
triggered this, it's not clear why ACE should be singled out here. ??
Also discussed in the CSR[1].
test/hotspot/jtreg/serviceability/jvmti/ClassStatus/ClassStatus.java
45 // public static void foo(Foo f) { }
Unclear why this exists. Also the test refers initially to class Foo
but then uses Foo2 and Foo3. ??
I'm guessing it's just a leftover from an earlier version of the test.
I've removed the comment, and updated the test @summary.
Serguei, anything to add?
There is also a CSR[2] in need of review.
I've added a couple of comments and will add myself as a reviewer
once things are near finalized.
Thanks for taking a look.
Updated webrev at:
http://cr.openjdk.java.net/~bchristi/8212117/webrev10/
-Brent
1.
https://bugs.openjdk.java.net/browse/JDK-8222071?focusedCommentId=14288303=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14288303
The spec for the 2-arg and 3-arg Class.forName() methods states they
will "locate, load, and link" the class, however the linking part is
not ensured to happen.
Although the VM spec allows flexibility WRT when classes are linked,
this is a corner where the Class.forName() spec is not being upheld.
While this is not an issue for most usages, 8181144 [3] demonstrates
how this can be a problem (with the debugging interface, in this case).
This fix ensures that linking happens during the course of
Class.forName(). Class.forName() already @throws LinkageError, so
no spec change is needed there. MethodHandles.Lookup.findClass()
needs a small spec update, due to calling Class.forName with
init=false,
Of course Errors are not required to be caught. It is therefore
possible that the new behavior could introduce previously unseen,
possibly unhandled LinkageErrors. A new VM flag
(-XX:+ClassForNameDeferLinking) is introduced to restore the
previous behavior (to keep such code running until it can be updated).
This change surfaced a couple new "A JNI error has occurred"
situations (see 8181033[5]) in the Launcher, by way of
test/jdk/tools/launcher/MainClassCantBeLoadedTest.java
(using the 3-arg Class::forName, detailed in the bug report[4]),
and
test/jdk/tools/launcher/modules/basic/LauncherErrors.java
(using the 2-arg Class::forName).
The Launcher is updated to maintain non-confusing error messages :).
The new test included with this fix ensures that 8181144[3] is
addressed. Thanks go to Serguei Spitsyn for writing the test.
Automated corelibs and hotspot tests pass cleanly.
Thanks,
-Brent
--
1. https://bugs.openjdk.java.net/browse/JDK-8212117
2. https://bugs.openjdk.java.net/browse/JDK-8222071
3. https://bugs.openjdk.java.net/browse/JDK-8181144
4.
https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986
5. https://bugs.openjdk.java.net/browse/JDK-8181033
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
On 9/6/19 2:20 PM, Brent Christian wrote: Update webrev is here: http://cr.openjdk.java.net/~bchristi/8212117/webrev11/ with changes to jvm.h, MethodHandles.java, and Class.java. Looks good. Mandy
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
Hi, Mandy
On 9/5/19 6:03 PM, Mandy Chung wrote:
On 9/5/19 5:09 PM, Brent Christian wrote:
jvm.h
349 * Link the 'arg' class (unless ClassForNameDeferLinking is set)
I suggest to drop "(unless ...)" phrase because the flag is temporary.
Sure, good idea.
jni.cpp
The implementation of JNI FindClass has the same behavior as 1-arg
Class.forName(name). The JNI spec needs fixing but it's a separate
issue.
Right - looks like David filed JDK-8230685.
find_class_from_class_loader
I like David's suggestion to assert that if init == true, link
must be true. Alternatively, pass an enum instead of two booleans
clearly tell linking or initializing.
Yup, assert added.
Lookup::findClass:
"In particular, the method first attempts to load the requested class"
I think this sentence is no longer needed together with your change. What
about:
/**
- * Looks up a class by name from the lookup context defined by this
{@code Lookup} object. The static
- * initializer of the class is not run.
+ * Looks up a class by name from the lookup context defined by this
{@code Lookup} object.
+ * This method attempts to locate, load, and link the class, and then
determines whether
+ * the class is accessible to this {@code Lookup} object.
+ * The static initializer of the class is not run.
*
* The lookup context here is determined by the {@linkplain
#lookupClass() lookup class}, its class
- * loader, and the {@linkplain #lookupModes() lookup modes}. In
particular, the method first attempts to
- * load the requested class, and then determines whether the class is
accessible to this lookup object.
+ * loader, and the {@linkplain #lookupModes() lookup modes}.
Looks good to me.
The note you added in this method should also be added to 2-arg
Class::forName for consistency.
OK, sure.
Update webrev is here:
http://cr.openjdk.java.net/~bchristi/8212117/webrev11/
with changes to jvm.h, MethodHandles.java, and Class.java.
I went ahead and put together a specdiff[1] for the changed methods
([2][3][4]). I've updated and finalized the CSR.
Thanks for the review,
-Brent
1.
http://cr.openjdk.java.net/~bchristi/8212117/webrev11-specdiff/specdiff-summary.html
2.
http://cr.openjdk.java.net/~bchristi/8212117/webrev11-specdiff/Class-report.html#forName(java.lang.String,boolean,java.lang.ClassLoader)
3.
http://cr.openjdk.java.net/~bchristi/8212117/webrev11-specdiff/Class-report.html#forName(java.lang.Module,java.lang.String)
4.
http://cr.openjdk.java.net/~bchristi/8212117/webrev11-specdiff/invoke/MethodHandles.Lookup-report.html#findClass(java.lang.String)
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
On 9/5/19 5:09 PM, Brent Christian wrote:
Updated webrev:
http://cr.openjdk.java.net/~bchristi/8212117/webrev10/
jvm.h
349 * Link the 'arg' class (unless ClassForNameDeferLinking is set)
I suggest to drop "(unless ...)" phrase because the flag is temporary.
jni.cpp
The implementation of JNI FindClass has the same behavior as 1-arg
Class.forName(name). The JNI spec needs fixing but it's a separate
issue.
find_class_from_class_loader
I like David's suggestion to assert that if init == true, link
must be true. Alternatively, pass an enum instead of two booleans
clearly tell linking or initializing.
Lookup::findClass:
"In particular, the method first attempts to load the requested class"
I think this sentence is no longer needed together with your change. What
about:
/**
- * Looks up a class by name from the lookup context defined by this
{@code Lookup} object. The static
- * initializer of the class is not run.
+ * Looks up a class by name from the lookup context defined by this
{@code Lookup} object.
+ * This method attempts to locate, load, and link the class, and then
determines whether
+ * the class is accessible to this {@code Lookup} object.
+ * The static initializer of the class is not run.
*
* The lookup context here is determined by the {@linkplain
#lookupClass() lookup class}, its class
- * loader, and the {@linkplain #lookupModes() lookup modes}. In
particular, the method first attempts to
- * load the requested class, and then determines whether the class is
accessible to this lookup object.
+ * loader, and the {@linkplain #lookupModes() lookup modes}.
The note you added in this method should also be added to 2-arg
Class::forName for consistency.
Otherwise, the fix looks good.
Mandy
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
Hi Brent,
Some quick reply below.
On 9/5/19 5:09 PM, Brent Christian wrote:
Hi, David
On 9/5/19 12:52 AM, David Holmes wrote:
Good to see this all come together :)
:)
So to clarify for others any current caller to
find_class_from_class_loader that passes true for "init" was already
implicitly asking for a linked class (as you must be linked before
you can be initialized). With that in mind I would suggest that in
find_class_from_class_loader you add an assert:
! jclass find_class_from_class_loader(JNIEnv* env, Symbol* name,
jboolean init, jboolean link,
Handle loader, Handle
protection_domain,
jboolean throwError, TRAPS) {
+assert((init && link) || !init, "incorrect use of init/link
arguments");
just to ensure the callers understand this.
I'm good with adding an assert to check for coherent arguments.
(Another interpretation is that if 'init' is set, then the 'link'
argument is ignored, since linking is implied).
Aside: in looking at this I've spotted another existing bug. JNI
FindClass is not specified to perform class initialization, but the
implementation passes init=true!
OK, thanks. I've noted this in JDK-8226540.
src/java.base/share/classes/java/lang/invoke/MethodHandles.java
I don't see the need for the new note given it already has
* @throws LinkageError if the linkage fails
(Discussed in the CSR)
src/java.base/share/classes/sun/launcher/LauncherHelper.java
... > Is AccessControlException the only exception, that is not a
LinkageError, that might be thrown when linking? I would think a
number of others are possible - in particular IllegalAccessError
might occur during verification. Other than the fact a test obviously
triggered this, it's not clear why ACE should be singled out here. ??
Also discussed in the CSR[1].
test/hotspot/jtreg/serviceability/jvmti/ClassStatus/ClassStatus.java
45 // public static void foo(Foo f) { }
Unclear why this exists. Also the test refers initially to class Foo
but then uses Foo2 and Foo3. ??
I'm guessing it's just a leftover from an earlier version of the test.
I've removed the comment, and updated the test @summary.
Yes, it is just a left over from earlier version of the test.
Serguei, anything to add?
I'm happy this test got used and included into the webrev, thanks!
There is also a CSR[2] in need of review.
I've added a couple of comments and will add myself as a reviewer
once things are near finalized.
Thanks for taking a look.
Updated webrev at:
http://cr.openjdk.java.net/~bchristi/8212117/webrev10/
Will look at the webrev soon.
Thanks,
serguei
-Brent
1.
https://bugs.openjdk.java.net/browse/JDK-8222071?focusedCommentId=14288303=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14288303
The spec for the 2-arg and 3-arg Class.forName() methods states they
will "locate, load, and link" the class, however the linking part is
not ensured to happen.
Although the VM spec allows flexibility WRT when classes are linked,
this is a corner where the Class.forName() spec is not being upheld.
While this is not an issue for most usages, 8181144 [3] demonstrates
how this can be a problem (with the debugging interface, in this case).
This fix ensures that linking happens during the course of
Class.forName(). Class.forName() already @throws LinkageError, so
no spec change is needed there. MethodHandles.Lookup.findClass()
needs a small spec update, due to calling Class.forName with
init=false,
Of course Errors are not required to be caught. It is therefore
possible that the new behavior could introduce previously unseen,
possibly unhandled LinkageErrors. A new VM flag
(-XX:+ClassForNameDeferLinking) is introduced to restore the
previous behavior (to keep such code running until it can be updated).
This change surfaced a couple new "A JNI error has occurred"
situations (see 8181033[5]) in the Launcher, by way of
test/jdk/tools/launcher/MainClassCantBeLoadedTest.java
(using the 3-arg Class::forName, detailed in the bug report[4]),
and
test/jdk/tools/launcher/modules/basic/LauncherErrors.java
(using the 2-arg Class::forName).
The Launcher is updated to maintain non-confusing error messages :).
The new test included with this fix ensures that 8181144[3] is
addressed. Thanks go to Serguei Spitsyn for writing the test.
Automated corelibs and hotspot tests pass cleanly.
Thanks,
-Brent
--
1. https://bugs.openjdk.java.net/browse/JDK-8212117
2. https://bugs.openjdk.java.net/browse/JDK-8222071
3. https://bugs.openjdk.java.net/browse/JDK-8181144
4.
https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986
5. https://bugs.openjdk.java.net/browse/JDK-8181033
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
On 9/4/19 2:12 PM, Brent Christian wrote:
There is also a CSR[2] in need of review.
The javadoc for Lookup::findClass:
"In particular, the method first attempts to load the requested class"
I think this sentence is no longer needed together with your change.
What about:
/**
- * Looks up a class by name from the lookup context defined by this
{@code Lookup} object. The static
- * initializer of the class is not run.
+ * Looks up a class by name from the lookup context defined by this
{@code Lookup} object.
+ * This method attempts to locate, load, and link the class, and then
determines whether
+ * the class is accessible to this {@code Lookup} object.
+ * The static initializer of the class is not run.
*
* The lookup context here is determined by the {@linkplain
#lookupClass() lookup class}, its class
- * loader, and the {@linkplain #lookupModes() lookup modes}. In
particular, the method first attempts to
- * load the requested class, and then determines whether the class is
accessible to this lookup object.
+ * loader, and the {@linkplain #lookupModes() lookup modes}.
The note you added in this method should also be added to 2-arg
Class::forName for consistency.
We should add @jls as Joe suggests.
Mandy
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
Hi, Joe @jls tags added (and as long as we're in Class.java, I added @jls to the 3-arg Class.forName(), which has an equivalent paragraph). Updated webrev: http://cr.openjdk.java.net/~bchristi/8212117/webrev10/ Thanks, -Brent On 9/5/19 3:30 PM, Joe Darcy wrote: Hello, For the doc changes in MethodHandle, please supplement the paragraph 1937 * 1938 * Note that this method throws errors related to loading and linking as 1939 * specified in Sections 12.2 and 12.3 of The Java Language 1940 * Specification. with @jls tags like @jls 12.2 Loading of Classes and Interfaces @jsl 12.3 Linking of Classes and Interfaces as done elsewhere in the base module. I think it would be slightly better to remove the section numbers from the main text but keep them in the @jls tags since it is now possible to check that the listed name of the section matches what is in the JLS. Thanks, -Joe On 9/4/2019 2:12 PM, Brent Christian wrote: Hi, Please review my fix for JDK-8212117[1]. The webrev is here: http://cr.openjdk.java.net/~bchristi/8212117/webrev09/ There is also a CSR[2] in need of review. The spec for the 2-arg and 3-arg Class.forName() methods states they will "locate, load, and link" the class, however the linking part is not ensured to happen. Although the VM spec allows flexibility WRT when classes are linked, this is a corner where the Class.forName() spec is not being upheld. While this is not an issue for most usages, 8181144 [3] demonstrates how this can be a problem (with the debugging interface, in this case). This fix ensures that linking happens during the course of Class.forName(). Class.forName() already @throws LinkageError, so no spec change is needed there. MethodHandles.Lookup.findClass() needs a small spec update, due to calling Class.forName with init=false, Of course Errors are not required to be caught. It is therefore possible that the new behavior could introduce previously unseen, possibly unhandled LinkageErrors. A new VM flag (-XX:+ClassForNameDeferLinking) is introduced to restore the previous behavior (to keep such code running until it can be updated). This change surfaced a couple new "A JNI error has occurred" situations (see 8181033[5]) in the Launcher, by way of test/jdk/tools/launcher/MainClassCantBeLoadedTest.java (using the 3-arg Class::forName, detailed in the bug report[4]), and test/jdk/tools/launcher/modules/basic/LauncherErrors.java (using the 2-arg Class::forName). The Launcher is updated to maintain non-confusing error messages :). The new test included with this fix ensures that 8181144[3] is addressed. Thanks go to Serguei Spitsyn for writing the test. Automated corelibs and hotspot tests pass cleanly. Thanks, -Brent -- 1. https://bugs.openjdk.java.net/browse/JDK-8212117 2. https://bugs.openjdk.java.net/browse/JDK-8222071 3. https://bugs.openjdk.java.net/browse/JDK-8181144 4. https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986 5. https://bugs.openjdk.java.net/browse/JDK-8181033
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
Hi, David
On 9/5/19 12:52 AM, David Holmes wrote:
Good to see this all come together :)
:)
So to clarify for others any current caller to
find_class_from_class_loader that passes true for "init" was already
implicitly asking for a linked class (as you must be linked before you
can be initialized). With that in mind I would suggest that in
find_class_from_class_loader you add an assert:
! jclass find_class_from_class_loader(JNIEnv* env, Symbol* name,
jboolean init, jboolean link,
Handle loader, Handle
protection_domain,
jboolean throwError, TRAPS) {
+assert((init && link) || !init, "incorrect use of init/link arguments");
just to ensure the callers understand this.
I'm good with adding an assert to check for coherent arguments.
(Another interpretation is that if 'init' is set, then the 'link'
argument is ignored, since linking is implied).
Aside: in looking at this I've spotted another existing bug. JNI
FindClass is not specified to perform class initialization, but the
implementation passes init=true!
OK, thanks. I've noted this in JDK-8226540.
src/java.base/share/classes/java/lang/invoke/MethodHandles.java
I don't see the need for the new note given it already has
* @throws LinkageError if the linkage fails
(Discussed in the CSR)
src/java.base/share/classes/sun/launcher/LauncherHelper.java
... > Is AccessControlException the only exception, that is not a
LinkageError, that might be thrown when linking? I would think a number
of others are possible - in particular IllegalAccessError might occur
during verification. Other than the fact a test obviously triggered
this, it's not clear why ACE should be singled out here. ??
Also discussed in the CSR[1].
test/hotspot/jtreg/serviceability/jvmti/ClassStatus/ClassStatus.java
45 // public static void foo(Foo f) { }
Unclear why this exists. Also the test refers initially to class Foo but
then uses Foo2 and Foo3. ??
I'm guessing it's just a leftover from an earlier version of the test.
I've removed the comment, and updated the test @summary.
Serguei, anything to add?
There is also a CSR[2] in need of review.
I've added a couple of comments and will add myself as a reviewer once
things are near finalized.
Thanks for taking a look.
Updated webrev at:
http://cr.openjdk.java.net/~bchristi/8212117/webrev10/
-Brent
1.
https://bugs.openjdk.java.net/browse/JDK-8222071?focusedCommentId=14288303=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14288303
The spec for the 2-arg and 3-arg Class.forName() methods states they
will "locate, load, and link" the class, however the linking part is
not ensured to happen.
Although the VM spec allows flexibility WRT when classes are linked,
this is a corner where the Class.forName() spec is not being upheld.
While this is not an issue for most usages, 8181144 [3] demonstrates
how this can be a problem (with the debugging interface, in this case).
This fix ensures that linking happens during the course of
Class.forName(). Class.forName() already @throws LinkageError, so no
spec change is needed there. MethodHandles.Lookup.findClass() needs a
small spec update, due to calling Class.forName with init=false,
Of course Errors are not required to be caught. It is therefore
possible that the new behavior could introduce previously unseen,
possibly unhandled LinkageErrors. A new VM flag
(-XX:+ClassForNameDeferLinking) is introduced to restore the previous
behavior (to keep such code running until it can be updated).
This change surfaced a couple new "A JNI error has occurred"
situations (see 8181033[5]) in the Launcher, by way of
test/jdk/tools/launcher/MainClassCantBeLoadedTest.java
(using the 3-arg Class::forName, detailed in the bug report[4]),
and
test/jdk/tools/launcher/modules/basic/LauncherErrors.java
(using the 2-arg Class::forName).
The Launcher is updated to maintain non-confusing error messages :).
The new test included with this fix ensures that 8181144[3] is
addressed. Thanks go to Serguei Spitsyn for writing the test.
Automated corelibs and hotspot tests pass cleanly.
Thanks,
-Brent
--
1. https://bugs.openjdk.java.net/browse/JDK-8212117
2. https://bugs.openjdk.java.net/browse/JDK-8222071
3. https://bugs.openjdk.java.net/browse/JDK-8181144
4.
https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986
5. https://bugs.openjdk.java.net/browse/JDK-8181033
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
Hello, For the doc changes in MethodHandle, please supplement the paragraph 1937 * 1938 * Note that this method throws errors related to loading and linking as 1939 * specified in Sections 12.2 and 12.3 of The Java Language 1940 * Specification. with @jls tags like @jls 12.2 Loading of Classes and Interfaces @jsl 12.3 Linking of Classes and Interfaces as done elsewhere in the base module. I think it would be slightly better to remove the section numbers from the main text but keep them in the @jls tags since it is now possible to check that the listed name of the section matches what is in the JLS. Thanks, -Joe On 9/4/2019 2:12 PM, Brent Christian wrote: Hi, Please review my fix for JDK-8212117[1]. The webrev is here: http://cr.openjdk.java.net/~bchristi/8212117/webrev09/ There is also a CSR[2] in need of review. The spec for the 2-arg and 3-arg Class.forName() methods states they will "locate, load, and link" the class, however the linking part is not ensured to happen. Although the VM spec allows flexibility WRT when classes are linked, this is a corner where the Class.forName() spec is not being upheld. While this is not an issue for most usages, 8181144 [3] demonstrates how this can be a problem (with the debugging interface, in this case). This fix ensures that linking happens during the course of Class.forName(). Class.forName() already @throws LinkageError, so no spec change is needed there. MethodHandles.Lookup.findClass() needs a small spec update, due to calling Class.forName with init=false, Of course Errors are not required to be caught. It is therefore possible that the new behavior could introduce previously unseen, possibly unhandled LinkageErrors. A new VM flag (-XX:+ClassForNameDeferLinking) is introduced to restore the previous behavior (to keep such code running until it can be updated). This change surfaced a couple new "A JNI error has occurred" situations (see 8181033[5]) in the Launcher, by way of test/jdk/tools/launcher/MainClassCantBeLoadedTest.java (using the 3-arg Class::forName, detailed in the bug report[4]), and test/jdk/tools/launcher/modules/basic/LauncherErrors.java (using the 2-arg Class::forName). The Launcher is updated to maintain non-confusing error messages :). The new test included with this fix ensures that 8181144[3] is addressed. Thanks go to Serguei Spitsyn for writing the test. Automated corelibs and hotspot tests pass cleanly. Thanks, -Brent -- 1. https://bugs.openjdk.java.net/browse/JDK-8212117 2. https://bugs.openjdk.java.net/browse/JDK-8222071 3. https://bugs.openjdk.java.net/browse/JDK-8181144 4. https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986 5. https://bugs.openjdk.java.net/browse/JDK-8181033
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
On 9/5/19 1:36 AM, David Holmes wrote: Hi Dan, With my CSR Group member hat on On 5/09/2019 8:06 am, Daniel D. Daugherty wrote: Brent, You currently have '-XX:+ClassForNameDeferLinking' as a 'product' option, but product options are harder to remove down the road. Would it be better as a diagnostic option? A diagnostic option requires Whether a flag is product or diagnostic (or experimental) should be a basic property of the flag based on its purpose. I would not want to see a trend of making new flags diagnostic just because it is easier to get rid of them later. The expectation with this fix and flag (which I've been heavily involved in) is that production code may be impacted by the change and need to use the flag to restore previous behaviour. So it really is a product flag that end users should be comfortable in using if they need it. This is the key phrase: > production code may be impacted by the change and need to > use the flag to restore previous behaviour. Thanks for making that clear. Maybe I missed it in what I read, but it is now clear that this should be a product flag. Dan The removal process for a product flag is phased (deprecate, obsolete, expire) but not particularly onerous. There is an expectation that this flag may be deprecated in 15 as it is intended as a transitional flag. Thanks, David - '-XX:+UnlockDiagnosticVMOptions' to be specified before it can be used, e.g.: java -XX:+UnlockDiagnosticVMOptions -XX:+ClassForNameDeferLinking Foo so it is a bit harder to use, but maybe that's a Good Thing (TM). Dan On 9/4/19 5:12 PM, Brent Christian wrote: Hi, Please review my fix for JDK-8212117[1]. The webrev is here: http://cr.openjdk.java.net/~bchristi/8212117/webrev09/ There is also a CSR[2] in need of review. The spec for the 2-arg and 3-arg Class.forName() methods states they will "locate, load, and link" the class, however the linking part is not ensured to happen. Although the VM spec allows flexibility WRT when classes are linked, this is a corner where the Class.forName() spec is not being upheld. While this is not an issue for most usages, 8181144 [3] demonstrates how this can be a problem (with the debugging interface, in this case). This fix ensures that linking happens during the course of Class.forName(). Class.forName() already @throws LinkageError, so no spec change is needed there. MethodHandles.Lookup.findClass() needs a small spec update, due to calling Class.forName with init=false, Of course Errors are not required to be caught. It is therefore possible that the new behavior could introduce previously unseen, possibly unhandled LinkageErrors. A new VM flag (-XX:+ClassForNameDeferLinking) is introduced to restore the previous behavior (to keep such code running until it can be updated). This change surfaced a couple new "A JNI error has occurred" situations (see 8181033[5]) in the Launcher, by way of test/jdk/tools/launcher/MainClassCantBeLoadedTest.java (using the 3-arg Class::forName, detailed in the bug report[4]), and test/jdk/tools/launcher/modules/basic/LauncherErrors.java (using the 2-arg Class::forName). The Launcher is updated to maintain non-confusing error messages :). The new test included with this fix ensures that 8181144[3] is addressed. Thanks go to Serguei Spitsyn for writing the test. Automated corelibs and hotspot tests pass cleanly. Thanks, -Brent -- 1. https://bugs.openjdk.java.net/browse/JDK-8212117 2. https://bugs.openjdk.java.net/browse/JDK-8222071 3. https://bugs.openjdk.java.net/browse/JDK-8181144 4. https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986 5. https://bugs.openjdk.java.net/browse/JDK-8181033
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
Hi Brent,
Good to see this all come together :)
A couple of comments below.
On 5/09/2019 7:12 am, Brent Christian wrote:
Hi,
Please review my fix for JDK-8212117[1]. The webrev is here:
http://cr.openjdk.java.net/~bchristi/8212117/webrev09/
So to clarify for others any current caller to
find_class_from_class_loader that passes true for "init" was already
implicitly asking for a linked class (as you must be linked before you
can be initialized). With that in mind I would suggest that in
find_class_from_class_loader you add an assert:
! jclass find_class_from_class_loader(JNIEnv* env, Symbol* name,
jboolean init, jboolean link,
Handle loader, Handle
protection_domain,
jboolean throwError, TRAPS) {
+assert((init && link) || !init, "incorrect use of init/link arguments");
just to ensure the callers understand this.
Aside: in looking at this I've spotted another existing bug. JNI
FindClass is not specified to perform class initialization, but the
implementation passes init=true!
---
src/java.base/share/classes/java/lang/invoke/MethodHandles.java
I don't see the need for the new note given it already has
* @throws LinkageError if the linkage fails
---
src/java.base/share/classes/sun/launcher/LauncherHelper.java
So to clarify, the launcher would previously load the main class without
linking via:
mainClass = LoadMainClass(env, mode, what);
CHECK_EXCEPTION_NULL_LEAVE(mainClass);
and then it would invoke the main method which implicitly initialized
which implicitly linked and so any exceptions related to linking would
be handled when the main thread detaches, and it all gets reported
nicely. But now that we link earlier the exception could be pending
after LoadMainClass and so we would "abort". So you catch that
unexpected exception in the LauncherHelper.
Is AccessControlException the only exception, that is not a
LinkageError, that might be thrown when linking? I would think a number
of others are possible - in particular IllegalAccessError might occur
during verification. Other than the fact a test obviously triggered
this, it's not clear why ACE should be singled out here. ??
---
test/hotspot/jtreg/serviceability/jvmti/ClassStatus/ClassStatus.java
45 // public static void foo(Foo f) { }
Unclear why this exists. Also the test refers initially to class Foo but
then uses Foo2 and Foo3. ??
Otherwise all seems good.
There is also a CSR[2] in need of review.
I've added a couple of comments and will add myself as a reviewer once
things are near finalized.
Thanks,
David
-
The spec for the 2-arg and 3-arg Class.forName() methods states they
will "locate, load, and link" the class, however the linking part is not
ensured to happen.
Although the VM spec allows flexibility WRT when classes are linked,
this is a corner where the Class.forName() spec is not being upheld.
While this is not an issue for most usages, 8181144 [3] demonstrates
how this can be a problem (with the debugging interface, in this case).
This fix ensures that linking happens during the course of
Class.forName(). Class.forName() already @throws LinkageError, so no
spec change is needed there. MethodHandles.Lookup.findClass() needs a
small spec update, due to calling Class.forName with init=false,
Of course Errors are not required to be caught. It is therefore
possible that the new behavior could introduce previously unseen,
possibly unhandled LinkageErrors. A new VM flag
(-XX:+ClassForNameDeferLinking) is introduced to restore the previous
behavior (to keep such code running until it can be updated).
This change surfaced a couple new "A JNI error has occurred" situations
(see 8181033[5]) in the Launcher, by way of
test/jdk/tools/launcher/MainClassCantBeLoadedTest.java
(using the 3-arg Class::forName, detailed in the bug report[4]),
and
test/jdk/tools/launcher/modules/basic/LauncherErrors.java
(using the 2-arg Class::forName).
The Launcher is updated to maintain non-confusing error messages :).
The new test included with this fix ensures that 8181144[3] is
addressed. Thanks go to Serguei Spitsyn for writing the test.
Automated corelibs and hotspot tests pass cleanly.
Thanks,
-Brent
--
1. https://bugs.openjdk.java.net/browse/JDK-8212117
2. https://bugs.openjdk.java.net/browse/JDK-8222071
3. https://bugs.openjdk.java.net/browse/JDK-8181144
4.
https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986
5. https://bugs.openjdk.java.net/browse/JDK-8181033
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
Hi Dan, With my CSR Group member hat on On 5/09/2019 8:06 am, Daniel D. Daugherty wrote: Brent, You currently have '-XX:+ClassForNameDeferLinking' as a 'product' option, but product options are harder to remove down the road. Would it be better as a diagnostic option? A diagnostic option requires Whether a flag is product or diagnostic (or experimental) should be a basic property of the flag based on its purpose. I would not want to see a trend of making new flags diagnostic just because it is easier to get rid of them later. The expectation with this fix and flag (which I've been heavily involved in) is that production code may be impacted by the change and need to use the flag to restore previous behaviour. So it really is a product flag that end users should be comfortable in using if they need it. The removal process for a product flag is phased (deprecate, obsolete, expire) but not particularly onerous. There is an expectation that this flag may be deprecated in 15 as it is intended as a transitional flag. Thanks, David - '-XX:+UnlockDiagnosticVMOptions' to be specified before it can be used, e.g.: java -XX:+UnlockDiagnosticVMOptions -XX:+ClassForNameDeferLinking Foo so it is a bit harder to use, but maybe that's a Good Thing (TM). Dan On 9/4/19 5:12 PM, Brent Christian wrote: Hi, Please review my fix for JDK-8212117[1]. The webrev is here: http://cr.openjdk.java.net/~bchristi/8212117/webrev09/ There is also a CSR[2] in need of review. The spec for the 2-arg and 3-arg Class.forName() methods states they will "locate, load, and link" the class, however the linking part is not ensured to happen. Although the VM spec allows flexibility WRT when classes are linked, this is a corner where the Class.forName() spec is not being upheld. While this is not an issue for most usages, 8181144 [3] demonstrates how this can be a problem (with the debugging interface, in this case). This fix ensures that linking happens during the course of Class.forName(). Class.forName() already @throws LinkageError, so no spec change is needed there. MethodHandles.Lookup.findClass() needs a small spec update, due to calling Class.forName with init=false, Of course Errors are not required to be caught. It is therefore possible that the new behavior could introduce previously unseen, possibly unhandled LinkageErrors. A new VM flag (-XX:+ClassForNameDeferLinking) is introduced to restore the previous behavior (to keep such code running until it can be updated). This change surfaced a couple new "A JNI error has occurred" situations (see 8181033[5]) in the Launcher, by way of test/jdk/tools/launcher/MainClassCantBeLoadedTest.java (using the 3-arg Class::forName, detailed in the bug report[4]), and test/jdk/tools/launcher/modules/basic/LauncherErrors.java (using the 2-arg Class::forName). The Launcher is updated to maintain non-confusing error messages :). The new test included with this fix ensures that 8181144[3] is addressed. Thanks go to Serguei Spitsyn for writing the test. Automated corelibs and hotspot tests pass cleanly. Thanks, -Brent -- 1. https://bugs.openjdk.java.net/browse/JDK-8212117 2. https://bugs.openjdk.java.net/browse/JDK-8222071 3. https://bugs.openjdk.java.net/browse/JDK-8181144 4. https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986 5. https://bugs.openjdk.java.net/browse/JDK-8181033
Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
Brent, You currently have '-XX:+ClassForNameDeferLinking' as a 'product' option, but product options are harder to remove down the road. Would it be better as a diagnostic option? A diagnostic option requires '-XX:+UnlockDiagnosticVMOptions' to be specified before it can be used, e.g.: java -XX:+UnlockDiagnosticVMOptions -XX:+ClassForNameDeferLinking Foo so it is a bit harder to use, but maybe that's a Good Thing (TM). Dan On 9/4/19 5:12 PM, Brent Christian wrote: Hi, Please review my fix for JDK-8212117[1]. The webrev is here: http://cr.openjdk.java.net/~bchristi/8212117/webrev09/ There is also a CSR[2] in need of review. The spec for the 2-arg and 3-arg Class.forName() methods states they will "locate, load, and link" the class, however the linking part is not ensured to happen. Although the VM spec allows flexibility WRT when classes are linked, this is a corner where the Class.forName() spec is not being upheld. While this is not an issue for most usages, 8181144 [3] demonstrates how this can be a problem (with the debugging interface, in this case). This fix ensures that linking happens during the course of Class.forName(). Class.forName() already @throws LinkageError, so no spec change is needed there. MethodHandles.Lookup.findClass() needs a small spec update, due to calling Class.forName with init=false, Of course Errors are not required to be caught. It is therefore possible that the new behavior could introduce previously unseen, possibly unhandled LinkageErrors. A new VM flag (-XX:+ClassForNameDeferLinking) is introduced to restore the previous behavior (to keep such code running until it can be updated). This change surfaced a couple new "A JNI error has occurred" situations (see 8181033[5]) in the Launcher, by way of test/jdk/tools/launcher/MainClassCantBeLoadedTest.java (using the 3-arg Class::forName, detailed in the bug report[4]), and test/jdk/tools/launcher/modules/basic/LauncherErrors.java (using the 2-arg Class::forName). The Launcher is updated to maintain non-confusing error messages :). The new test included with this fix ensures that 8181144[3] is addressed. Thanks go to Serguei Spitsyn for writing the test. Automated corelibs and hotspot tests pass cleanly. Thanks, -Brent -- 1. https://bugs.openjdk.java.net/browse/JDK-8212117 2. https://bugs.openjdk.java.net/browse/JDK-8222071 3. https://bugs.openjdk.java.net/browse/JDK-8181144 4. https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986 5. https://bugs.openjdk.java.net/browse/JDK-8181033
RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class
Hi, Please review my fix for JDK-8212117[1]. The webrev is here: http://cr.openjdk.java.net/~bchristi/8212117/webrev09/ There is also a CSR[2] in need of review. The spec for the 2-arg and 3-arg Class.forName() methods states they will "locate, load, and link" the class, however the linking part is not ensured to happen. Although the VM spec allows flexibility WRT when classes are linked, this is a corner where the Class.forName() spec is not being upheld. While this is not an issue for most usages, 8181144 [3] demonstrates how this can be a problem (with the debugging interface, in this case). This fix ensures that linking happens during the course of Class.forName(). Class.forName() already @throws LinkageError, so no spec change is needed there. MethodHandles.Lookup.findClass() needs a small spec update, due to calling Class.forName with init=false, Of course Errors are not required to be caught. It is therefore possible that the new behavior could introduce previously unseen, possibly unhandled LinkageErrors. A new VM flag (-XX:+ClassForNameDeferLinking) is introduced to restore the previous behavior (to keep such code running until it can be updated). This change surfaced a couple new "A JNI error has occurred" situations (see 8181033[5]) in the Launcher, by way of test/jdk/tools/launcher/MainClassCantBeLoadedTest.java (using the 3-arg Class::forName, detailed in the bug report[4]), and test/jdk/tools/launcher/modules/basic/LauncherErrors.java (using the 2-arg Class::forName). The Launcher is updated to maintain non-confusing error messages :). The new test included with this fix ensures that 8181144[3] is addressed. Thanks go to Serguei Spitsyn for writing the test. Automated corelibs and hotspot tests pass cleanly. Thanks, -Brent -- 1. https://bugs.openjdk.java.net/browse/JDK-8212117 2. https://bugs.openjdk.java.net/browse/JDK-8222071 3. https://bugs.openjdk.java.net/browse/JDK-8181144 4. https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986 5. https://bugs.openjdk.java.net/browse/JDK-8181033
