RFR[13]: 8227277: HeapInspection::find_instances_at_safepoint walks dead objects

[coleen.phillimore at oracle.com]

On 7/5/19 11:26 AM, Erik Österlund wrote:
>
>
> On 2019-07-05 13:35, David Holmes wrote:
>> Hi Erik,
>>
>> On 5/07/2019 8:19 pm, Erik Österlund wrote:
>>> Hi,
>>>
>>> In the HeapInspection::find_instances_at_safepoint function, the 
>>> unsafe heap iteration API (which also walks dead objects) is used to 
>>> find objects that are instance of a class, used for concurrent lock 
>>> dumping where we find dead 
>>> java.util.concurrent.locks.AbstractOwnableSynchronizer objects and 
>>> pointer chase to its possibly dead owner threadObj. There is a 
>>> comment saying that if this starts crashing because we use CMS, we 
>>> should probably change to use the safe_object_iterate() API instead, 
>>> which does not include dead objects.
>>>
>>> Arguably, whether CMS is observed to crash or not, we really should 
>>> not be walking over dead objects and exposing them anyway. It's not 
>>> safe... and it will crash sooner or later.
>>>
>>> For example, CMS yields to safepoints (including young GCs) while 
>>> sweeping. This means that both the AbstractOwnableSynchronizer and 
>>> its owner thread might have died, but while sweeping, we could yield 
>>> for a young GC that promotes objects overriding the memory of the 
>>> dead thread object with random primitives, but not yet freeing the 
>>> dead AbstractOwnableSynchronizer. A subsequent dumping operation 
>>> could use the heap walker to find the dead 
>>> AbstractOwnableSynchronizer, and pointer chase into its dead owner 
>>> thread, which by now has been freed and had its memory clobbered 
>>> with primitive data.
>>>
>>> This will all eventually end up in a glorious crash. So we shouldn't 
>>> do this.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8227277
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8227277/webrev.00/
>>
>> That seems eminently reasonable. :)
>
> Thanks!
>
>> Are there any valid uses for the (unsafe) object_iterate?
>
> Well... valid might be an overstatement, but I think it probably won't 
> crash if you don't pointer chase through dead references in dead 
> objects. We simply can't do that.

This change looks good, but I have to echo David's question.  It looks 
like we have the same thing in jvmtiTagMap, with some out of date comments.

hare/prims/jvmtiTagMap.cpp:    // consider using safe_object_iterate() 
which avoids perm gen
share/prims/jvmtiTagMap.cpp: Universe::heap()->object_iterate(_blk);
share/prims/jvmtiTagMap.cpp: Universe::heap()->object_iterate(&blk);

Should we eliminate all uses of Universe::heap() version of 
object_iterate?  It looks like the GCs call it, and it's probably safe 
in those places, so should not be a virtual function for each GC?

Thanks,
Coleen

>
> Thanks,
> /Erik
>
>> Cheers,
>> David
>>
>>> Thanks,
>>> /Erik
>