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

Wed Jun 8 17:52:54 UTC 2016

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).

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 
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>