RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails to throw IllegalAccessException for final fields

[David Holmes]

On 12/01/2019 10:13 am, Mandy Chung wrote:
> On 1/11/19 3:03 PM, David Holmes wrote:
>> On 12/01/2019 8:47 am, Mandy Chung wrote:
>>> On 1/11/19 2:38 PM, David Holmes wrote:
>>>> There seem to be a number of spec issues around this. Shouldn't 
>>>> findStaticSetter say something about what happens when the field is 
>>>> final? Same for findSetter? This issue seems to be much bigger than 
>>>> just a simple bug fix.
>>>>
>>>
>>> I don't see any issue with the spec for findSetter and 
>>> findStaticSetter.  findSetter returns a method handle equivalent to 
>>> `putField` bytecode and it throws IAE when access checking is 
>>> performed since it's final and not writeable.  Similiarly for 
>>> findStaticSetter.
>>
>> Well that is what it does, but it doesn't explicitly mention final 
>> fields and the @throws information for IAE:
>>
>>     IllegalAccessException - if access checking fails, or if the field 
>> is static
>>
>> doesn't convey anything about getting IAE for a final field. Perhaps 
>> it is implicit in the "this behaves like bytecode" but it is very 
>> obscure.
>>
> 
> I assume you agree that there is no spec issue w.r.t. findSetter or 
> findStaticSetter while you may find it obscure.

I think the implicit reliance on bytecode behaviour is a problem when it 
involves final fields - see below.

>>>> A CSR request will need to be filed.
>>>>
>>>
>>> Of course, as this is a spec change.
>>
>> I'm unclear what spec is actually to be changed here and in what way? 
> 
> I expect Adam will send a revised webrev to include the proposed spec 
> change.
>> If these methods produce MH that obey bytecode semantics then the fact 
>> the Field had setAccessible(true) called on it is not relevant - the 
>> JVMS knows nothing about setAccessible. The only issue I see is the 
>> implementation not throwing IAE when the Field represents a final 
>> field, because it examines the "accessibility" state. (And that should 
>> apply whether the field is static or not.)
> 
> This is the current implementation matching the specification:
>      If the field's accessible flag is not set, access checking is 
> performed immediately on behalf of the lookup class.
> 
> I think it's intended to match Field::setAccessible behavior but 
> Field::setAccessible has no effect in suppressing the language access 
> check in static final field.  Hence I agree with Adam's fix changes that 
> the access check should be performed for static final field even its 
> accessible flag is set.

I think there is a problem knowing when "access check" means just access 
check and when it means "access check plus the special hack for setting 
final fields". I'm not reading any final field semantics into the 
existing text, because bytecode does not know about setAccessible and 
final fields. I can see an argument to be made for the case where 
setAccessible(true) has been called to disable language level access 
checks, and you want the MH to follow the same access rules. But that 
does not to me imply that the ability to write to a final field should 
be part of that. If that is intended then it needs to be very, very 
clearly spelt out.

And as per my other email there is already a problem with the bytecode 
equivalence of MH semantics and final fields, because you can set final 
fields in <init> or <clinit> (though only once). I don't think MH 
captures that - nor can it. Which means we can't defer to bytecode 
semantics here but rather have to very clearly define how MH interact 
with final fields.

David

> Mandy