RFR: JDK-8146975 - NullPointerException in IIOPInputStream.inputClassFields

Wed Jun 15 23:43:15 UTC 2016

thanks Roger ... I'll attend to them

regards
Mark

On 15/06/2016 21:57, Roger Riggs wrote:
>
> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>