ART: More lenient lock merging in the verifier
Be more lenient on mismatched lock levels when merging register
lines in the verifier. Registers may be aliases, in which case
it is fine to drop the lock information, as long as at least one
alias survives to keep the data alive.
Bug: 23502994
Change-Id: I0844115480286f5e6ab5ba71dced571e50dd42a5
diff --git a/runtime/verifier/register_line.cc b/runtime/verifier/register_line.cc
index 33c90e3..eade787 100644
--- a/runtime/verifier/register_line.cc
+++ b/runtime/verifier/register_line.cc
@@ -391,6 +391,34 @@
}
}
+// Check whether there is another register in the search map that is locked the same way as the
+// register in the src map. This establishes an alias.
+static bool FindLockAliasedRegister(
+ uint32_t src,
+ const AllocationTrackingSafeMap<uint32_t, uint32_t, kAllocatorTagVerifier>& src_map,
+ const AllocationTrackingSafeMap<uint32_t, uint32_t, kAllocatorTagVerifier>& search_map) {
+ auto it = src_map.find(src);
+ if (it == src_map.end()) {
+ // "Not locked" is trivially aliased.
+ return true;
+ }
+ uint32_t src_lock_levels = it->second;
+ if (src_lock_levels == 0) {
+ // "Not locked" is trivially aliased.
+ return true;
+ }
+
+ // Scan the map for the same value.
+ for (const std::pair<uint32_t, uint32_t>& pair : search_map) {
+ if (pair.first != src && pair.second == src_lock_levels) {
+ return true;
+ }
+ }
+
+ // Nothing found, no alias.
+ return false;
+}
+
bool RegisterLine::MergeRegisters(MethodVerifier* verifier, const RegisterLine* incoming_line) {
bool changed = false;
DCHECK(incoming_line != nullptr);
@@ -417,9 +445,29 @@
size_t depths = reg_to_lock_depths_.count(idx);
size_t incoming_depths = incoming_line->reg_to_lock_depths_.count(idx);
if (depths != incoming_depths) {
- if (depths == 0 || incoming_depths == 0) {
- reg_to_lock_depths_.erase(idx);
- } else {
+ // Stack levels aren't matching. This is potentially bad, as we don't do a
+ // flow-sensitive analysis.
+ // However, this could be an alias of something locked in one path, and the alias was
+ // destroyed in another path. It is fine to drop this as long as there's another alias
+ // for the lock around. The last vanishing alias will then report that things would be
+ // left unlocked. We need to check for aliases for both lock levels.
+ //
+ // Example (lock status in curly braces as pair of register and lock leels):
+ //
+ // lock v1 {v1=1}
+ // / \
+ // v0 = v1 {v0=1, v1=1} v0 = v2 {v1=1}
+ // \ /
+ // {v1=1}
+ // // Dropping v0, as the status can't be merged
+ // // but the lock info ("locked at depth 1" and)
+ // // "not locked at all") is available.
+ if (!FindLockAliasedRegister(idx,
+ reg_to_lock_depths_,
+ reg_to_lock_depths_) ||
+ !FindLockAliasedRegister(idx,
+ incoming_line->reg_to_lock_depths_,
+ reg_to_lock_depths_)) {
verifier->Fail(VERIFY_ERROR_LOCKING);
if (kDumpLockFailures) {
LOG(WARNING) << "mismatched stack depths for register v" << idx
@@ -429,20 +477,51 @@
}
break;
}
+ // We found aliases, set this to zero.
+ reg_to_lock_depths_.erase(idx);
} else if (depths > 0) {
// Check whether they're actually the same levels.
uint32_t locked_levels = reg_to_lock_depths_.find(idx)->second;
uint32_t incoming_locked_levels = incoming_line->reg_to_lock_depths_.find(idx)->second;
if (locked_levels != incoming_locked_levels) {
- verifier->Fail(VERIFY_ERROR_LOCKING);
- if (kDumpLockFailures) {
- LOG(WARNING) << "mismatched lock levels for register v" << idx << ": "
- << std::hex << locked_levels << std::dec << " != "
- << std::hex << incoming_locked_levels << std::dec << " in "
- << PrettyMethod(verifier->GetMethodReference().dex_method_index,
- *verifier->GetMethodReference().dex_file);
+ // Lock levels aren't matching. This is potentially bad, as we don't do a
+ // flow-sensitive analysis.
+ // However, this could be an alias of something locked in one path, and the alias was
+ // destroyed in another path. It is fine to drop this as long as there's another alias
+ // for the lock around. The last vanishing alias will then report that things would be
+ // left unlocked. We need to check for aliases for both lock levels.
+ //
+ // Example (lock status in curly braces as pair of register and lock leels):
+ //
+ // lock v1 {v1=1}
+ // lock v2 {v1=1, v2=2}
+ // / \
+ // v0 = v1 {v0=1, v1=1, v2=2} v0 = v2 {v0=2, v1=1, v2=2}
+ // \ /
+ // {v1=1, v2=2}
+ // // Dropping v0, as the status can't be
+ // // merged but the lock info ("locked at
+ // // depth 1" and "locked at depth 2") is
+ // // available.
+ if (!FindLockAliasedRegister(idx,
+ reg_to_lock_depths_,
+ reg_to_lock_depths_) ||
+ !FindLockAliasedRegister(idx,
+ incoming_line->reg_to_lock_depths_,
+ reg_to_lock_depths_)) {
+ // No aliases for both current and incoming, we'll lose information.
+ verifier->Fail(VERIFY_ERROR_LOCKING);
+ if (kDumpLockFailures) {
+ LOG(WARNING) << "mismatched lock levels for register v" << idx << ": "
+ << std::hex << locked_levels << std::dec << " != "
+ << std::hex << incoming_locked_levels << std::dec << " in "
+ << PrettyMethod(verifier->GetMethodReference().dex_method_index,
+ *verifier->GetMethodReference().dex_file);
+ }
+ break;
}
- break;
+ // We found aliases, set this to zero.
+ reg_to_lock_depths_.erase(idx);
}
}
}
diff --git a/test/088-monitor-verification/src/TwoPath.java b/test/088-monitor-verification/src/TwoPath.java
index 2542de7..bdc15ad 100644
--- a/test/088-monitor-verification/src/TwoPath.java
+++ b/test/088-monitor-verification/src/TwoPath.java
@@ -31,6 +31,8 @@
* Conditionally uses one of the synchronized objects.
*/
public static void twoPath(Object obj1, Object obj2, int x) {
+ Main.assertIsManaged();
+
Object localObj;
synchronized (obj1) {
