[9] RFR (S): 8157181: Compilers accept modification of final fields outside initializer methods

Fri Jun 10 07:30:07 UTC 2016

On 10/06/2016 4:38 PM, Zoltán Majó wrote:
> Hi David,
>
>
> On 06/10/2016 02:59 AM, David Holmes wrote:
>> Hi Zoltan,
>>
>> On 9/06/2016 3:52 AM, Zoltán Majó wrote:
>>> Hi David,
>>>
>>>
>>>
>>> On 06/02/2016 01:51 PM, David Holmes wrote:
>>>> [...]
>>>> The simplest, and common, approach to these kind of issues is to only
>>>> apply the correction for the current class file version and keep the
>>>> relaxed rules for all others. That provides the best compatibility
>>>> story. Of course if security is a concern then that is a different
>>>> matter - but such concerns can not be discussed here.
>>>
>>> thank you for the suggestion!
>>>
>>> In the latest webrev (webrev.06), I enable the additional check for
>>> classfile version >= 51 (JAVA_7_VERSION). With that version, we're
>>> conforming with the specification. So far no problems have showed up
>>> with our test base. The new check can be disabled with the
>>> CheckFinalFieldModifications flag (you've suggested that in your JBS
>>> comment).
>>>
>>> We can also change the checked version to 52 (JAVA_8) or 53 (JAVA_9).
>>
>> I think this needs to go to CCC so that the compatibility issue can be
>> examined. Normally, after long periods of non-conformance, we don't
>> "back date" tighter checks - so my initial feeling is that this should
>> only be enforced for Java 9 classfiles onwards. But that is not a
>> decision for individual engineers to make.
>
> Thank you for the feedback!
>
> I have changed the checked version to JAVA_9 (as you initially
> suggested). Here is the updated webrev:
>
> http://cr.openjdk.java.net/~zmajo/8157181/webrev.08/
>
> I hope we don't need CCC approval if we apply the correction for the
> current class version only.
>
> I can file a new bug to investigate if the checks can be tightened up to
> pre-JAVA_9 -- that could form the discussion with the CCC.

That sounds fine - thanks.

Note I have not reviewed the actual code.

Cheers,
David

