RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails to throw IllegalAccessException for final fields
Adam Farley8
adam.farley at uk.ibm.com
Tue Mar 26 17:24:13 UTC 2019
Hiya Mandy,
I've updated the webrev with your proposal, plus a few tweaks for
formatting.
http://cr.openjdk.java.net/~afarley/8216558.2/webrev
Let me know what you think.
Best Regards
Adam Farley
IBM Runtimes
Mandy Chung <mandy.chung at oracle.com> wrote on 26/03/2019 03:51:29:
> From: Mandy Chung <mandy.chung at oracle.com>
> To: Adam Farley8 <adam.farley at uk.ibm.com>, Joe Darcy
<joe.darcy at oracle.com>
> Cc: core-libs-dev <core-libs-dev at openjdk.java.net>
> Date: 26/03/2019 03:51
> Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails
> to throw IllegalAccessException for final fields
>
>
> On 3/25/19 11:36 AM, Adam Farley8 wrote:
> Addendum: URL for new webrev: http://cr.openjdk.java.net/~afarley/
> 8216558.1/webrev/
>
> Thanks for sending a versioned webrev.
> > What is a USA test?
>
> UNREFLECT_SETTER_ACCESSIBLE.
>
> I was trying to be brief, and I lost readability.
>
> Will re-upload with the expanded name.
>
> >
> > Why the assertion becomes conditional?
>
> Because we only need this check if the test is USA.
>
> Previously, setting a variable as accessible gave us a setter
> without errors 100% of the time.
>
> The test expects this, and anticipates an empty list of variables
> which threw errors when passed to unreflectSetter.
>
> Since we now throw an exception when calling the unreflectSetter
> method, even when the variable is accessible, this check ensures
> that the only variables throwing an exception are the static final
fields.
>
> >
> > Have you considered another way doing it?
>
> Yep, several. This seemed the simplest.
>
> Other options I considered were:
>
> --------------
>
> 1) Changing the test to store a list of variables, rather than just
> their names, so the "isAccessible" method could be used instead.
>
> Discarded because it makes the array compare slower for every test.
>
> 2) Editing the array of anticipated excludeds during test execution,
> to remove the exceptions we don't expect to see.
>
> Discarded because the change seemed too bulky to accommodate a single
test.
>
> 3) Make the "isAccessible" methods smarter, and rely on them
> exclusively to determine the list of anticipated exceptions.
>
> Ditto
>
> ---------------------
>
> Option 3 could work. I prefer the one-liner stream check.
>
> Thoughts?
>
>
> Each test case has a set of inaccessible fields under normal access
checks.
> For MH_UNREFLECT_GETTER_ACCESSIBLE and MH_UNREFLECT_SETTER_ACCESSIBLE
> cases, before the patch, all fields are accessible as
setAccessible(true)
> is called on all fields. That's the special casing for these cases and
> expect actualFieldNames is empty.
>
> This patch now changes the set of inaccessible fields for
unreflectSetter
> (static final fields are inaccessible). A cleaner way to fix this is
> to filter a given set of inaccessible fields and return the set of
> inaccessible fields applicable to a FieldLookup case.
>
> Attached is my suggested patch. It adds a new
FieldLookup::inaccessibeFields
> method and the XXX_ACCESSIBLE Lookup overrides it to do the filtering.
>
> Mandy
>
>
> diff --git a/test/jdk/java/lang/invoke/VarHandles/accessibility/
> TestFieldLookupAccessibility.java b/test/jdk/java/lang/invoke/
> VarHandles/accessibility/TestFieldLookupAccessibility.java
> --- a/test/jdk/java/lang/invoke/VarHandles/accessibility/
> TestFieldLookupAccessibility.java
> +++ b/test/jdk/java/lang/invoke/VarHandles/accessibility/
> TestFieldLookupAccessibility.java
> @@ -22,7 +22,7 @@
> */
>
> /* @test
> - * @bug 8152645
> + * @bug 8152645 8216558
> * @summary test field lookup accessibility of MethodHandles and
VarHandles
> * @compile TestFieldLookupAccessibility.java
> * pkg/A.java pkg/B_extends_A.java pkg/C.java
> @@ -96,6 +96,12 @@
> Object lookup(MethodHandles.Lookup l, Field f) throws
Exception {
> return l.unreflectGetter(cloneAndSetAccessible(f));
> }
> +
> + // Setting the accessibility bit of a Field grants access
under
> + // all conditions for MethodHandler getters
> + Set<String> inaccessibleFields(Set<String>
inaccessibleFields) {
> + return new HashSet<>();
> + }
> },
> MH_UNREFLECT_SETTER() {
> Object lookup(MethodHandles.Lookup l, Field f) throws
Exception {
> @@ -103,13 +109,25 @@
> }
>
> boolean isAccessible(Field f) {
> - return f.isAccessible() || !Modifier.isFinal
> (f.getModifiers());
> + return f.isAccessible() && !Modifier.isStatic
> (f.getModifiers()) || !Modifier.isFinal(f.getModifiers());
> }
> },
> MH_UNREFLECT_SETTER_ACCESSIBLE() {
> Object lookup(MethodHandles.Lookup l, Field f) throws
Exception {
> return l.unreflectSetter(cloneAndSetAccessible(f));
> }
> +
> + boolean isAccessible(Field f) {
> + return !(Modifier.isStatic(f.getModifiers()) &&
> Modifier.isFinal(f.getModifiers()));
> + }
> +
> + // Setting the accessibility bit of a Field grants
> access to non-static
> + // final fields for MethodHandler setters
> + Set<String> inaccessibleFields(Set<String>
inaccessibleFields) {
> + Set<String> result = new HashSet<>();
> + inaccessibleFields.stream().filter(f -> f.contains
> ("_static_")).forEach(result::add);
> + return result;
> + }
> },
> VH() {
> Object lookup(MethodHandles.Lookup l, Field f) throws
Exception {
> @@ -142,6 +160,10 @@
> return true;
> }
>
> + Set<String> inaccessibleFields(Set<String> inaccessibleFields)
{
> + return new HashSet<>(inaccessibleFields);
> + }
> +
> static Field cloneAndSetAccessible(Field f) throws Exception {
> // Clone to avoid mutating source field
> f = f.getDeclaringClass().getDeclaredField(f.getName());
> @@ -180,7 +202,7 @@
> @Test(dataProvider = "lookupProvider")
> public void test(FieldLookup fl, Class<?> src,
> MethodHandles.Lookup l, Set<String> inaccessibleFields) {
> // Add to the expected failures all inaccessible fields due
> to accessibility modifiers
> - Set<String> expected = new HashSet<>(inaccessibleFields);
> + Set<String> expected =
fl.inaccessibleFields(inaccessibleFields);
> Map<Field, Throwable> actual = new HashMap<>();
>
> for (Field f : fields(src)) {
> @@ -203,12 +225,9 @@
> if (!actualFieldNames.equals(expected)) {
> if (actualFieldNames.isEmpty()) {
> // Setting the accessibility bit of a Field grants
> access under
> - // all conditions for MethodHander getters and setters
> - if (fl != FieldLookup.MH_UNREFLECT_GETTER_ACCESSIBLE &&
> - fl != FieldLookup.MH_UNREFLECT_SETTER_ACCESSIBLE) {
> + // all conditions for MethodHandler getters
> Assert.assertEquals(actualFieldNames, expected,
> "No accessibility failures:");
> }
> - }
> else {
> Assert.assertEquals(actualFieldNames, expected,
> "Accessibility failures differ:");
> }
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number
741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
More information about the core-libs-dev
mailing list