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

[Adam Farley8]

Hi Mandy,

Apologies for the delay.

Could you review this cdiff as a proposal for the jtreg test?

Made sense to modify the existing test set for MethodHandle rather than 
add a new one.

------------------------------------------------

test/jdk/java/lang/invoke/MethodHandlesGeneralTest.java

*** 830,844 ****
--- 830,860 ----
      public void testUnreflectSetter() throws Throwable {
 CodeCacheOverflowProcessor.runMHTest(this::testUnreflectSetter0);
      }
 
      public void testUnreflectSetter0() throws Throwable {
+         //First we test the newer "static final field" behaviour.
+         testUnreflectSetterStaticFinalField();
+         //Now we test the more proven functionality.
          if (CAN_SKIP_WORKING)  return;
          startTest("unreflectSetter");
          testSetter(TEST_UNREFLECT);
      }
 
+     static final int staticFinalField = 0;
+     public void testUnreflectSetterStaticFinalField() throws Throwable {
+         try {
+             Lookup l = MethodHandles.lookup();
+             Field f = 
MethodHandlesGeneralTest.class.getDeclaredField("staticFinalField");
+             f.setAccessible(true);
+             MethodHandle s = l.unreflectSetter(f);
+             throw new 
RuntimeException("MethodHandle.Lookup.unreflectSetter(Field) failed to 
throw IAE when Field is static and final.");
+         } catch (IllegalAccessException e) {
+             // IllegalAccessException expected.
+         }
+     }
+ 
      @Test
      public void testFindSetter() throws Throwable {
          CodeCacheOverflowProcessor.runMHTest(this::testFindSetter0);
      }
 


------------------------------------------------

Best Regards

Adam Farley 
IBM Runtimes


Mandy Chung <mandy.chung at oracle.com> wrote on 31/01/2019 18:58:25:

> From: Mandy Chung <mandy.chung at oracle.com>
> To: Adam Farley8 <adam.farley at uk.ibm.com>
> Cc: core-libs-dev <core-libs-dev at openjdk.java.net>
> Date: 31/01/2019 18:58
> Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails 
> to throw IllegalAccessException for final fields
> 
> 
> 
> On 1/31/19 4:18 AM, Adam Farley8 wrote:
> > Hi Mandy,
> > 
> > I've made the changes to the webrev and uploaded the corrected 
version.
> > 
> > I've also uploaded the zips in the csr and bug.
> > 
> > As for the test, I have mentioned that the test is attached to the 
bug. 
> > I attached the same test to the CSR.
> 
> I did look at that but it's not a jtreg regression test.  I assume
> you planned to convert it to jtreg test.  Anyway, the test attached
> in the bug report is the reproducer of the bug.
> 
> It would help the reviewer to include the regression test in
> the webrev along with the fix unless a test is not necessary.
> The OpenJDK developer guide gives a pretty good overview on the
> process [1] and please do ask if you have any question.
> 
> https://urldefense.proofpoint.com/v2/url?
> u=http-3A__openjdk.java.net_guide_changePlanning.html-23bug&d=DwIC-
> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=uZYDsYv1Pe21YaYk_fuEwKg7Cc5a9sdsiGMPjDqgT5k&s=mvs_zaoLuTOUkmdByobfKcpvovlTEiHtNcz-
> UMc3d70&e=
> 
> > Are you saying that the test supplied is not a suitable test?
> 
> TestStaticFinal.java is a standalone test.  It needs to convert
> to a jtreg regression test.
> 
> The regression should throw an exception if Lookup::unreflectSetter
> returns MH on a static final field with and without accessible flag
> set.  However TestStaticFinal.java also exits with 0.  The regression
> test should pass with your bug and fail without the fix (throwing
> an exception).
> 
> This test verifies unreflectSetter on a static final field.  I
> suggest it also includes a case with instance final field with
> and without accessible flag set to show the exception for
> unreflectSetter what case allows setting final fields.
> 
> Mandy
> [1] https://urldefense.proofpoint.com/v2/url?
> u=http-3A__openjdk.java.net_guide_changePlanning.html-23bug&d=DwIC-
> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=uZYDsYv1Pe21YaYk_fuEwKg7Cc5a9sdsiGMPjDqgT5k&s=mvs_zaoLuTOUkmdByobfKcpvovlTEiHtNcz-
> UMc3d70&e=
>      P.S. should be updated to refer to CSR that replaced CCC.
> 

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