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

[David Holmes]

Hi John,

On 15/01/2019 11:47 am, John Rose wrote:
> On Jan 11, 2019, at 4:53 PM, David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
>>
>> 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.
> 
> The "bytecode behavior" of an unreflectSetter MH is clearly documented
> by appealing to a call to Field::set.  That is a pre-existing specification
> that proves that the observed behavior for statics is a bug, and for
> non-statics is a feature.  No CSR is needed, and, David, you are simply
> looking in the wrong places.  I admit that the right place is tricky to 
> find.

Thanks John - yes I missed that and thought it appealed to direct 
bytecode (putField/putStatic etc).

Cheers,
David
-----

> 
>> 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.
> 
> Yes, the documentation should be improved, so that email threads like
> this won't be necessary to tease out the meaning of the spec.  I placed
> a proposal for *clarifying* (not *changing*) it in the bug comments.
> 
>> 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.
> 
> That's true.  The most it can do is capture the effect, if any, of
> setAcc(true) on a reflected Field.
> 
>> Which means we can't defer to bytecode semantics here but rather have 
>> to very clearly define how MH interact with final fields.
> 
> And that's already done, except for the "clearly" part.
> 
> Thanks for raising this issue!
> 
> — John
> 
> diff --git 
> a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java 
> b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
> --- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
> +++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
> @@ -421,6 +421,10 @@
>        * because the desired class member is missing, or because the
>        * desired class member is not accessible to the lookup class, or
>        * because the lookup object is not trusted enough to access the 
> member.
> +     * In the case of a field setter function on a {@code final} field,
> +     * finality enforcement is treated as a kind of access control,
> +     * and the lookup will fail, except in special cases of
> +     * {@link Lookup#unreflectSetter Lookup.unreflectSetter}.
>        * In any of these cases, a {@code ReflectiveOperationException} 
> will be
>        * thrown from the attempted lookup.  The exact class will be one of
>        * the following:
> @@ -1436,6 +1440,7 @@
>            * @return a method handle which can store values into the field
>            * @throws NoSuchFieldException if the field does not exist
>            * @throws IllegalAccessException if access checking fails, or 
> if the field is {@code static}
> +         *                                or {@code final}
>            * @exception SecurityException if a security manager is 
> present and it
>            *                              <a 
> href="MethodHandles.Lookup.html#secmgr">refuses access</a>
>            * @throws NullPointerException if any argument is null
> @@ -1559,6 +1564,7 @@
>            * @return a method handle which can store values into the field
>            * @throws NoSuchFieldException if the field does not exist
>            * @throws IllegalAccessException if access checking fails, or 
> if the field is not {@code static}
> +         *                                or is {@code final}
>            * @exception SecurityException if a security manager is 
> present and it
>            *                              <a 
> href="MethodHandles.Lookup.html#secmgr">refuses access</a>
>            * @throws NullPointerException if any argument is null
> @@ -1868,19 +1874,29 @@
>           /**
>            * Produces a method handle giving write access to a reflected 
> field.
>            * The type of the method handle will have a void return type.
> -         * If the field is static, the method handle will take a single
> +         * If the field is {@code static}, the method handle will take 
> a single
>            * argument, of the field's value type, the value to be stored.
>            * Otherwise, the two arguments will be the instance containing
>            * the field, and the value to be stored.
> -         * If the field's {@code accessible} flag is not set,
> +         * If the {@code Field} object's {@code accessible} flag is not 
> set,
>            * access checking is performed immediately on behalf of the 
> lookup class.
>            * <p>
> -         * If the field is static, and
> +         * If the field is {@code final}, write access will not be
> +         * allowed and access checking will fail, except under certain
> +         * narrow circumstances documented for {@link Field#set Field.set}.
> +         * A method handle is returned only if a corresponding call to
> +         * the {@code Field} object's {@code set} method could return
> +         * normally.  In particular, fields which are both {@code static}
> +         * and {@code final} may never be set.
> +         * <p>
> +         * If the field is {@code static}, and
>            * if the returned method handle is invoked, the field's class 
> will
>            * be initialized, if it has not already been initialized.
>            * @param f the reflected field
>            * @return a method handle which can store values into the 
> reflected field
> -         * @throws IllegalAccessException if access checking fails
> +         * @throws IllegalAccessException if access checking fails,
> +         *         or if the field is {@code final} and write access
> +         *         is not enabled on the {@code Field} object
>            * @throws NullPointerException if the argument is null
>            */
>           public MethodHandle unreflectSetter(Field f) throws 
> IllegalAccessException {
>