am 3e5e21d4: am 1772c34e: Merge "StrictMode: gather and return violating stacks in Binder replies" into gingerbread
Merge commit '3e5e21d4dc74751e64d17379c5563ece39a7e35d'
* commit '3e5e21d4dc74751e64d17379c5563ece39a7e35d':
StrictMode: gather and return violating stacks in Binder replies
diff --git a/core/java/android/app/ApplicationErrorReport.java b/core/java/android/app/ApplicationErrorReport.java
index 9834c4c..6981cd6 100644
--- a/core/java/android/app/ApplicationErrorReport.java
+++ b/core/java/android/app/ApplicationErrorReport.java
@@ -27,6 +27,7 @@
import android.os.SystemProperties;
import android.provider.Settings;
import android.util.Printer;
+
import java.io.PrintWriter;
import java.io.StringWriter;
diff --git a/core/java/android/database/DatabaseUtils.java b/core/java/android/database/DatabaseUtils.java
index af93eee..4063534 100644
--- a/core/java/android/database/DatabaseUtils.java
+++ b/core/java/android/database/DatabaseUtils.java
@@ -108,7 +108,7 @@
* @see Parcel#readException
*/
public static final void readExceptionFromParcel(Parcel reply) {
- int code = reply.readInt();
+ int code = reply.readExceptionCode();
if (code == 0) return;
String msg = reply.readString();
DatabaseUtils.readExceptionFromParcel(reply, msg, code);
@@ -116,7 +116,7 @@
public static void readExceptionWithFileNotFoundExceptionFromParcel(
Parcel reply) throws FileNotFoundException {
- int code = reply.readInt();
+ int code = reply.readExceptionCode();
if (code == 0) return;
String msg = reply.readString();
if (code == 1) {
@@ -128,7 +128,7 @@
public static void readExceptionWithOperationApplicationExceptionFromParcel(
Parcel reply) throws OperationApplicationException {
- int code = reply.readInt();
+ int code = reply.readExceptionCode();
if (code == 0) return;
String msg = reply.readString();
if (code == 10) {
diff --git a/core/java/android/os/Binder.java b/core/java/android/os/Binder.java
index 4c40982..59b8274 100644
--- a/core/java/android/os/Binder.java
+++ b/core/java/android/os/Binder.java
@@ -299,6 +299,8 @@
private native final void init();
private native final void destroy();
+
+ // Entry point from android_util_Binder.cpp's onTransact
private boolean execTransact(int code, int dataObj, int replyObj,
int flags) {
Parcel data = Parcel.obtain(dataObj);
diff --git a/core/java/android/os/Parcel.java b/core/java/android/os/Parcel.java
index 8ad600c..bfe3b60 100644
--- a/core/java/android/os/Parcel.java
+++ b/core/java/android/os/Parcel.java
@@ -176,6 +176,7 @@
*/
public final class Parcel {
private static final boolean DEBUG_RECYCLE = false;
+ private static final String TAG = "Parcel";
@SuppressWarnings({"UnusedDeclaration"})
private int mObject; // used by native code
@@ -214,12 +215,14 @@
private static final int VAL_BOOLEANARRAY = 23;
private static final int VAL_CHARSEQUENCEARRAY = 24;
+ // The initial int32 in a Binder call's reply Parcel header:
private static final int EX_SECURITY = -1;
private static final int EX_BAD_PARCELABLE = -2;
private static final int EX_ILLEGAL_ARGUMENT = -3;
private static final int EX_NULL_POINTER = -4;
private static final int EX_ILLEGAL_STATE = -5;
-
+ private static final int EX_HAS_REPLY_HEADER = -128; // special; see below
+
public final static Parcelable.Creator<String> STRING_CREATOR
= new Parcelable.Creator<String>() {
public String createFromParcel(Parcel source) {
@@ -1216,7 +1219,31 @@
* @see #readException
*/
public final void writeNoException() {
- writeInt(0);
+ // Despite the name of this function ("write no exception"),
+ // it should instead be thought of as "write the RPC response
+ // header", but because this function name is written out by
+ // the AIDL compiler, we're not going to rename it.
+ //
+ // The response header, in the non-exception case (see also
+ // writeException above, also called by the AIDL compiler), is
+ // either a 0 (the default case), or EX_HAS_REPLY_HEADER if
+ // StrictMode has gathered up violations that have occurred
+ // during a Binder call, in which case we write out the number
+ // of violations and their details, serialized, before the
+ // actual RPC respons data. The receiving end of this is
+ // readException(), below.
+ if (StrictMode.hasGatheredViolations()) {
+ writeInt(EX_HAS_REPLY_HEADER);
+ final int sizePosition = dataPosition();
+ writeInt(0); // total size of fat header, to be filled in later
+ StrictMode.writeGatheredViolationsToParcel(this);
+ final int payloadPosition = dataPosition();
+ setDataPosition(sizePosition);
+ writeInt(payloadPosition - sizePosition); // header size
+ setDataPosition(payloadPosition);
+ } else {
+ writeInt(0);
+ }
}
/**
@@ -1229,10 +1256,44 @@
* @see #writeNoException
*/
public final void readException() {
+ int code = readExceptionCode();
+ if (code != 0) {
+ String msg = readString();
+ readException(code, msg);
+ }
+ }
+
+ /**
+ * Parses the header of a Binder call's response Parcel and
+ * returns the exception code. Deals with lite or fat headers.
+ * In the common successful case, this header is generally zero.
+ * In less common cases, it's a small negative number and will be
+ * followed by an error string.
+ *
+ * This exists purely for android.database.DatabaseUtils and
+ * insulating it from having to handle fat headers as returned by
+ * e.g. StrictMode-induced RPC responses.
+ *
+ * @hide
+ */
+ public final int readExceptionCode() {
int code = readInt();
- if (code == 0) return;
- String msg = readString();
- readException(code, msg);
+ if (code == EX_HAS_REPLY_HEADER) {
+ int headerSize = readInt();
+ if (headerSize == 0) {
+ Log.e(TAG, "Unexpected zero-sized Parcel reply header.");
+ } else {
+ // Currently the only thing in the header is StrictMode stacks,
+ // but discussions around event/RPC tracing suggest we might
+ // put that here too. If so, switch on sub-header tags here.
+ // But for now, just parse out the StrictMode stuff.
+ StrictMode.readAndHandleBinderCallViolations(this);
+ }
+ // And fat response headers are currently only used when
+ // there are no exceptions, so return no error:
+ return 0;
+ }
+ return code;
}
/**
@@ -1872,13 +1933,13 @@
creator = (Parcelable.Creator)f.get(null);
}
catch (IllegalAccessException e) {
- Log.e("Parcel", "Class not found when unmarshalling: "
+ Log.e(TAG, "Class not found when unmarshalling: "
+ name + ", e: " + e);
throw new BadParcelableException(
"IllegalAccessException when unmarshalling: " + name);
}
catch (ClassNotFoundException e) {
- Log.e("Parcel", "Class not found when unmarshalling: "
+ Log.e(TAG, "Class not found when unmarshalling: "
+ name + ", e: " + e);
throw new BadParcelableException(
"ClassNotFoundException when unmarshalling: " + name);
@@ -1983,7 +2044,7 @@
if (DEBUG_RECYCLE) {
mStack = new RuntimeException();
}
- //Log.i("Parcel", "Initializing obj=0x" + Integer.toHexString(obj), mStack);
+ //Log.i(TAG, "Initializing obj=0x" + Integer.toHexString(obj), mStack);
init(obj);
}
@@ -1991,7 +2052,7 @@
protected void finalize() throws Throwable {
if (DEBUG_RECYCLE) {
if (mStack != null) {
- Log.w("Parcel", "Client did not call Parcel.recycle()", mStack);
+ Log.w(TAG, "Client did not call Parcel.recycle()", mStack);
}
}
destroy();
@@ -2015,7 +2076,7 @@
ClassLoader loader) {
while (N > 0) {
Object value = readValue(loader);
- //Log.d("Parcel", "Unmarshalling value=" + value);
+ //Log.d(TAG, "Unmarshalling value=" + value);
outVal.add(value);
N--;
}
@@ -2025,7 +2086,7 @@
ClassLoader loader) {
for (int i = 0; i < N; i++) {
Object value = readValue(loader);
- //Log.d("Parcel", "Unmarshalling value=" + value);
+ //Log.d(TAG, "Unmarshalling value=" + value);
outVal[i] = value;
}
}
@@ -2035,7 +2096,7 @@
while (N > 0) {
int key = readInt();
Object value = readValue(loader);
- //Log.i("Parcel", "Unmarshalling key=" + key + " value=" + value);
+ //Log.i(TAG, "Unmarshalling key=" + key + " value=" + value);
outVal.append(key, value);
N--;
}
@@ -2046,7 +2107,7 @@
while (N > 0) {
int key = readInt();
boolean value = this.readByte() == 1;
- //Log.i("Parcel", "Unmarshalling key=" + key + " value=" + value);
+ //Log.i(TAG, "Unmarshalling key=" + key + " value=" + value);
outVal.append(key, value);
N--;
}
diff --git a/core/java/android/os/StrictMode.java b/core/java/android/os/StrictMode.java
index f7ad884..312bca1 100644
--- a/core/java/android/os/StrictMode.java
+++ b/core/java/android/os/StrictMode.java
@@ -23,6 +23,9 @@
import dalvik.system.BlockGuard;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
import java.util.HashMap;
/**
@@ -31,6 +34,7 @@
*/
public final class StrictMode {
private static final String TAG = "StrictMode";
+ private static final boolean LOG_V = false;
// Only log a duplicate stack trace to the logs every second.
private static final long MIN_LOG_INTERVAL_MS = 1000;
@@ -86,6 +90,19 @@
PENALTY_DROPBOX | PENALTY_DEATH;
/**
+ * Log of strict mode violation stack traces that have occurred
+ * during a Binder call, to be serialized back later to the caller
+ * via Parcel.writeNoException() (amusingly) where the caller can
+ * choose how to react.
+ */
+ private static ThreadLocal<ArrayList<ApplicationErrorReport.CrashInfo>> gatheredViolations =
+ new ThreadLocal<ArrayList<ApplicationErrorReport.CrashInfo>>() {
+ @Override protected ArrayList<ApplicationErrorReport.CrashInfo> initialValue() {
+ return new ArrayList<ApplicationErrorReport.CrashInfo>(1);
+ }
+ };
+
+ /**
* Sets the policy for what actions the current thread is denied,
* as well as the penalty for violating the policy.
*
@@ -118,6 +135,24 @@
}
}
+ private static class StrictModeNetworkViolation extends BlockGuard.BlockGuardPolicyException {
+ public StrictModeNetworkViolation(int policyMask) {
+ super(policyMask, DISALLOW_NETWORK);
+ }
+ }
+
+ private static class StrictModeDiskReadViolation extends BlockGuard.BlockGuardPolicyException {
+ public StrictModeDiskReadViolation(int policyMask) {
+ super(policyMask, DISALLOW_DISK_READ);
+ }
+ }
+
+ private static class StrictModeDiskWriteViolation extends BlockGuard.BlockGuardPolicyException {
+ public StrictModeDiskWriteViolation(int policyMask) {
+ super(policyMask, DISALLOW_DISK_WRITE);
+ }
+ }
+
/**
* Returns the bitmask of the current thread's blocking policy.
*
@@ -127,6 +162,52 @@
return BlockGuard.getThreadPolicy().getPolicyMask();
}
+ /**
+ * Parses the BlockGuard policy mask out from the Exception's
+ * getMessage() String value. Kinda gross, but least
+ * invasive. :/
+ *
+ * Input is of form "policy=137 violation=64"
+ *
+ * Returns 0 on failure, which is a valid policy, but not a
+ * valid policy during a violation (else there must've been
+ * some policy in effect to violate).
+ */
+ private static int parsePolicyFromMessage(String message) {
+ if (message == null || !message.startsWith("policy=")) {
+ return 0;
+ }
+ int spaceIndex = message.indexOf(' ');
+ if (spaceIndex == -1) {
+ return 0;
+ }
+ String policyString = message.substring(7, spaceIndex);
+ try {
+ return Integer.valueOf(policyString).intValue();
+ } catch (NumberFormatException e) {
+ return 0;
+ }
+ }
+
+ /**
+ * Like parsePolicyFromMessage(), but returns the violation.
+ */
+ private static int parseViolationFromMessage(String message) {
+ if (message == null) {
+ return 0;
+ }
+ int violationIndex = message.indexOf("violation=");
+ if (violationIndex == -1) {
+ return 0;
+ }
+ String violationString = message.substring(violationIndex + 10);
+ try {
+ return Integer.valueOf(violationString).intValue();
+ } catch (NumberFormatException e) {
+ return 0;
+ }
+ }
+
private static class AndroidBlockGuardPolicy implements BlockGuard.Policy {
private int mPolicyMask;
@@ -139,6 +220,11 @@
mPolicyMask = policyMask;
}
+ @Override
+ public String toString() {
+ return "AndroidBlockGuardPolicy; mPolicyMask=" + mPolicyMask;
+ }
+
// Part of BlockGuard.Policy interface:
public int getPolicyMask() {
return mPolicyMask;
@@ -149,7 +235,7 @@
if ((mPolicyMask & DISALLOW_DISK_WRITE) == 0) {
return;
}
- handleViolation(DISALLOW_DISK_WRITE);
+ startHandlingViolationException(new StrictModeDiskWriteViolation(mPolicyMask));
}
// Part of BlockGuard.Policy interface:
@@ -157,7 +243,7 @@
if ((mPolicyMask & DISALLOW_DISK_READ) == 0) {
return;
}
- handleViolation(DISALLOW_DISK_READ);
+ startHandlingViolationException(new StrictModeDiskReadViolation(mPolicyMask));
}
// Part of BlockGuard.Policy interface:
@@ -165,17 +251,23 @@
if ((mPolicyMask & DISALLOW_NETWORK) == 0) {
return;
}
- handleViolation(DISALLOW_NETWORK);
+ startHandlingViolationException(new StrictModeNetworkViolation(mPolicyMask));
}
public void setPolicyMask(int policyMask) {
mPolicyMask = policyMask;
}
- private void handleViolation(int violationBit) {
- final BlockGuard.BlockGuardPolicyException violation =
- new BlockGuard.BlockGuardPolicyException(mPolicyMask, violationBit);
- violation.fillInStackTrace();
+ // Start handling a violation that just started and hasn't
+ // actually run yet (e.g. no disk write or network operation
+ // has yet occurred). This sees if we're in an event loop
+ // thread and, if so, uses it to roughly measure how long the
+ // violation took.
+ void startHandlingViolationException(BlockGuard.BlockGuardPolicyException e) {
+ e.fillInStackTrace();
+ final ApplicationErrorReport.CrashInfo crashInfo = new ApplicationErrorReport.CrashInfo(e);
+ crashInfo.durationMillis = -1; // unknown
+ final int savedPolicy = mPolicyMask;
Looper looper = Looper.myLooper();
if (looper == null) {
@@ -183,38 +275,50 @@
// violation takes place. This case should be rare,
// as most users will care about timing violations
// that happen on their main UI thread.
- handleViolationWithTime(violation, -1L /* no time */);
+ handleViolation(crashInfo, savedPolicy);
} else {
MessageQueue queue = Looper.myQueue();
final long violationTime = SystemClock.uptimeMillis();
queue.addIdleHandler(new MessageQueue.IdleHandler() {
public boolean queueIdle() {
long afterViolationTime = SystemClock.uptimeMillis();
- handleViolationWithTime(violation, afterViolationTime - violationTime);
+ crashInfo.durationMillis = afterViolationTime - violationTime;
+ handleViolation(crashInfo, savedPolicy);
return false; // remove this idle handler from the array
}
});
}
+
}
- private void handleViolationWithTime(
- BlockGuard.BlockGuardPolicyException violation,
- long durationMillis) {
+ // Note: It's possible (even quite likely) that the
+ // thread-local policy mask has changed from the time the
+ // violation fired and now (after the violating code ran) due
+ // to people who push/pop temporary policy in regions of code,
+ // hence the policy being passed around.
+ void handleViolation(
+ final ApplicationErrorReport.CrashInfo crashInfo,
+ int policy) {
+ if (crashInfo.stackTrace == null) {
+ Log.d(TAG, "unexpected null stacktrace");
+ return;
+ }
- // It's possible (even quite likely) that mPolicyMask has
- // changed from the time the violation fired and now
- // (after the violating code ran) due to people who
- // push/pop temporary policy in regions of code. So use
- // the old policy here.
- int policy = violation.getPolicy();
-
- // Not _really_ a Crash, but we use the same data structure...
- ApplicationErrorReport.CrashInfo crashInfo =
- new ApplicationErrorReport.CrashInfo(violation);
- crashInfo.durationMillis = durationMillis;
+ if (LOG_V) Log.d(TAG, "handleViolation; policy=" + policy);
if ((policy & PENALTY_GATHER) != 0) {
- Log.d(TAG, "StrictMode violation via Binder call; ignoring for now.");
+ ArrayList<ApplicationErrorReport.CrashInfo> violations = gatheredViolations.get();
+ if (violations.size() >= 5) {
+ // Too many. In a loop or something? Don't gather them all.
+ return;
+ }
+ for (ApplicationErrorReport.CrashInfo previous : violations) {
+ if (crashInfo.stackTrace.equals(previous.stackTrace)) {
+ // Duplicate. Don't log.
+ return;
+ }
+ }
+ violations.add(crashInfo);
return;
}
@@ -231,11 +335,11 @@
if ((policy & PENALTY_LOG) != 0 &&
timeSinceLastViolationMillis > MIN_LOG_INTERVAL_MS) {
- if (durationMillis != -1) {
- Log.d(TAG, "StrictMode policy violation; ~duration=" + durationMillis + " ms",
- violation);
+ if (crashInfo.durationMillis != -1) {
+ Log.d(TAG, "StrictMode policy violation; ~duration=" +
+ crashInfo.durationMillis + " ms: " + crashInfo.stackTrace);
} else {
- Log.d(TAG, "StrictMode policy violation.", violation);
+ Log.d(TAG, "StrictMode policy violation: " + crashInfo.stackTrace);
}
}
@@ -255,7 +359,8 @@
}
if (violationMask != 0) {
- violationMask |= violation.getPolicyViolation();
+ int violationBit = parseViolationFromMessage(crashInfo.exceptionMessage);
+ violationMask |= violationBit;
final int savedPolicy = getThreadBlockingPolicy();
try {
// First, remove any policy before we call into the Activity Manager,
@@ -267,7 +372,7 @@
ActivityManagerNative.getDefault().handleApplicationStrictModeViolation(
RuntimeInit.getApplicationObject(),
violationMask,
- new ApplicationErrorReport.CrashInfo(violation));
+ crashInfo);
} catch (RemoteException e) {
Log.e(TAG, "RemoteException trying to handle StrictMode violation", e);
} finally {
@@ -285,6 +390,68 @@
}
/**
+ * Called from Parcel.writeNoException()
+ */
+ /* package */ static boolean hasGatheredViolations() {
+ return !gatheredViolations.get().isEmpty();
+ }
+
+ /**
+ * Called from Parcel.writeNoException()
+ */
+ /* package */ static void writeGatheredViolationsToParcel(Parcel p) {
+ ArrayList<ApplicationErrorReport.CrashInfo> violations = gatheredViolations.get();
+ p.writeInt(violations.size());
+ for (int i = 0; i < violations.size(); ++i) {
+ violations.get(i).writeToParcel(p, 0 /* unused flags? */);
+ }
+
+ if (LOG_V) Log.d(TAG, "wrote violations to response parcel; num=" + violations.size());
+ violations.clear();
+ }
+
+ private static class LogStackTrace extends Exception {}
+
+ /**
+ * Called from Parcel.readException() when the exception is EX_STRICT_MODE_VIOLATIONS,
+ * we here read back all the encoded violations.
+ */
+ /* package */ static void readAndHandleBinderCallViolations(Parcel p) {
+ // Our own stack trace to append
+ Exception e = new LogStackTrace();
+ StringWriter sw = new StringWriter();
+ e.printStackTrace(new PrintWriter(sw));
+ String ourStack = sw.toString();
+
+ int policyMask = getThreadBlockingPolicy();
+
+ int numViolations = p.readInt();
+ for (int i = 0; i < numViolations; ++i) {
+ if (LOG_V) Log.d(TAG, "strict mode violation stacks read from binder call. i=" + i);
+ ApplicationErrorReport.CrashInfo crashInfo = new ApplicationErrorReport.CrashInfo(p);
+ crashInfo.stackTrace += "# via Binder call with stack:\n" + ourStack;
+
+ // Unlike the in-process violations in which case we
+ // trigger an error _before_ the thing occurs, in this
+ // case the violating thing has already occurred, so we
+ // can't use our heuristic of waiting for the next event
+ // loop idle cycle to measure the approximate violation
+ // duration. Instead, just skip that step and use -1
+ // (unknown duration) for now.
+ // TODO: keep a thread-local on remote process of first
+ // violation time's uptimeMillis, and when writing that
+ // back out in Parcel reply, include in the header the
+ // violation time and use it here.
+ crashInfo.durationMillis = -1;
+
+ BlockGuard.Policy policy = BlockGuard.getThreadPolicy();
+ if (policy instanceof AndroidBlockGuardPolicy) {
+ ((AndroidBlockGuardPolicy) policy).handleViolation(crashInfo, policyMask);
+ }
+ }
+ }
+
+ /**
* Called from android_util_Binder.cpp's
* android_os_Parcel_enforceInterface when an incoming Binder call
* requires changing the StrictMode policy mask. The role of this
diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index bed893a..60babad 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -51,6 +51,9 @@
// Note: must be kept in sync with android/os/StrictMode.java's PENALTY_GATHER
#define STRICT_MODE_PENALTY_GATHER 0x100
+// Note: must be kept in sync with android/os/Parcel.java's EX_HAS_REPLY_HEADER
+#define EX_HAS_REPLY_HEADER -128
+
// XXX This can be made public if we want to provide
// support for typed data.
struct small_flat_data
@@ -959,7 +962,15 @@
int32_t Parcel::readExceptionCode() const
{
int32_t exception_code = readAligned<int32_t>();
- // TODO: skip over the response header here, once that's in.
+ if (exception_code == EX_HAS_REPLY_HEADER) {
+ int32_t header_size = readAligned<int32_t>();
+ // Skip over fat responses headers. Not used (or propagated) in
+ // native code
+ setDataPosition(dataPosition() + header_size);
+ // And fat response headers are currently only used when there are no
+ // exceptions, so return no error:
+ return 0;
+ }
return exception_code;
}