Avoid abort in malformed dex code.
Don't allow a perceived double monitor-enter on a register
to abort libartd.
Allow expected verifier errors in the smali tests.
Tidy includes in the method verifier.
Bug: 17978759
Change-Id: Ic44924c788cd2334f91a047fb41b459b89a1843b
diff --git a/runtime/verifier/method_verifier.h b/runtime/verifier/method_verifier.h
index 357acf0..0c4bf3c 100644
--- a/runtime/verifier/method_verifier.h
+++ b/runtime/verifier/method_verifier.h
@@ -18,32 +18,26 @@
#define ART_RUNTIME_VERIFIER_METHOD_VERIFIER_H_
#include <memory>
-#include <set>
#include <vector>
-#include "base/casts.h"
#include "base/macros.h"
-#include "base/stl_util.h"
-#include "class_reference.h"
#include "dex_file.h"
-#include "dex_instruction.h"
#include "handle.h"
#include "instruction_flags.h"
#include "method_reference.h"
-#include "reg_type.h"
#include "reg_type_cache.h"
-#include "register_line.h"
-#include "safe_map.h"
namespace art {
+class Instruction;
struct ReferenceMap2Visitor;
-template<class T> class Handle;
namespace verifier {
-class MethodVerifier;
class DexPcToReferenceMap;
+class MethodVerifier;
+class RegisterLine;
+class RegType;
/*
* "Direct" and "virtual" methods are stored independently. The type of call used to invoke the
@@ -128,6 +122,8 @@
private:
std::unique_ptr<RegisterLine*[]> register_lines_;
size_t size_;
+
+ DISALLOW_COPY_AND_ASSIGN(PcToRegisterLineTable);
};
// The verifier
@@ -733,6 +729,8 @@
// even though we might detect to be a compiler. Should only be set when running
// VerifyMethodAndDump.
const bool verify_to_dump_;
+
+ DISALLOW_COPY_AND_ASSIGN(MethodVerifier);
};
std::ostream& operator<<(std::ostream& os, const MethodVerifier::FailureKind& rhs);
diff --git a/runtime/verifier/reg_type.h b/runtime/verifier/reg_type.h
index 34d6caa..05958b5 100644
--- a/runtime/verifier/reg_type.h
+++ b/runtime/verifier/reg_type.h
@@ -17,17 +17,14 @@
#ifndef ART_RUNTIME_VERIFIER_REG_TYPE_H_
#define ART_RUNTIME_VERIFIER_REG_TYPE_H_
-#include <limits>
#include <stdint.h>
+#include <limits>
#include <set>
#include <string>
-#include "jni.h"
-
#include "base/macros.h"
#include "base/mutex.h"
#include "gc_root.h"
-#include "globals.h"
#include "object_callbacks.h"
#include "primitive.h"
@@ -35,6 +32,7 @@
namespace mirror {
class Class;
} // namespace mirror
+
namespace verifier {
class RegTypeCache;
@@ -578,17 +576,17 @@
bool IsConstantChar() const OVERRIDE {
return IsConstant() && ConstantValue() >= 0 &&
- ConstantValue() <= std::numeric_limits<jchar>::max();
+ ConstantValue() <= std::numeric_limits<uint16_t>::max();
}
bool IsConstantByte() const OVERRIDE {
return IsConstant() &&
- ConstantValue() >= std::numeric_limits<jbyte>::min() &&
- ConstantValue() <= std::numeric_limits<jbyte>::max();
+ ConstantValue() >= std::numeric_limits<int8_t>::min() &&
+ ConstantValue() <= std::numeric_limits<int8_t>::max();
}
bool IsConstantShort() const OVERRIDE {
return IsConstant() &&
- ConstantValue() >= std::numeric_limits<jshort>::min() &&
- ConstantValue() <= std::numeric_limits<jshort>::max();
+ ConstantValue() >= std::numeric_limits<int16_t>::min() &&
+ ConstantValue() <= std::numeric_limits<int16_t>::max();
}
virtual bool IsConstantTypes() const OVERRIDE { return true; }
diff --git a/runtime/verifier/register_line-inl.h b/runtime/verifier/register_line-inl.h
index 219e687..244deed 100644
--- a/runtime/verifier/register_line-inl.h
+++ b/runtime/verifier/register_line-inl.h
@@ -114,6 +114,17 @@
}
}
+inline size_t RegisterLine::GetMaxNonZeroReferenceReg(MethodVerifier* verifier,
+ size_t max_ref_reg) const {
+ size_t i = static_cast<int>(max_ref_reg) < 0 ? 0 : max_ref_reg;
+ for (; i < num_regs_; i++) {
+ if (GetRegisterType(verifier, i).IsNonZeroReferenceTypes()) {
+ max_ref_reg = i;
+ }
+ }
+ return max_ref_reg;
+}
+
inline bool RegisterLine::VerifyRegisterType(MethodVerifier* verifier, uint32_t vsrc,
const RegType& check_type) {
// Verify the src register type against the check type refining the type of the register
diff --git a/runtime/verifier/register_line.cc b/runtime/verifier/register_line.cc
index 3139204..72d7938 100644
--- a/runtime/verifier/register_line.cc
+++ b/runtime/verifier/register_line.cc
@@ -310,8 +310,12 @@
verifier->Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "monitor-enter stack overflow: "
<< monitors_.size();
} else {
- SetRegToLockDepth(reg_idx, monitors_.size());
- monitors_.push_back(insn_idx);
+ if (SetRegToLockDepth(reg_idx, monitors_.size())) {
+ monitors_.push_back(insn_idx);
+ } else {
+ verifier->Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "unexpected monitor-enter on register v" <<
+ reg_idx;
+ }
}
}
diff --git a/runtime/verifier/register_line.h b/runtime/verifier/register_line.h
index 8f7823a..52b5c13 100644
--- a/runtime/verifier/register_line.h
+++ b/runtime/verifier/register_line.h
@@ -20,14 +20,16 @@
#include <memory>
#include <vector>
-#include "dex_instruction.h"
-#include "reg_type.h"
#include "safe_map.h"
namespace art {
+
+class Instruction;
+
namespace verifier {
class MethodVerifier;
+class RegType;
/*
* Register type categories, for type checking.
@@ -275,15 +277,7 @@
bool MergeRegisters(MethodVerifier* verifier, const RegisterLine* incoming_line)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
- size_t GetMaxNonZeroReferenceReg(MethodVerifier* verifier, size_t max_ref_reg) {
- size_t i = static_cast<int>(max_ref_reg) < 0 ? 0 : max_ref_reg;
- for (; i < num_regs_; i++) {
- if (GetRegisterType(verifier, i).IsNonZeroReferenceTypes()) {
- max_ref_reg = i;
- }
- }
- return max_ref_reg;
- }
+ size_t GetMaxNonZeroReferenceReg(MethodVerifier* verifier, size_t max_ref_reg) const;
// Write a bit at each register location that holds a reference.
void WriteReferenceBitMap(MethodVerifier* verifier, std::vector<uint8_t>* data, size_t max_bytes);
@@ -313,15 +307,18 @@
}
}
- void SetRegToLockDepth(size_t reg, size_t depth) {
+ bool SetRegToLockDepth(size_t reg, size_t depth) {
CHECK_LT(depth, 32u);
- DCHECK(!IsSetLockDepth(reg, depth));
+ if (IsSetLockDepth(reg, depth)) {
+ return false; // Register already holds lock so locking twice is erroneous.
+ }
auto it = reg_to_lock_depths_.find(reg);
if (it == reg_to_lock_depths_.end()) {
reg_to_lock_depths_.Put(reg, 1 << depth);
} else {
it->second |= (1 << depth);
}
+ return true;
}
void ClearRegToLockDepth(size_t reg, size_t depth) {
@@ -347,21 +344,23 @@
SetResultTypeToUnknown(verifier);
}
- // Storage for the result register's type, valid after an invocation
+ // Storage for the result register's type, valid after an invocation.
uint16_t result_[2];
// Length of reg_types_
const uint32_t num_regs_;
- // A stack of monitor enter locations
+ // A stack of monitor enter locations.
std::vector<uint32_t, TrackingAllocator<uint32_t, kAllocatorTagVerifier>> monitors_;
// A map from register to a bit vector of indices into the monitors_ stack. As we pop the monitor
// stack we verify that monitor-enter/exit are correctly nested. That is, if there was a
- // monitor-enter on v5 and then on v6, we expect the monitor-exit to be on v6 then on v5
+ // monitor-enter on v5 and then on v6, we expect the monitor-exit to be on v6 then on v5.
AllocationTrackingSafeMap<uint32_t, uint32_t, kAllocatorTagVerifier> reg_to_lock_depths_;
// An array of RegType Ids associated with each dex register.
uint16_t line_[0];
+
+ DISALLOW_COPY_AND_ASSIGN(RegisterLine);
};
} // namespace verifier
diff --git a/test/800-smali/expected.txt b/test/800-smali/expected.txt
index 3e3955b..f766b0a 100644
--- a/test/800-smali/expected.txt
+++ b/test/800-smali/expected.txt
@@ -1,4 +1,5 @@
b/17790197
+b/17978759
FloatBadArgReg
negLong
Done!
diff --git a/test/800-smali/smali/b_17978759.smali b/test/800-smali/smali/b_17978759.smali
new file mode 100644
index 0000000..07bcae5
--- /dev/null
+++ b/test/800-smali/smali/b_17978759.smali
@@ -0,0 +1,28 @@
+.class public LB17978759;
+.super Ljava/lang/Object;
+
+ .method public constructor <init>()V
+ .registers 1
+ invoke-direct {p0}, Ljava/lang/Object;-><init>()V
+ return-void
+ .end method
+
+ .method public test()V
+ .registers 2
+
+ move-object v0, p0
+ # v0 and p0 alias
+ monitor-enter p0
+ # monitor-enter on p0
+ monitor-exit v0
+ # monitor-exit on v0, however, verifier doesn't track this and so this is
+ # a warning. Verifier will still think p0 is locked.
+
+ move-object v0, p0
+ # v0 will now appear locked.
+ monitor-enter v0
+ # Attempt to lock v0 twice is a verifier failure.
+ monitor-exit v0
+
+ return-void
+ .end method
diff --git a/test/800-smali/src/Main.java b/test/800-smali/src/Main.java
index 87549d9..014edc0 100644
--- a/test/800-smali/src/Main.java
+++ b/test/800-smali/src/Main.java
@@ -15,6 +15,7 @@
*/
import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
import java.util.LinkedList;
import java.util.List;
@@ -49,6 +50,7 @@
testCases = new LinkedList<TestCase>();
testCases.add(new TestCase("b/17790197", "B17790197", "getInt", null, null, 100));
+ testCases.add(new TestCase("b/17978759", "B17978759", "test", null, new VerifyError(), null));
testCases.add(new TestCase("FloatBadArgReg", "FloatBadArgReg", "getInt",
new Object[]{100}, null, 100));
testCases.add(new TestCase("negLong", "negLong", "negLong", null, null, 122142L));
@@ -66,47 +68,59 @@
}
private void runTest(TestCase tc) throws Exception {
- Class<?> c = Class.forName(tc.testClass);
-
- Method[] methods = c.getDeclaredMethods();
-
- // For simplicity we assume that test methods are not overloaded. So searching by name
- // will give us the method we need to run.
- Method method = null;
- for (Method m : methods) {
- if (m.getName().equals(tc.testMethodName)) {
- method = m;
- break;
- }
- }
-
- if (method == null) {
- throw new IllegalArgumentException("Could not find test method " + tc.testMethodName +
- " in class " + tc.testClass + " for test " + tc.testName);
- }
-
Exception errorReturn = null;
try {
- Object retValue = method.invoke(null, tc.values);
- if (tc.expectedException != null) {
- errorReturn = new IllegalStateException("Expected an exception in test " +
- tc.testName);
+ Class<?> c = Class.forName(tc.testClass);
+
+ Method[] methods = c.getDeclaredMethods();
+
+ // For simplicity we assume that test methods are not overloaded. So searching by name
+ // will give us the method we need to run.
+ Method method = null;
+ for (Method m : methods) {
+ if (m.getName().equals(tc.testMethodName)) {
+ method = m;
+ break;
+ }
}
- if (tc.expectedReturn == null && retValue != null) {
- errorReturn = new IllegalStateException("Expected a null result in test " +
- tc.testName);
- } else if (tc.expectedReturn != null &&
- (retValue == null || !tc.expectedReturn.equals(retValue))) {
- errorReturn = new IllegalStateException("Expected return " + tc.expectedReturn +
- ", but got " + retValue);
+
+ if (method == null) {
+ errorReturn = new IllegalArgumentException("Could not find test method " +
+ tc.testMethodName + " in class " +
+ tc.testClass + " for test " +
+ tc.testName);
+ } else {
+ Object retValue;
+ if (Modifier.isStatic(method.getModifiers())) {
+ retValue = method.invoke(null, tc.values);
+ } else {
+ retValue = method.invoke(method.getDeclaringClass().newInstance(), tc.values);
+ }
+ if (tc.expectedException != null) {
+ errorReturn = new IllegalStateException("Expected an exception in test " +
+ tc.testName);
+ }
+ if (tc.expectedReturn == null && retValue != null) {
+ errorReturn = new IllegalStateException("Expected a null result in test " +
+ tc.testName);
+ } else if (tc.expectedReturn != null &&
+ (retValue == null || !tc.expectedReturn.equals(retValue))) {
+ errorReturn = new IllegalStateException("Expected return " +
+ tc.expectedReturn +
+ ", but got " + retValue);
+ } else {
+ // Expected result, do nothing.
+ }
}
- } catch (Exception exc) {
+ } catch (Throwable exc) {
if (tc.expectedException == null) {
errorReturn = new IllegalStateException("Did not expect exception", exc);
} else if (!tc.expectedException.getClass().equals(exc.getClass())) {
errorReturn = new IllegalStateException("Expected " +
- tc.expectedException.getClass().getName() +
- ", but got " + exc.getClass(), exc);
+ tc.expectedException.getClass().getName() +
+ ", but got " + exc.getClass(), exc);
+ } else {
+ // Expected exception, do nothing.
}
} finally {
if (errorReturn != null) {