RFR: 8314225: SIGSEGV in JavaThread::is_lock_owned [v6]

[Kevin Walls]

On Fri, 3 May 2024 02:01:51 GMT, Dean Long <dlong at openjdk.org> wrote:

>> Kevin Walls has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   monitor->owner() == nullptr handling in fill_in
>
> src/hotspot/share/runtime/synchronizer.cpp line 1060:
> 
>> 1058:       // the ObjectMonitor.
>> 1059:     } else if (LockingMode == LM_LEGACY && mark.has_locker()
>> 1060:                && JavaThread::cast(current)->is_lock_owned((address)mark.locker())) {
> 
> This looks risky.  How about guarding it with a check for current->is_Java_thread()?

Yes!

> src/hotspot/share/runtime/vframeArray.cpp line 94:
> 
>> 92:         assert(!monitor->owner_is_scalar_replaced() || realloc_failures, "object should be reallocated already");
>> 93:         BasicObjectLock* dest = _monitors->at(index);
>> 94:         if (monitor->owner_is_scalar_replaced() || monitor->owner() == nullptr) {
> 
> The only way to get a null owner is if owner_is_scalar_replaced() is true:
> https://github.com/openjdk/jdk/blob/6bef0474c8b8773d0d20c0f25c36a2ce9cdbd7e8/src/hotspot/share/runtime/stackValue.hpp#L52
> and to get this far with it still null means `realloc_failures` is true.  We could avoid a null check later in unpack_on_stack if we skip adding to _monitors in this case.  So maybe use a GrowableArray inside MonitorChunk and add elements using append().
> Suggestion:
> 
>         if (monitor->owner_is_scalar_replaced()) {

Yes, no nullptr check needed there.
As discussed, moving to a GrowableArray complicates things quite a lot.  fill_in and unpack_on stack both make a similar iteration, but with slightly different information available.  Growing the array on demand and making both loops variable involves more changes in MonitorChunk and its accessors.  I think we're agreed to not be that disruptive for this change.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18940#discussion_r1593640314
PR Review Comment: https://git.openjdk.org/jdk/pull/18940#discussion_r1593639706