RFR: 8073688: Infinite loop reading types during jmap attach.

[Kevin Walls]

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)


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.

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