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

[Mandy Chung]

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.

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

Mandy