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

[David Holmes]

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.

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

David
-----

> Mandy
> 
>> David
>> -----
>>
>> On 12/01/2019 5:07 am, Mandy Chung wrote:
>>> This issue requires a spec change and it's a behavioral change. 
>>> Field::set on a static final field throws IAE.  Although there are 
>>> few cases in the world that hack around it, I think the compatibility 
>>> risk is low in this change.
>>>
>>> I added a suggested fix in the JBS issue.
>>>
>>> Mandy
>>>
>>> On 1/11/19 10:07 AM, Mandy Chung wrote:
>>>> Hi Adam,
>>>>
>>>> This is indeed a bug.    I suggest to do the check in the 
>>>> checkAccess method closes to where it performs the instance final 
>>>> field check. Please also add a test case.
>>>>
>>>> Mandy
>>>>
>>>> On 1/11/19 6:38 AM, Adam Farley8 wrote:
>>>>> Hi All,
>>>>>
>>>>> First; thanks for responding so quickly. :)
>>>>>
>>>>> Second, it sounds like changing the contents of a final field is
>>>>> acceptable under the spec, but *not* changing the contents of a static
>>>>> final field.
>>>>>
>>>>> The test case uploaded to the bug uses a static final field for the 
>>>>> test,
>>>>> so I think we can agree that the ability to create a setter for a 
>>>>> static
>>>>> final field is still a bug.
>>>>>
>>>>> It sounds like the same fix could work if we check for "static" 
>>>>> when we
>>>>> check for "final", with a modified message.
>>>>>
>>>>> As Field.set (post-setAccessible) fails to set the value of a 
>>>>> static final
>>>>> field, I think we can agree that unreflectSetter should fail to 
>>>>> create a
>>>>> setter MethodHandle for this same field.
>>>>>
>>>>> Best Regards
>>>>>
>>>>> Adam Farley
>>>>> IBM Runtimes
>>>>>
>>>
>