RFR: 8313202: MutexLocker should disallow null Mutexes [v5]

[Daniel D. Daugherty]

On Thu, 24 Aug 2023 07:55:01 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> As seen in [JDK-8313081](https://bugs.openjdk.org/browse/JDK-8313081), it is fairly easy to pass nullptr `Mutex` to `MutexLocker` by accident, which would just silently avoid the lock.
>> 
>> There are a few places in Hotspot where we pass `nullptr` to simulate re-entrancy and/or conditionally take the lock. Those places can be more explicit, and the default `MutexLocker` can disallow nullptrs for extra safety.
>> 
>> More thorough testing with different GC/JIT combinations is running now, we might find more issues there. Meanwhile, please comment on the approach.
>> 
>> Additional testing:
>>  - [x] `grep -R "MutexLocker " src/hotspot | grep -i null`, only new `ConditionalMutexLocker` hits
>>  - [x] `grep -R "MutexLocker " src/hotspot | grep -i ?`, no hits
>>  - [x] Linux AArch64 fastdebug, `tier1 tier2 tier3` x `Serial Parallel G1 Shenandoah`
>
> Aleksey Shipilev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8313202-mutexlocker-nulls
>  - Merge branch 'master' into JDK-8313202-mutexlocker-nulls
>  - Accept one more potentially nullptr mutex
>  - Merge branch 'master' into JDK-8313202-mutexlocker-nulls
>  - Replace ReentrantMutexLocker with ConditionalMutexLocker
>  - Workaround for JDK-8313210
>  - Fixing CodeCache analytics
>  - Initial work

Thumbs up, but I do have some questions about some of the
new ConditionalMutexLocker uses.

src/hotspot/share/classfile/classLoader.cpp line 941:

> 939: 
> 940: void ClassLoader::release_load_zip_library() {
> 941:   ConditionalMutexLocker locker(Zip_lock, Zip_lock != nullptr, Monitor::_no_safepoint_check_flag);

Why is this one now `ConditionalMutexLocker`?

src/hotspot/share/code/stubs.cpp line 241:

> 239: 
> 240: void StubQueue::print() {
> 241:   ConditionalMutexLocker lock(_mutex, _mutex != nullptr, Mutex::_no_safepoint_check_flag);

Why is this one now a `ConditionalMutexLocker`?

src/hotspot/share/runtime/mutexLocker.hpp line 274:

> 272:  public:
> 273:   MonitorLocker(Monitor* monitor, Mutex::SafepointCheckFlag flag = Mutex::_safepoint_check_flag) :
> 274:     MutexLocker(monitor, flag), _flag(flag) {}

The assert will now be: `"null mutex is not allowed"`
instead of `"null monitor not allowed"`. Not really a
problem, just trying to make it clear.

src/hotspot/share/runtime/mutexLocker.hpp line 277:

> 275: 
> 276:   MonitorLocker(Thread* thread, Monitor* monitor, Mutex::SafepointCheckFlag flag = Mutex::_safepoint_check_flag) :
> 277:     MutexLocker(thread, monitor, flag), _flag(flag) {}

The assert will now be: `"null mutex is not allowed"`
instead of `"null monitor not allowed"`. Not really a
problem, just trying to make it clear.

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

Marked as reviewed by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15043#pullrequestreview-1608528700
PR Review Comment: https://git.openjdk.org/jdk/pull/15043#discussion_r1314262278
PR Review Comment: https://git.openjdk.org/jdk/pull/15043#discussion_r1314262163
PR Review Comment: https://git.openjdk.org/jdk/pull/15043#discussion_r1314260745
PR Review Comment: https://git.openjdk.org/jdk/pull/15043#discussion_r1314260860