Hi Mark,
Thanks for the test cleanup.
In the new security policy files, is it important to enumerate the
individual permissions instead of using AllPermission?
If it is significant, the perhaps a comment is in order, otherwise,
there will be doubt about why the long list instead of the simpler
AllPermission.
ConcurrentHashMapTest, line 128/137: has some commented out code that
can be removed (or explained).
HelloServer.java: line 36: more commented out code to be removed
Since _HelloImpl_Tie.java and _HelloInterface_Stub.java are being
committed, please add the copyright header.
I didn't get the RmiIiopReturnValueTest to run both invocations; the
second one died with could not initialize class javax.rmi.CORBA.Util.
But I didn't take the time to debug it. It probably works for you.
In RmiIiopReturnValueTest, there are a few unused imports,
DataInputStream, File, TimeUnit, CountDownLatch that could be removed.
Thanks, Roger
On 6/15/2016 2:33 PM, Mark Sheppard wrote:
Hi,
please oblige and review the updated webrev:
http://cr.openjdk.java.net/~msheppar/8146975/jdk9/webrev.01/
http://cr.openjdk.java.net/~msheppar/8146975/jdk9/test/webrev.01/
based on the feedback, the following amendments were made:
the scope of the tests reduced
the removal of the export fro module-info.java -
it transpires that the granting of all permissions to the corba module
in the policy file, mitigates the
need for this addition
added check for SecurityManager and perform doPrivileged for
getDeclaredField
cleanups suggestions from Sean
regards
Mar
On 09/06/2016 23:27, Mark Sheppard wrote:
thanks Roger ... will reduce the focus of the test
regards
Mark
On 09/06/2016 15:58, Roger Riggs wrote:
Hi Mark,
With respect to the test, it seems to do a lot more than just be
the test for the current issue.
Perhaps some javadoc in the test could identify the the important
parts of the test
as opposed to the supporting infrastructure.
If there is more code than is needed for this issue then perhaps the
test should be whittled down
to just what is needed for this issue or should be promoted to a
more general test with a regular name
(not in a subdirectory with the bug number). I think this has been
our direction anyway, to
write tests for the functionality and not only for a specific issue.
Thanks, Roger
On 6/9/2016 10:18 AM, Roger Riggs wrote:
Hi Mark,
IIOPInputStream.java:
- Does adding doPriv overhead to every field access have a
noticeable impact on performance?
If most of the fields are accessible or can easily be checked as
being in the base module,
I could see trying the access directly and catching the security
exception to retry with doPriv.
Roger
ps.
There is a newer version of webrev that provides convenient next
and previous links.
http://hg.openjdk.java.net/code-tools/webrev/raw-file/d26c194772db/webrev.ksh
On 6/9/2016 8:57 AM, Seán Coffey wrote:
Hey Mark,
Looks fine. A few comments.
2291 Class<?> declaredFieldClass = declaredClassField.getType();
2292
2293 if (declaredFieldClass == null) {
2294 continue;
2295 }
I'm not sure if this check is required. Can getType() return null ?
==
2833 Field classField;
2834 classField = c
2835 .getDeclaredField(fieldName);
2836 return classField;
Could we fold that code down to 'return
c.getDeclaredField(fieldName);' ?
For the tests :
==
ConcurrentHashMapTest.java :
you might want to remove the old rmiServerProcess.destroy(),
orbdProcess.destroyForcibly() comments.
==
HelloClient.java
line 157 : s/1000/ONE_SECOND
The use of static ports could be one to watch. If we encounter
port issues, the test might have to be revisited.
Regards,
Sean.
On 08/06/2016 23:18, Mark Sheppard wrote:
Hi
please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8146975/jdk9/webrev/
http://cr.openjdk.java.net/~msheppar/8146975/jdk9/test/webrev/
which address the issue raised in
https://bugs.openjdk.java.net/browse/JDK-8146975
the type checking in inputClassFields and other places failed to
fully allowing for
the processing of return ValueTypes, and hence the
getDeclaredField fails as
"application code" exist on the call stack restricting access.
This leads to a security exception,
which in turn leads to an IllegalArgumentExcetption, the
processing of which failed to allow
for a null object value in the stream.
This has now been rectified, with the getDeclaredField wrapped in
a doPrivileged call.
regards
Mark
