From 58f27b5033542150a78fb050e2f85253a48efa67 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Tue, 6 Dec 2016 16:47:00 -0700 Subject: [PATCH] Fix two StrictMode stack collection bugs. When Binder calls are nested, we can quickly end up with a snowball of stacktraces that can cause the original transaction to fail. This CL makes two specific changes to alleviate this pressure: -- Consider a nested Binder call from PID A -> B -> C. If both B and C encounter dozens of StrictMode violations, then gatheredViolations in B will end up with 10 ViolationInfo (5 from B and 5 from C). This problem only grows with each successive nested call. To solve this, always limit ourselves to only ever write out 3 ViolationInfo from any given process. -- CrashInfo already nicely truncates any large stack traces to 20kB, but readAndHandleBinderCallViolations() blindly appends the entire local trace, and never considers truncating again. Similar to the first problem above, nested calls can quickly cause the stackTrace value to explode in size. To solve this, we always re-truncate the stackTrace value after appending our local stack. Also fix some NPE bugs when missing crashInfo. Test: builds, boots Bug: 32575987 Change-Id: Ie8373ca277296f920f2b1c564d419c702a8ee0f2 --- .../android/app/ApplicationErrorReport.java | 5 ++ core/java/android/os/StrictMode.java | 80 ++++++++----------- 2 files changed, 38 insertions(+), 47 deletions(-) diff --git a/core/java/android/app/ApplicationErrorReport.java b/core/java/android/app/ApplicationErrorReport.java index 6cfa362d15eab..9f1a539ab240c 100644 --- a/core/java/android/app/ApplicationErrorReport.java +++ b/core/java/android/app/ApplicationErrorReport.java @@ -378,6 +378,11 @@ public class ApplicationErrorReport implements Parcelable { exceptionMessage = sanitizeString(exceptionMessage); } + /** {@hide} */ + public void appendStackTrace(String tr) { + stackTrace = sanitizeString(stackTrace + tr); + } + /** * Ensure that the string is of reasonable size, truncating from the middle if needed. */ diff --git a/core/java/android/os/StrictMode.java b/core/java/android/os/StrictMode.java index f2519be0ad803..fd0970ed57591 100644 --- a/core/java/android/os/StrictMode.java +++ b/core/java/android/os/StrictMode.java @@ -1474,9 +1474,6 @@ public final class StrictMode { if (violations == null) { violations = new ArrayList(1); gatheredViolations.set(violations); - } else if (violations.size() >= 5) { - // Too many. In a loop or something? Don't gather them all. - return; } for (ViolationInfo previous : violations) { if (info.crashInfo.stackTrace.equals(previous.crashInfo.stackTrace)) { @@ -1990,18 +1987,14 @@ public final class StrictMode { if (violations == null) { p.writeInt(0); } else { - p.writeInt(violations.size()); - for (int i = 0; i < violations.size(); ++i) { - int start = p.dataPosition(); - violations.get(i).writeToParcel(p, 0 /* unused flags? */); - int size = p.dataPosition()-start; - if (size > 10*1024) { - Slog.d(TAG, "Wrote violation #" + i + " of " + violations.size() + ": " - + (p.dataPosition()-start) + " bytes"); - } + // To avoid taking up too much transaction space, only include + // details for the first 3 violations. Deep inside, CrashInfo + // will truncate each stack trace to ~20kB. + final int size = Math.min(violations.size(), 3); + p.writeInt(size); + for (int i = 0; i < size; i++) { + violations.get(i).writeToParcel(p, 0); } - if (LOG_V) Log.d(TAG, "wrote violations to response parcel; num=" + violations.size()); - violations.clear(); // somewhat redundant, as we're about to null the threadlocal } gatheredViolations.set(null); } @@ -2015,40 +2008,19 @@ public final class StrictMode { /* package */ static void readAndHandleBinderCallViolations(Parcel p) { // Our own stack trace to append StringWriter sw = new StringWriter(); + sw.append("# via Binder call with stack:\n"); PrintWriter pw = new FastPrintWriter(sw, false, 256); new LogStackTrace().printStackTrace(pw); pw.flush(); String ourStack = sw.toString(); - int policyMask = getThreadPolicyMask(); - boolean currentlyGathering = (policyMask & PENALTY_GATHER) != 0; + final int policyMask = getThreadPolicyMask(); + final boolean currentlyGathering = (policyMask & PENALTY_GATHER) != 0; - 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); - ViolationInfo info = new ViolationInfo(p, !currentlyGathering); - if (info.crashInfo.stackTrace != null && info.crashInfo.stackTrace.length() > 30000) { - String front = info.crashInfo.stackTrace.substring(0, 256); - // 30000 characters is way too large for this to be any sane kind of - // strict mode collection of stacks. We've had a problem where we leave - // strict mode violations associated with the thread, and it keeps tacking - // more and more stacks on to the violations. Looks like we're in this casse, - // so we'll report it and bail on all of the current strict mode violations - // we currently are maintaining for this thread. - // First, drain the remaining violations from the parcel. - i++; // Skip the current entry. - for (; i < numViolations; i++) { - info = new ViolationInfo(p, !currentlyGathering); - } - // Next clear out all gathered violations. - clearGatheredViolations(); - // Now report the problem. - Slog.wtfStack(TAG, "Stack is too large: numViolations=" + numViolations - + " policy=#" + Integer.toHexString(policyMask) - + " front=" + front); - return; - } - info.crashInfo.stackTrace += "# via Binder call with stack:\n" + ourStack; + final int size = p.readInt(); + for (int i = 0; i < size; i++) { + final ViolationInfo info = new ViolationInfo(p, !currentlyGathering); + info.crashInfo.appendStackTrace(ourStack); BlockGuard.Policy policy = BlockGuard.getThreadPolicy(); if (policy instanceof AndroidBlockGuardPolicy) { ((AndroidBlockGuardPolicy) policy).handleViolationWithTimingAttempt(info); @@ -2391,7 +2363,7 @@ public final class StrictMode { * @hide */ public static class ViolationInfo implements Parcelable { - public String message; + public final String message; /** * Stack and other stuff info. @@ -2450,6 +2422,7 @@ public final class StrictMode { * Create an uninitialized instance of ViolationInfo */ public ViolationInfo() { + message = null; crashInfo = null; policy = 0; } @@ -2496,7 +2469,9 @@ public final class StrictMode { @Override public int hashCode() { int result = 17; - result = 37 * result + crashInfo.stackTrace.hashCode(); + if (crashInfo != null) { + result = 37 * result + crashInfo.stackTrace.hashCode(); + } if (numAnimationsRunning != 0) { result *= 37; } @@ -2526,7 +2501,11 @@ public final class StrictMode { */ public ViolationInfo(Parcel in, boolean unsetGatheringBit) { message = in.readString(); - crashInfo = new ApplicationErrorReport.CrashInfo(in); + if (in.readInt() != 0) { + crashInfo = new ApplicationErrorReport.CrashInfo(in); + } else { + crashInfo = null; + } int rawPolicy = in.readInt(); if (unsetGatheringBit) { policy = rawPolicy & ~PENALTY_GATHER; @@ -2548,7 +2527,12 @@ public final class StrictMode { @Override public void writeToParcel(Parcel dest, int flags) { dest.writeString(message); - crashInfo.writeToParcel(dest, flags); + if (crashInfo != null) { + dest.writeInt(1); + crashInfo.writeToParcel(dest, flags); + } else { + dest.writeInt(0); + } int start = dest.dataPosition(); dest.writeInt(policy); dest.writeInt(durationMillis); @@ -2576,7 +2560,9 @@ public final class StrictMode { * Dump a ViolationInfo instance to a Printer. */ public void dump(Printer pw, String prefix) { - crashInfo.dump(pw, prefix); + if (crashInfo != null) { + crashInfo.dump(pw, prefix); + } pw.println(prefix + "policy: " + policy); if (durationMillis != -1) { pw.println(prefix + "durationMillis: " + durationMillis);