JDK-8192936 : RI does not follow the JVMTI RedefineClasses spec that is too strict in the definition
  • Type: Bug
  • Component: hotspot
  • Sub-Component: jvmti
  • Affected Version: 6.0,7,8,9,10,11
  • Priority: P2
  • Status: Closed
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2017-12-02
  • Updated: 2019-08-07
  • Resolved: 2019-04-20
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 13
13 b18Fixed
Related Reports
CSR :  
Relates :  
Relates :  
Relates :  
Relates :  
Description
There is a discrepancy between the behaviour of Java 9 and the jvmti
spec (http://cr.openjdk.java.net/~mr/jigsaw/spec/jvmti.html#RedefineClasses)

The spec states:

"The redefinition may change method bodies, the constant pool and attributes.
The redefinition must not add, remove or rename fields or methods, change the signatures of methods, change modifiers, or change inheritance."

However, RI allows private methods and inner classes to be added without throwing
JVMTI_ERROR_UNSUPPORTED_REDEFINITION_METHOD_ADDED.

There is the transform method:

        @Override
        public byte[] transform(Module module, ClassLoader loader, String className, Class<?> classBeingRedefined,
                ProtectionDomain protectionDomain, byte[] classfileBuffer) throws IllegalClassFormatException {
            if (className.equals("p/C")) {
                transformModule(module);
                byte[] retransformedClazz1Bytes = new LocalDirClassLoader("Instrumentation.redefine_after", null).loadClassData("p/C.class");
            } else {
                return null;
            }
        }

that substitutes the class p/C 
----------------------------------------------------------------
package p;

public class C {
    public static boolean checkReads() {
        return false;
    }

    public static boolean checkUses() {
        return false;
    }

    public static boolean checkProvides() {
        return false;
    }
}
----------------------------------------------------------------
with the class p/C
----------------------------------------------------------------
package p;

import p3.E;

import java.util.ServiceLoader;

public class C {
    public static boolean checkReads() {
        return E.getOne() == 1;
    }

    public static boolean checkUses() {
        return ServiceLoader.load(C.class.getModule().getLayer(), ServiceOne.class)
                .findFirst()
                .map(s -> s instanceof ServiceOneImpl)
                .orElse(false);
    }

    public static boolean checkProvides() {
        return ServiceLoader.load(C.class.getModule().getLayer(), ServiceTwo.class)
                .findFirst()
                .map(s -> s instanceof ServiceTwoImpl)
                .orElse(false);
    }
}
----------------------------------------------------------------

This retransformation is allowed by RI, although it contradicts the specification above because the transformed class contains extra public static final InnerClass Lookup and methods:

private static synthetic Method lambda$checkProvides$1:"(Lp/ServiceTwo;)Ljava/lang/Boolean;"
private static synthetic Method lambda$checkUses$0:"(Lp/ServiceOne;)Ljava/lang/Boolean;"

The specification should be weakened according to accessibility of methods and/or fields(?).

Comments
Verified that the option was added and incorrect redefinition is not allowed when the option is false.
07-08-2019

Current decision is to align the implementation with the spec. This decision is going to cause some inconveniences to the customers. The proposed solution is to introduce a compatibility mode with new command-line VM option: -XX:{+|-}AllowRedefinitionToAddOrDeleteMethods New option will enable old behavior and allow the JVM TI RedefineClasses and RetransformClasses to add/delete private static and private final instance methods in the new class versions. Without this option the old behavior will be disabled. New option is deprecated right away. The plan is to keep it for a couple of releases to allow customers (tool vendors) to remove dependency on old behavior from their tools. For details, please, see the CSR JDK-8221528.
28-03-2019

Nice!
06-02-2019

Changed from P1 to P2 because the tck-red label has been removed
19-07-2018

We'll revisit the test once specification is updated. For now I've removed tck-red-11 label. [~lkuskov], [~pchinnasamy]: Can you please handle the test exclusion from JCK 11?
18-07-2018

As part of the fix the spec is going to be changed and current test will not be valid.
17-07-2018

Yes, I will add the email discussion to the comments. It was in my plan.
05-06-2018

Suggestion from David Holmes: I think specifying either what can be done, or what can't be done, are equally problematic. You need to start with a full list of everything that might be changed and then identify which can and can't be changed. From a semantic perspective Robert was right that a capability should be telling you what you can do, not what you can't. But that's why I preferred to leave the what you can/can't do to the definition of the methods, rather then the capability. The capability for "private final" doesn't really make any sense. A private method by definition is final as it can not be overridden. Requiring a private method have the ACC_FINAL bit set seems unnecessarily restrictive as it is not normal Java coding standard/style to use "final" with private methods. That means that this reduces to a capability to add/remove private methods - whether instance or static and regardless of whether declared final or not. Cheers, David
28-04-2018

Below is how looks like a capability-based suggestion. I'm posting it here to show how this approach would look like. It can be improved but there is some doubt it serves well our need in spec update. David Holmes shared a better suggestion that will be added to the next comment. - Remove from RetransformClasses and RedefineClasses spec the statments about what must not do these functions: The retransformation/redefinition must not add, remove or rename fields or methods, change the signatures of methods, change modifiers, or change inheritance. These restrictions may be lifted in future versions. - I'm not sure yet what to do with the statement: See the error return description below for information on error codes returned if an unsupported retransformation/redefinition is attempted. We can leave this statement where it is now or move to the capabilities description. - Update the spec of the can_retransform_classes and can_redefine_classes capabilities: can_retransform_classes Can retransform classes with RetransformClasses, where the class is non-primitive and non-array and the retransformation may change method bodies, the constant pool and attributes. The capability does not allow to add, remove or rename fields or methods, change the signatures of methods, change modifiers, or change inheritance. can_redefine_classes Can redefine classes with RedefineClasses, where the class is non-primitive and non-array and the redefinition may change method bodies, the constant pool and attributes. The capability does not allow to add, remove or rename fields or methods, change the signatures of methods, change modifiers, or change inheritance. - Add new capabilities: can_redefine_classes_add_or_delete_private_static_methods Can redefine classes with RedefineClasses add or delete private static methods. can_redefine_classes_add_or_delete_private_final_methods Can redefine classes with RedefineClasses supports add or delete private final methods. can_retransform_classes_add_or_delete_private_static_methods Can retransform classes with RetransformClasses add or delete private static methods. can_retransform_classes_add_or_delete_private_final_methods Can retransform classes with RetransformClasses supports add or delete private final methods. A few comments: 1. In general, I'm not sure how to tell that the can_retransform_classes and can_redefine_classes are not supposed to support. It has to be about what do they support. 2. The 'add' and 'delete' are combined in one capability, as it seems, they have to be allowed or disallowed together. If adding methods is allowed then deletion has to be allowed as well for consistency. 3. It looks a little bit strange that we have separated versions of capabilities for retransform and redefine. I'm not sure we really wanted it. But it is for consistency with can_retransform_classes/can_redefine_classes. Not sure, it is useful, as these caps control the same.
28-04-2018

The issue is labeled as tck-red-11 to make it must-fix for JDK 11. The IBM realization strictly interprets the specification hence some JCK tests fail when they are wrongly adding private methods during a class transformation. Although these tests are incorrect according to the spec they pass on the RI. This disparency should be resolved either on the spec side or on RI.
17-04-2018

There is one more comment from Karen that is worth to mention: p.s. In 8192936 Leonid also mentions addition of inner classes. How does that not fit with the JVMTI specification? My understanding of inner classes is that they are completely controlled by the addition or removal of attributes, which is allowed in JVMTI today. I believe the first restriction on attribute modification is for upcoming Nestmates since it changes access controls in the JVM.
16-04-2018

In the comments - Dan recorded that the change was added as part of https://bugs.openjdk.java.net/browse/JDK-6404550 which Robert fixed in 2006. As Alan states this was a request to instrument native methods for late attach - and the NetBeans request was a JVMTI Spec request: https://bugs.openjdk.java.net/browse/JDK-6341303 which was closed as a duplicate. So let me see if I understand the connection between this request and the addition of private methods (static and instance): Step 1: Request for wrapping native methods: 6263317 - which worked by replacing a native method with a non-native method with the same name which wraps a new native method with a new name. This works when the classfile is transfomed at ClassFileLoadHook time and you can add methods. 6263317 added the JVMTI spec/java.lang.instrument doc and implementation for setNativeMethodPrefix and can_set_native_method_prefix capability. Step 2: Add dynamic attach - and the mechanism does not work for dynamically attached agents since you can not add methods. Solution: 6404550 was to allow dynamic addition of private static and private final instance methods which are actually the wrapped native methods. We missed the JVMTI spec change for this. If this is accurate,Sergeui - it would help to add this to JDK-8192936 to justify why we would want a specific change. thanks, Karen
16-04-2018

> The text in RedefineClasses: "The redefinition must not add, remove or rename fields or methods, change the signatures of methods, change modifiers, or change inheritance. These restrictions may be lifted in future versions." should not be in the body of the definition, but in the capabilities -- and not in that form. That text even conflicts with the existence of the can_redefine_any_class capability. I don't agree with that. Perhaps there should have been an explicit capability for exactly what kind of redefinition can be done but we missed that boat. The can_redefine_any_class capability is about the set of classes that may be redefined _not_ about what redefinitions can be applied to them. The JDWP spec tried to define what could/could-not be done, with canUnrestrictedlyRedefineClasses, and made a mess of it. We've added clarifications to that for the JEP-181 - see:the CSR JDK-8197445
16-04-2018

A nice suggestion from Robert: Indeed: the RedefineClasses spec conflicts with the SetNativeMethodPrefix spec. The text in RedefineClasses: "The redefinition must not add, remove or rename fields or methods, change the signatures of methods, change modifiers, or change inheritance. These restrictions may be lifted in future versions." should not be in the body of the definition, but in the capabilities -- and not in that form. That text even conflicts with the existence of the can_redefine_any_class capability. Maybe something like: Capabilities Optional Functionality: might not be implemented for all virtual machines. The following capability (as returned by GetCapabilities) must be true to use this function. Capability Effect can_redefine_classes Can redefine classes with RedefineClasses, which do not add, remove or rename fields or methods, change the signatures of methods, change modifiers, or change inheritance. Optional Features can_redefine_any_class Can modify (retransform or redefine) any non-primitive non-array class. See IsModifiableClass. Ideally there would been a can_redefine_adding_private_method or something, but that can't be done retroactively. -Robert . . . Rather than reverse engineer the spec from the hotspot implementation... Capabilities are the mechanism by which the level of functionality is defined. Capabilities say what can be done, not what can't. The wording "The redefinition must not add, remove or rename fields or methods, change the signatures of methods, change modifiers, or change inheritance. These restrictions may be lifted in future versions." was clearly left from an earlier version. Probably should not have been there in the first place but certainly should have updated/removed as JVMTI evolved. As written, it explicitly conflicts with the can_redefine_any_class capability: can_redefine_any_class Can modify (retransform or redefine) any non-primitive non-array class. See IsModifiableClass. as well as conflicting with the implementation of SetNativeMethodPrefixes et. al. and the RI in terms of private/final/static. i would suggest removing the quoted text and adding to the text for the can_redefine_classes capability. Maybe something like: can_redefine_classes Can redefine classes with RedefineClasses, where the class is non-primitive and non-array and the redefinition does not add, remove or rename fields or methods, change the signatures of methods, change modifiers, or change inheritance. -Robert
16-04-2018

I'll add a email discussion comments to keep "all these little pieces of this information" in one place. Karen: On 21/02/2018 1:54 AM, Karen Kinnear wrote: > Folks, > > As part of the Valhalla EG discussions for JVMTI changes for nestmates (thank you Serguei and David), > IBM brought up a request that we update the JVMTI documentation to reflect that we allow addition > of private methods. > > Is there a reason we do not document this? I���m inviting those who were involved at the time - please include > others if needed. David H.: > This issue is tracked by: > https://bugs.openjdk.java.net/browse/JDK-8192936 > "RI does not follow the JVMTI RedefineClasses spec that is too strict in the definition" > > As I wrote there ... It is not at all clear how JDK-6404550 morphed into > "Permit the adding or deleting of private final/static methods with redefine" - nor why > those changes failed to make any change to the spec itself. > It is also unclear whether the add/delete is restricted to final/static methods or > any private method? I can see that the intent was to only allow something > that would not perturb the vtable for existing instances. Serguei: > I support documenting this in the JVMTI spec and had a plan to fix it in 11. > However, it is not clear to me yet if we have a consensus on it. > . . . > I agree, there is a confusion somewhere. > Is it possible, the JDK-6404550 in JIRA is a different bug than the one in the Bugtraq system? > The JDK-6404550 in JIRA has a different synopsis: > https://bugs.openjdk.java.net/browse/JDK-6404550 > Cannot implement late attach in NetBeans Profiler due to missing functionality in JVMTI David H.: > I would like to see a detailed analysis of the implications of allowing this. I _think_ it is safe but ... > . . . > Digging deeper ... to fix the problem described in that bug they augmented JVM TI to allow private method redefinition as an alternate to the "native rebinding" technique that had been used previously. See the final comment in: > https://bugs.openjdk.java.net/browse/JDK-6341303 > "JVMTI Spec: Need a way how to rebind Object.wait and Thread.sleep with late attach" > which was closed as a duplicate. Serguei: > Valid concern. > Also, I'd love to collect more details on the initial motivation to relax the JVMTI spec. > Most likely we had no CCC/CSR filed on this change. > . . . > Thank you for the point. > This explains it. > It seems, the bug synopsis was changed at some moment. Dan: > The synopsis for 6404550 has never changed. Here's the subject line when it was created on 2006.03.27: > CR 6404550 *HOT* Created P1 hotspot/jvmti Cannot implement late attach in NetBeans Profiler due to missing functionality in JVMTI > I think the confusion arises over comments like this in 6341303: >> rfield Robert Field added a comment - 2006-05-04 11:54 >> BT2:EVALUATION >> This can now be accomplished with Java programming language instrumentation, via: >> 6404550: missing late attach (JVM TI redefine) functionality > > Permit the adding or deleting of private final/static methods with redefine >> Closing this bug as a duplicate. > > That's just Robert's style for an sccs delta comment: > D 1.65.2.3 06/04/25 23:36:35 rfield 140 139 00023/00013/03263 > MRs: > COMMENTS: > 6404550: missing late attach (JVM TI redefine) functionality > Add/delete private methods, continued: changes per review > > Back in the ancient past we tried to include some brief info about the change in the delta comment. > This was one of many deltas associated with 6404550. > > Please see the attached email that I sent on 2012.12.17 about the > history behind this issue... (sent to Karen, Mikael V, and Serguei) > > It seems I forwarded that same email to Coleen, Markus G and Serguei > back on 2014.05.20. Since Markus is on that thread, it must have had > something to do with research about JFR... > > I need to do a detailed read thru my e-mail archive for 6404550 to > see if I can spot some clues about why we didn't do a spec update. > > Dan Karen: > Dan, > > Thank you for all the background digging. This is really helpful. > > Serguei - do you know what tests exist for this behavior? > > The way I read the source code - we currently allow ADD and DELETE for > PRIVATE OR STATIC OR FINAL methods. Did I read that correctly? > > With the current implementation, I am not sure if deletion works for private methods - do we > have a test for that? Or could you add one as part of this exercise? > > Today we create a vtable entry for private methods (my misunderstanding ~ 2006ish). > After discussions with David I no longer believe we need those. > Today, klassVtable::adjust_method_entries has an assertion > assert(!old_method->is_deleted(), ���vtable methods may not be deleted���) > > I may have read the code incorrectly - but I would expect to hit this assertion > if you had a private method you were deleting that was not final and not static. > > option 1) we could explicitly tighten the restrictions to match what we have implemented > option 2) we could make this work by changing klassVtable.cpp::update_inherited_vtable > handling of private fields to be done the way it handles final fields. > option 3) I read the code incorrectly? > > thanks, > Karen Dan: > I mentioned a test that I found in the email that I attached to my previous reply: > >> Subject: >> Re: adding/deleting methods in JVM/TI RedefineClasses()/RetransformClasses() >> From: >> "Daniel D. Daugherty" <daniel.daugherty@oracle.com> >> Date: 12/17/12, 3:03 PM >> To: Karen Kinnear <Karen.Kinnear@Oracle.com> >> CC: Mikael Vidstedt <mikael.vidstedt@oracle.com>, >> Serguei Spitsyn <serguei.spitsyn@oracle.com> >> >> Looks like I added a JLI/JPLIS test that exercises this "feature" >> by calling the newly added method via reflection: >> >> $ ls /work/shared/bug_hunt/jdk8/exp/test/java/lang/instrument/RedefineMethodAdd* >> RedefineMethodAddInvoke.sh >> RedefineMethodAddInvokeAgent.java >> RedefineMethodAddInvokeApp.java >> RedefineMethodAddInvokeTarget.java >> RedefineMethodAddInvokeTarget_1.java >> RedefineMethodAddInvokeTarget_2.java >> >> This test was added for the following bug: >> 6667089 3/3 multiple redefinitions of a class break reflection >> >> All these little pieces of information keep coming back at me... >> >> Dan > > I haven't checked to see if that test is still there or if any additional tests have been added. > > Dan
21-02-2018

