RFR: JDK-8146975 - NullPointerException in IIOPInputStream.inputClassFields

[Mark Sheppard]

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