Bug in monitor locking/unlocking on ARM32 C1 due to uninitialized BasicObjectLock::_displaced_header
Chris Cole
chris at sageembedded.com
Wed May 12 17:02:32 UTC 2021
Hi,
Not sure if this is the appropriate place or method to report an OpenJDK
bug, if not please advise.
I have discovered a bug with ARM32 C1 monitor locking/unlocking that can
result in deadlock. The bug was introduced with JCK-8241234 "Unify monitor
enter/exit runtime entries" [1]. This change introduced a call to
ObjectSynchronizer::quick_entry() within the logic for
Runtime1::monitorenter().
If the monitor is inflated and already owned by the current thread, then
ObjectSynchronizer::quick_entry() simply increments
ObjectMonitor::_recursions and returns. In this case
Runtime1::monitorenter() returns to the JIT compiled code without calling
"lock->set_displaced_header(markWord::unused_mark())" (see [2]). For ARM32
the _displaced_header field is not always initialized in JIT code before
calling Runtime1::monitorenter() helper. If the uninitialized value of
_displaced_header field on stack happens to be NULL, this causes an issue
because the JIT code to exit the monitor first checks for a NULL
_displaced_header as an indication for non-inflated recursive locking which
is a noop for exiting the monitor (see [3]). This means that the
Runtime1::monitorexit() helper is not called as required to exit this
inflated monitor, and the ObjectMonitor::_recursions is not decremented as
required. This leads to thread not unlocking the monitor when required and
deadlock when another thread tries to lock the monitor.
This bug is not present on AArch64 and x86, because the displaced header is
initialized in JIT code with the "unlocked object header" value (which is
non-zero) before calling Runtime1::monitorenter() helper (see [4] and [5]).
Note sure about other CPU architectures.
I see two ways to fix this.
1) In ObjectSynchronizer::quick_entry() move the
"lock->set_displaced_header(markWord::unused_mark())" statement to before
the "if (owner == current)" at line 340 in share/runtime/synchronizer.cpp
(see [6]), so that Runtime1::monitorenter() helper logic always initializes
the displaced header field as was the case before JCK-8241234.
2) For ARM32 add JIT code to initialize the displaced header field before
calling Runtime1::monitorenter() helper as done for AArch64 and x86.
Not sure which is better (or maybe both are required for some reason I am
not aware of). I believe this "displacted header" on the stack can be
looked at by stack walkers but I am not familiar with the exact details and
if there are implications on this fix.
The bug is also present in OpenJDK 11.0.10 and later (introduced by the
backport of JDK-8241234 [1]).
I/my company (Sage Embedded Software) has signed the Oracle Contributor
Agreement (OCA) and have been granted access to JCK.
The bug can be reproduced in my environment with the OpenJDK Community TCK
testing of java.io.PipedReader that deadlocks, but because reproduction of
the issue requires uninitialized stack field to be zero, it might not
happen in some environments. I have a Java test case that can reproduce
this issue on ARM in 32 bit mode. It is pasted inline below at the end of
the email. There is a "getZeroOnStack()" method that I think helps get a
zero into the uninitialized _displaced_header field. The test case source
code is copied from OpenJDK java.io.PipedReader source code and then
modified. It needs to be run with only C1 enabled (I am using minimal
variant to enforce this) and the following command line options
(-XX:-BackgroundCompilation -XX:CompileThreshold=500
-XX:CompileOnly="com.sageembedded.test.MonitorBugTest::receive"). The test
case should run and then end with "Source thread done" and "Reading
complete" output if the bug is not reproduced. If the monitor bug is
reproduced the test case will not exit and the main thread will be
deadlocked, with the main thread last printing "read() before wait" and
missing "read() after wait" and "Reading complete". If useful I can provide
the output of this test cause including -XX:PrintAssebly and logging that I
added to ObjectSynchronizer::quick_entry() that shows uninitialized
lock->_displaced_header and ObjectMonitor->_recursions continue to get
incremented (into the 1000s) as the MonitorBugTest.receive() method is
called in a loop.
Please let me know if there is anything else that would be helpful. I hope
to become active in the OpenJDK Community. My time is a little limited at
the moment, so sometimes it might take a day to respond (I have 3 and 6
year old kids). In the coming years I expect to have additional time to be
more involved in the OpenJDK Community.
Best regards,
Chris Cole
Sage Embedded Software LLC
[1] https://bugs.openjdk.java.net/browse/JDK-8241234
[2]
https://github.com/openjdk/jdk/blob/dfe8833f5d9a9ac59857143a86d07f85769b8eae/src/hotspot/share/runtime/synchronizer.cpp#L343
[3]
https://github.com/openjdk/jdk/blob/dfe8833f5d9a9ac59857143a86d07f85769b8eae/src/hotspot/cpu/x86/c1_MacroAssembler_x86.cpp#L130
[4]
https://github.com/openjdk/jdk/blob/71b8ad45b4de6836e3bb2716ebf136f3f8ea2198/src/hotspot/cpu/aarch64/c1_MacroAssembler_aarch64.cpp#L95
[5]
https://github.com/openjdk/jdk/blob/dfe8833f5d9a9ac59857143a86d07f85769b8eae/src/hotspot/cpu/x86/c1_MacroAssembler_x86.cpp#L74
[6]
https://github.com/openjdk/jdk/blob/dfe8833f5d9a9ac59857143a86d07f85769b8eae/src/hotspot/share/runtime/synchronizer.cpp#L340
/*
* Copyright (c) 1996, 2020, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package com.sageembedded.test;
import java.io.IOException;
import java.io.InterruptedIOException;
/*
* java -cp bin -minimal -XX:-BackgroundCompilation -XX:CompileThreshold=500
* -XX:CompileOnly="com.sageembedded.test.MonitorBugTest::receive"
* -XX:+PrintCompilation -XX:+UnlockDiagnosticVMOptions -XX:+PrintAssembly
* com.sageembedded.test.MonitorBugTest
*/
public class MonitorBugTest {
private static int DATA_SIZE = 1000;
private static int BUFFER_SIZE = 256;
private char buffer[] = new char[BUFFER_SIZE];
private int writeIndex = -1;
private int readIndex = 0;
public Object lock = new Object();
public static void main(String[] args) {
MonitorBugTest test = new MonitorBugTest();
test.run();
}
private void run() {
System.out.println("Starting test");
SourceThread source = new SourceThread();
source.start();
try {
for (int i = 0; i < DATA_SIZE; i++) {
read();
}
} catch (IOException e) {
e.printStackTrace();
}
System.out.println("Reading complete");
}
synchronized void receive(char data[], int offset, int length)
throws IOException {
while (--length >= 0) {
getZeroOnStack(offset);
receive(data[offset++]);
}
}
private void getZeroOnStack(int offset) {
int l1;
int l2;
int l3;
int l4;
int l5;
int l6;
int l7;
int l8;
int l9;
int l10;
int l11;
int l12;
int l13;
int l14;
int l15;
int l16;
l1 = 0;
l2 = 0;
l3 = 0;
l4 = 0;
l5 = 0;
l6 = 0;
l7 = 0;
l8 = 0;
l9 = 0;
l10 = 0;
l11 = 0;
l12 = 0;
l13 = 0;
l14 = 0;
l15 = 0;
l16 = 0;
}
synchronized void receive(int c) throws IOException {
while (writeIndex == readIndex) {
notifyAll();
try {
wait(1000);
} catch (InterruptedException e) {
throw new InterruptedIOException();
}
}
if (writeIndex < 0) {
writeIndex = 0;
readIndex = 0;
}
buffer[writeIndex++] = (char) c;
if (writeIndex >= buffer.length) {
writeIndex = 0;
}
}
synchronized void last() {
notifyAll();
}
public synchronized int read() throws IOException {
while (writeIndex < 0) {
notifyAll();
try {
System.out.println("read() before wait");
wait(1000);
System.out.println("read() after wait");
} catch (InterruptedException e) {
throw new InterruptedIOException();
}
}
int value = buffer[readIndex++];
if (readIndex >= buffer.length) {
readIndex = 0;
}
if (writeIndex == readIndex) {
writeIndex = -1;
}
return value;
}
private class SourceThread extends Thread {
@Override
public void run() {
System.out.println("Source thread start");
char data[] = new char[DATA_SIZE];
try {
receive(data, 0, data.length);
last();
} catch (IOException e) {
e.printStackTrace();
}
System.out.println("Source thread done");
}
}
}
More information about the hotspot-dev
mailing list