It is not at all clear how JDK-6404550 morphed into " Permit the adding or deleting of private final/static methods with redefine" - nor why those changes failed to make any change to the spec itself. It is also unclear whether the add/delete is restricted to final/static methods or any private method? I can see that the intent was to only allow something that would not perturb the vtable for existing instances.
05-12-2017

The ability to add/remove private methods was added via the following bug: JDK-6404550 Cannot implement late attach in NetBeans Profiler due to missing functionality in JVMTI 6404550 was fixed in JDK1.6-B83. Here's the relevant SCCS delta comments for: hotspot/src/share/vm/prims/jvmtiRedefineClasses.cpp: D 1.65.2.7 06/04/26 15:26:16 rfield 144 143 00010/00008/03286 MRs: COMMENTS: 6404550: missing late attach (JVM TI redefine) functionality Add/delete private methods, continued: changes per review D 1.65.2.6 06/04/26 13:59:12 rfield 143 142 00011/00003/03283 MRs: COMMENTS: 6404550: missing late attach (JVM TI redefine) functionality Add/delete private methods, continued: JLS says private is final, but VM doesn't think so -- hack: require added/deleted methods to be final or static The vtable of classes implementing redefined interfaces need update too D 1.65.2.5 06/04/26 00:31:36 rfield 142 141 00002/00002/03284 MRs: COMMENTS: 6404550: missing late attach (JVM TI redefine) functionality Add/delete private methods, continued: changes per review, yada yada D 1.65.2.4 06/04/26 00:24:24 rfield 141 140 00002/00002/03284 MRs: COMMENTS: 6404550: missing late attach (JVM TI redefine) functionality Add/delete private methods, continued: changes per review, yada yada D 1.65.2.3 06/04/25 23:36:35 rfield 140 139 00023/00013/03263 MRs: COMMENTS: 6404550: missing late attach (JVM TI redefine) functionality Add/delete private methods, continued: changes per review D 1.57.1.2 06/04/21 12:23:09 rfield 129 114 00375/00132/02143 MRs: COMMENTS: 6404550: missing late attach (JVM TI redefine) functionality Permit the adding or deleting of private methods with redefine A method now has an assigned idnum, rather than an index into the methods() array jmethodIDs are now JNI handles.
05-12-2017

See also the discussion in: http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/022020.html It was claimed there that the ability to add/remove private methods is a regression introduced by: https://bugs.openjdk.java.net/browse/JDK-8149743 but I see no bug filed for that (or at least not linked back to JDK-18149743). Note also that the spec is silent on whether inner classes can be added or removed: they are not explicitly listed either way. I consider this an oversight. Also note that for nestmates (JEP181 - JDK-8046171) we will explicitly prohibit any change to nest membership through retransformation/redefinition.
03-12-2017

Leonid, should this be labeled tck-red for 10?
02-12-2017

Leonid, is the jck label needed for this bug?
02-12-2017