RFR: JDK-8146975 - NullPointerException in IIOPInputStream.inputClassFields

[Roger Riggs]

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
>>>>>
>>>>
>>>
>>
>