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

[coleen.phillimore at oracle.com]

On 7/8/19 12:27 PM, Erik Osterlund wrote:
> Hi Coleen,
>
> Thanks for the review. I am 100% for not using the unsafe API in shared code at all. Could we make that change for 14 though?

Oh, definitely!
Coleen
>
> Thanks,
> /Erik
>
>> On 8 Jul 2019, at 17:11, coleen.phillimore at oracle.com wrote:
>>
>>
>>
>>> 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