[14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal

[Vladimir Kozlov]

Thank you, Mandy

On 6/20/19 6:20 PM, Mandy Chung wrote:
> Hi Vladimir,
> 
> As long as the owner of the test review the patch and confirm the
> test policy intends to extend the default policy, those test change
> will be fine.
> 
> test/jdk/java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java
> 
> 417 DEFAULT_POLICY.implies(domain, permission)) return true; Nit: separating this to a new if-case after 426 to make it 
> distinct.

I did as you suggested:

@@ -420,6 +422,7 @@
                      return true;
                  }
              }
+            if (DEFAULT_POLICY.implies(domain, permission)) return true;
              return false;
          }

> 
> I reviewed test/jdk/java/lang/Class/getDeclaredField/* and
> test/jdk/java/lang/invoke/* patch and they look fine.

Thanks.

> 
> My suggestion is to minimize the risk of your patch.  Since Daniel
> has reviewed the logging test change (which is the bulk of this patch),
> I have no issue if the reviewers approve this patch.  I will file
> a separate RFE for the test library idea.

I will be pushing my changes since I don't see objections in reviews.

Main discussion is how make these tests more robust which could be done separately.

Thanks,
Vladimir

> 
> Mandy
> 
> 
> On 6/20/19 11:05 AM, Vladimir Kozlov wrote:
>> Hi Mandy
>>
>> I am not sure about this suggestion. Graal module may not be present in the JDK (on SPARC for example).
>> And I don't want pollute general tests with Graal specific code: ModulePolicy.get("jdk.internal.vm.compiler").
>>
>> If you or other have concerns about some tests looking on default policy I can keep them on problem list to run them 
>> later only with libgraal.
>>
>> I found only 2 closed tests in which I had doubt about using default policy. I kept them on problem list.
>> Other tests, as I understand, manipulate permissions for test classes. They don't extend system classes so default 
>> policy should not affect test class permissions.
>>
>> Thanks,
>> Vladimir
>>
>> On 6/19/19 11:23 PM, Mandy Chung wrote:
>>> Hi Vladimir,
>>>
>>> Indeed these are test issues that the tests will need to grant permissions
>>> to jdk.internal.vm.compiler as the default policy grants.
>>>
>>> Thanks for going extra miles to fix the tests.
>>>
>>> My suggestion may be a bit general.  What I intend to say the custom
>>> security policy should extend the default policy unless it intentionally
>>> excludes configuring permissions for specific modules.  I review the
>>> the patch but the test doesn't clearly tell what the test intends to
>>> do w.r.t. security configuration.
>>>
>>> We should avoid inadvertently granting permissions that the test expects
>>> to disallow.  A better solution is to limit granting permissions just for
>>> `jdk.internal.vm.compiler` module rather than all.
>>>
>>> Attached is ModulePolicy class that allows you to get the Policy for
>>> a specific module. It can be put in the test library that these tests
>>> can use them.
>>>
>>> So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and
>>> implies method will call the returned ModulePolicy if present.
>>>
>>> test/lib/jdk/test/lib/security is one existing testlibrary for security
>>> related stuff.  You can consider putting ModulePolicy.java there.
>>>
>>> This is one idea.
>>>
>>> Mandy
>>>
>>> On 6/19/19 6:03 PM, Vladimir Kozlov wrote:
>>>> http://cr.openjdk.java.net/~kvn/8185139/webrev.00/
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8185139
>>>>
>>>> For Graal to work we have to give Graal module all permissions which is specified in default policy [1].
>>>> Unfortunately this cause problem for Graal running tests which overwrite default policy.
>>>>
>>>> I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her 
>>>> suggestion. Note, this is not only Graal problem. There were already similar fixes before [2].
>>>>
>>>> I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they 
>>>> would not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We 
>>>> will enable such tests again when libgraal is supported.
>>>>
>>>> I ran testing which execute these tests with Graal. It shows only known problems which are not related to these 
>>>> changes.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> [1] http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156
>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8189291
>>>
>