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

[Erik Österlund]

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.

Thanks,
/Erik

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