<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<tt>Hi Vladimir,<br>
<br>
As long as the owner of the test review the patch and confirm the<br>
test policy intends to extend the default policy, those test
change<br>
will be fine.<br>
<br>
test/jdk/java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java</tt><br>
<pre><span class="changed"> 417 DEFAULT_POLICY.implies(domain, permission)) return true;
Nit: separating this to a new if-case after 426 to make it distinct.</span>
</pre>
<tt>I reviewed</tt><tt> test/jdk/java/lang/Class/getDeclaredField/*
and</tt><tt><br>
</tt><tt>test/jdk/java/lang/invoke/* patch and they look fine.<br>
<br>
My suggestion is to minimize the risk of your patch. Since Daniel<br>
has reviewed the logging test change (which is the bulk of this
patch),<br>
I have no issue if the reviewers approve this patch. I will file
<br>
a separate RFE for the test library idea.<br>
<br>
Mandy<br>
<br>
</tt><br>
<div class="moz-cite-prefix">On 6/20/19 11:05 AM, Vladimir Kozlov
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:53c2c755-572f-c76e-5872-6bd6ed8914b8@oracle.com">Hi
Mandy
<br>
<br>
I am not sure about this suggestion. Graal module may not be
present in the JDK (on SPARC for example).
<br>
And I don't want pollute general tests with Graal specific code:
ModulePolicy.get("jdk.internal.vm.compiler").
<br>
<br>
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.
<br>
<br>
I found only 2 closed tests in which I had doubt about using
default policy. I kept them on problem list.
<br>
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.
<br>
<br>
Thanks,
<br>
Vladimir
<br>
<br>
On 6/19/19 11:23 PM, Mandy Chung wrote:
<br>
<blockquote type="cite">Hi Vladimir,
<br>
<br>
Indeed these are test issues that the tests will need to grant
permissions
<br>
to jdk.internal.vm.compiler as the default policy grants.
<br>
<br>
Thanks for going extra miles to fix the tests.
<br>
<br>
My suggestion may be a bit general. What I intend to say the
custom
<br>
security policy should extend the default policy unless it
intentionally
<br>
excludes configuring permissions for specific modules. I review
the
<br>
the patch but the test doesn't clearly tell what the test
intends to
<br>
do w.r.t. security configuration.
<br>
<br>
We should avoid inadvertently granting permissions that the test
expects
<br>
to disallow. A better solution is to limit granting permissions
just for
<br>
`jdk.internal.vm.compiler` module rather than all.
<br>
<br>
Attached is ModulePolicy class that allows you to get the Policy
for
<br>
a specific module. It can be put in the test library that these
tests
<br>
can use them.
<br>
<br>
So the test can call
ModulePolicy.get("jdk.internal.vm.compiler") and
<br>
implies method will call the returned ModulePolicy if present.
<br>
<br>
test/lib/jdk/test/lib/security is one existing testlibrary for
security
<br>
related stuff. You can consider putting ModulePolicy.java
there.
<br>
<br>
This is one idea.
<br>
<br>
Mandy
<br>
<br>
On 6/19/19 6:03 PM, Vladimir Kozlov wrote:
<br>
<blockquote type="cite"><a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kvn/8185139/webrev.00/">http://cr.openjdk.java.net/~kvn/8185139/webrev.00/</a>
<br>
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8185139">https://bugs.openjdk.java.net/browse/JDK-8185139</a>
<br>
<br>
For Graal to work we have to give Graal module all permissions
which is specified in default policy [1].
<br>
Unfortunately this cause problem for Graal running tests which
overwrite default policy.
<br>
<br>
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].
<br>
<br>
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.
<br>
<br>
I ran testing which execute these tests with Graal. It shows
only known problems which are not related to these changes.
<br>
<br>
Thanks,
<br>
Vladimir
<br>
<br>
[1]
<a class="moz-txt-link-freetext" href="http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156">http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156</a><br>
[2] <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8189291">https://bugs.openjdk.java.net/browse/JDK-8189291</a>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>