RFR (8u): 8073688: Infinite loop reading types during jmap attach.

[Staffan Larsen]

Ok with me.

Thanks,
/Staffan

> On 4 mar 2015, at 18:41, Kevin Walls <kevin.walls at oracle.com> wrote:
> 
> 
> Hi,
> 
> While this is in memory: I'd like to request review for an 8u backport.
> 
> The changeset imports cleanly into jdk8u/hs-dev/hotspot
> 
> Thanks
> Kevin
> 
> 
> On 04/03/2015 15:17, Staffan Larsen wrote:
>> Looks good!
>> 
>> Thanks,
>> /Staffan
>> 
>>> On 4 mar 2015, at 15:14, Kevin Walls <kevin.walls at oracle.com> wrote:
>>> 
>>> 
>>> Sure, I updated the webrev:
>>> 
>>> http://cr.openjdk.java.net/~kevinw/8073688/webrev.01/
>>> 
>>> Thanks
>>> Kevin
>>> 
>>> 
>>> On 04/03/2015 14:05, Staffan Larsen wrote:
>>>>> On 4 mar 2015, at 14:07, Kevin Walls <kevin.walls at oracle.com> wrote:
>>>>> 
>>>>> 
>>>>> Hi Staffan --
>>>>> 
>>>>> staticness: I could have argued that either way, it doesn't need to be static.  A static count of how many HotSpotTypeDataBase duplicate type errors we have seen, or one count per HotSpotTypeDataBase?  Any doubt I will make it per HotSpotTypeDatabase.  As I sometimes load multiple cores in kjdb, or load them multiple times, I should be encouraging less staticness in the SA overall: so yes let me remove that static! (updated webrev in same location)
>>>> Thanks.
>>>> 
>>>>> Well spotted on the structs loop: 8-)
>>>>> readVMTypes, readVMntConstants, readVMLongConstants may increase, or call something could increase, the counter.
>>>>> readVMStructs does not.
>>>>> 
>>>>> These are four methods that are so similar you'd imagine they should get merged into one method one day.  It seemed wrong to me to make the four methods yet more different, by taking out the check on duplicateDefCount for one of them.
>>>>> 
>>>>> Also, maybe readVMStructs should be checking for duplicates anyway, and maybe one day it will.  I'm guessing it doesn't because it was harder in that method compared to the others. 8-)
>>>>> 
>>>>> So I put it there for regularity, but if you think it's misleading I'll take it out.
>>>> Yes, please take it out.
>>>> 
>>>> Thanks,
>>>> /Staffan
>>>> 
>>>>> In practice, as long as we have the duplicate check in readVMTypes(),  we have the protection we need as this is called first.  It is highly unlikely that would succeed and another of the methods would fail, the other checks are just making the code consistent.
>>>>> 
>>>>> Thanks
>>>>> Kevin
>>>>> 
>>>>> 
>>>>> 
>>>>> On 04/03/2015 12:29, Staffan Larsen wrote:
>>>>>>> On 4 mar 2015, at 12:29, Kevin Walls <kevin.walls at oracle.com> wrote:
>>>>>>> 
>>>>>>> Thanks Dmitry -
>>>>>>> 
>>>>>>> I will explain the counter's presence where we introduce it:
>>>>>>> 
>>>>>>>  // Counter to ensure read loops terminate:
>>>>>>>  private static final int MAX_DUPLICATE_DEFINITIONS = 100;
>>>>>>>  private static int duplicateDefCount = 0;
>>>>>> Should duplicateDefCount really be static?
>>>>>> 
>>>>>> For the use of duplicateDefCount on line 477 (readVMStructs()) I do not see where it is increased inside the loop? Can you clarify?
>>>>>> 
>>>>>> /Staffan
>>>>>> 
>>>>>>> Thanks
>>>>>>> Kevin
>>>>>>> 
>>>>>>> 
>>>>>>> On 03/03/2015 20:10, Dmitry Samersoff wrote:
>>>>>>>> Kevin,
>>>>>>>> 
>>>>>>>> The fix looks good for me, but please add a comment explaining the
>>>>>>>> problem we are solving here.
>>>>>>>> 
>>>>>>>> -Dmitry
>>>>>>>> 
>>>>>>>> On 2015-03-03 16:15, Kevin Walls wrote:
>>>>>>>>> Hi,
>>>>>>>>> 
>>>>>>>>> This is a review request for a way to make the SA tools protect
>>>>>>>>> themselves from infinite loops during initialisation.
>>>>>>>>> 
>>>>>>>>> Attaching jmap (for example) to a JVM can fail, infinitely writing an
>>>>>>>>> error - and filling a disk if being logged to a file.  This reproduces
>>>>>>>>> on a Solaris package based install, but not with other distribution
>>>>>>>>> bundles.  In those packages, there's a link JDK/jre/lib/sparc/libjvm ->
>>>>>>>>> client/libjvm.so.  If a server/libjvm.so is loaded and running, we see
>>>>>>>>> libverify.so pull in client/libjvm.so, as it finds the symlink in its
>>>>>>>>> $ORIGIN, in preference to finding the already loaded libjvm.so.
>>>>>>>>> 
>>>>>>>>> Symbol lookup in the SA is fooled by having multiple libjvm.so loaded.
>>>>>>>>> There are various things wrong with that.  Protection against zero
>>>>>>>>> strides through the type arrays and a maximum count for duplicated types
>>>>>>>>> will protect us from a few possible problems.
>>>>>>>>> 
>>>>>>>>> I'll also work a bug for the "install" repo where we create that
>>>>>>>>> symlink, but the tools need to protect themselves from this kind of
>>>>>>>>> problem.
>>>>>>>>> 
>>>>>>>>> Testing was manual.
>>>>>>>>> 
>>>>>>>>> bug
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8073688
>>>>>>>>> 
>>>>>>>>> webrev
>>>>>>>>> http://cr.openjdk.java.net/~kevinw/8073688/webrev.00/
>>>>>>>>> 
>>>>>>>>> Thanks
>>>>>>>>> Kevin
>