RFR: JDK-8146975 - NullPointerException in IIOPInputStream.inputClassFields

[Mark Sheppard]

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