> Thank you!
>
> Best regards,
>
>
> Zoltan
>
>>
>>
>> Thanks,
>> David
>>
>>> Please see the updated webrev (webrev.06) in my reply to Vladimir K.
>>>
>>> Thank you!
>>>
>>> Best regards,
>>>
>>>
>>> Zoltan
>>>
>>>>
>>>> David
>>>> -----
>>>>
>>>>> Before JVMS-7, where putfield/putstatic linkage checks were tightened,
>>>>> it was allowed to change final fields from anywhere in the same class.
>>>>>
>>>>> putfield "Linking Exceptions":
>>>>>
>>>>>   JVMS-6 [1]:
>>>>>     "Otherwise, if the field is final, it must be declared in the
>>>>> current class. Otherwise, an IllegalAccessError is thrown."
>>>>>
>>>>>   JVMS-7 [2]:
>>>>>     "Otherwise, if the field is final, it must be declared in the
>>>>> current class, and the instruction must occur in an instance
>>>>> initialization method (<init>) of the current class. Otherwise, an
>>>>> IllegalAccessError is thrown."
>>>>>
>>>>>
>>>>> src/share/vm/interpreter/linkResolver.cpp:
>>>>>
>>>>> +         (byte == Bytecodes::_putstatic && fd.is_static() &&
>>>>> method_name != vmSymbols::class_initializer_name()) ||
>>>>> +         ((byte == Bytecodes::_putfield || byte ==
>>>>> Bytecodes::_nofast_putfield) && !fd.is_static() && method_name !=
>>>>> vmSymbols::object_initializer_name())
>>>>>
>>>>>
>>>>> Also, as we found earlier, checking method name is not enough:
>>>>>
>>>>> bool Method::is_static_initializer() const {
>>>>>   // For classfiles version 51 or greater, ensure that the clinit
>>>>> method is
>>>>>   // static.  Non-static methods with the name "<clinit>" are not
>>>>> static
>>>>>   // initializers. (older classfiles exempted for backward
>>>>> compatibility)
>>>>>   return name() == vmSymbols::class_initializer_name() &&
>>>>>          has_valid_initializer_flags();
>>>>> }
>>>>>
>>>>>
>>>>> -  LinkInfo link_info(defc, name, type, KlassHandle(),
>>>>> /*check_access=*/false);
>>>>> +  LinkInfo link_info(defc, name, type, KlassHandle(), NULL,
>>>>> /*check_access=*/false);
>>>>>
>>>>> Use Handle() instead of NULL.
>>>>>
>>>>> Also, I'd prefer to see LinkInfo::_current_method and new constructor
>>>>> added:
>>>>>
>>>>>   LinkInfo(KlassHandle resolved_klass, Symbol* name, Symbol*
>>>>> signature,
>>>>>            Handle current_method, bool check_access = true) :
>>>>>
>>>>> _current_klass can be derived from _current_method, since:
>>>>>
>>>>> //   current_klass  = sending method holder (i.e., class containing
>>>>> the
>>>>> method
>>>>> //                    containing the call being resolved)
>>>>>
>>>>>> I did the following testing:
>>>>>> - JPRT
>>>>>> - pre-PIT RBT run (in progress)
>>>>>> - the reproducer.
>>>>>>
>>>>>> The reproducer now behaves as expected (an IllegalAccessError is
>>>>>> thrown). Also, the pre-PIT RBT run has shown no new failures so far.
>>>>>>
>>>>>> But there is a problem with JPRT: A JCK test, putstatic01801m, fails.
>>>>>>
>>>>>> The reason for the failure is that the the test generates a method
>>>>>> that
>>>>>> contains a putstatic to a static final field (i.e., the bytecodes
>>>>>> generated by the test do not conform to the JVMS). (Even the test
>>>>>> mentions that the Java source equivalent to the generated
>>>>>> bytecodes is
>>>>>> compilable only if the "final" keyword is removed from the
>>>>>> declaration
>>>>>> of the static field.)
>>>>>>
>>>>>> So (if we decide to push the current fix) the issue with the test
>>>>>> needs
>>>>>> to be fixed first. Do you know how we could proceed with that?
>>>>> File a bug against JCK.
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>> [1]
>>>>> https://docs.oracle.com/javase/specs/jvms/se6/html/Instructions2.doc11.html
>>>>>
>>>>>
>>>>>
>>>>> [2]
>>>>> https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-6.html#jvms-6.5.putfield
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> [3]
>>>>> https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.4.3.2
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Thank you and best regards,
>>>>>>
>>>>>>
>>>>>> Zoltan
>>>>>>
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>>
>>>>>>>>
>>>>>>>> I agree with you, alternatively we can propose a more generic fix
>>>>>>>> (fix
>>>>>>>> the interpreter and the compilers as well). The fix would most
>>>>>>>> likely
>>>>>>>> affect field resolution in LinkResolver::resolve_field() around
>>>>>>>> here:
>>>>>>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/interpreter/linkResolver.cpp#l910
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Changing resolve_field() to throw an IllegalAccessError for
>>>>>>>> inappropriate field access will propagate the exception to the
>>>>>>>> interpreter. Changing ciField::will_link() will most likely kill
>>>>>>>> (some)
>>>>>>>> compilations (if, e.g., fields are linked through
>>>>>>>> ciField::will_link()).
>>>>>>>>
>>>>>>>> I'd prefer to take the second option (changing field
>>>>>>>> resolution), but
>>>>>>>> there might be some disadvantages related to option I might be
>>>>>>>> overseeing.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Thank you!
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>>
>>>>>>>>
>>>>>>>> Zoltan
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> More details about the failure are here [3].
>>>>>>>>>>
>>>>>>>>>> With the patch applied, the program always completes as it is
>>>>>>>>>> expected
>>>>>>>>>> by Jython, no matter if we use -Xint, -Xcomp, or -Xmixed
>>>>>>>>>> (because we
>>>>>>>>>> always interpret methods non-conforming with the VM spec).
>>>>>>>>>>
>>>>>>>>>> Here is the updated webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~zmajo/8157181/webrev.01/
>>>>>>>>> Bailing out the whole compilation in C2 is even less
>>>>>>>>> appropriate. It
>>>>>>>>> should generate an uncommon trap for such accesses instead (see
>>>>>>>>> Parse::do_field_access).
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>> Vladimir Ivanov
>>>>>>>>>
>>>>>>>>> [1]
>>>>>>>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/c1/c1_GraphBuilder.cpp#l1595
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>
>