From 1126a08dd823c95a73dbad7b3491605b3c9cbc64 Mon Sep 17 00:00:00 2001 From: Mitch Phillips Date: Mon, 20 Mar 2023 14:43:23 -0700 Subject: [PATCH] New ActivityManager<->debuggerd protocol for recoverable crashes. Recoverable GWP-ASan catches memory safety bugs, prints debuggerd reports, but then is able to patch up the process so that a crash doesn't have to occur. Currently, because AM receives an IPC from debuggerd, this results in the app being killed. We previously worked around this by just making debuggerd not send an IPC for GWP-ASan crashes, but this was overzealous and resulted in the debuggerd crash not finding its way to dropbox. So, to handle this, we introduce a new API between debuggerd and ActivityManager. For recoverable crashes, introduce a new log tag and extend other stats logging areas to also add fields to indicate the crash was recoverable. Readers of these fields should assume that, if the recoverable field is set, the app didn't actually crash but the app had a recoverable memory safety violation. The GWP-ASan CTS additions (in this topic) cover the behaviour and ensure that the recoverable crashes make their way to dropbox. Bug: 270720163 Test: atest CtsGwpAsanTestCases Change-Id: If3515d30a8dcacd9434b30751ae735b050d88e7d --- .../server/am/ActivityManagerService.java | 13 +- .../android/server/am/EventLogTags.logtags | 2 +- .../server/am/NativeCrashListener.java | 143 ++++++++++-------- 3 files changed, 92 insertions(+), 66 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 80ba683043d2e..66161b96fc0be 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -8405,13 +8405,16 @@ public class ActivityManagerService extends IActivityManager.Stub } } + boolean recoverable = eventType.equals("native_recoverable_crash"); + EventLogTags.writeAmCrash(Binder.getCallingPid(), UserHandle.getUserId(Binder.getCallingUid()), processName, r == null ? -1 : r.info.flags, crashInfo.exceptionClassName, crashInfo.exceptionMessage, crashInfo.throwFileName, - crashInfo.throwLineNumber); + crashInfo.throwLineNumber, + recoverable ? 1 : 0); int processClassEnum = processName.equals("system_server") ? ServerProtoEnums.SYSTEM_SERVER : (r != null) ? r.getProcessClassEnum() @@ -8479,7 +8482,13 @@ public class ActivityManagerService extends IActivityManager.Stub eventType, r, processName, null, null, null, null, null, null, crashInfo, new Float(loadingProgress), incrementalMetrics, null); - mAppErrors.crashApplication(r, crashInfo); + // For GWP-ASan recoverable crashes, don't make the app crash (the whole point of + // 'recoverable' is that the app doesn't crash). Normally, for nonrecoreable native crashes, + // debuggerd will terminate the process, but there's a backup where ActivityManager will + // also kill it. Avoid that. + if (!recoverable) { + mAppErrors.crashApplication(r, crashInfo); + } } public void handleApplicationStrictModeViolation( diff --git a/services/core/java/com/android/server/am/EventLogTags.logtags b/services/core/java/com/android/server/am/EventLogTags.logtags index d080036733a5f..6ce70a1ee1d44 100644 --- a/services/core/java/com/android/server/am/EventLogTags.logtags +++ b/services/core/java/com/android/server/am/EventLogTags.logtags @@ -53,7 +53,7 @@ option java_package com.android.server.am 30037 am_process_start_timeout (User|1|5),(PID|1|5),(UID|1|5),(Process Name|3) # Unhandled exception -30039 am_crash (User|1|5),(PID|1|5),(Process Name|3),(Flags|1|5),(Exception|3),(Message|3),(File|3),(Line|1|5) +30039 am_crash (User|1|5),(PID|1|5),(Process Name|3),(Flags|1|5),(Exception|3),(Message|3),(File|3),(Line|1|5),(Recoverable|1|5) # Log.wtf() called 30040 am_wtf (User|1|5),(PID|1|5),(Process Name|3),(Flags|1|5),(Tag|3),(Message|3) diff --git a/services/core/java/com/android/server/am/NativeCrashListener.java b/services/core/java/com/android/server/am/NativeCrashListener.java index 94eb07611037b..cd119e7e3fbc0 100644 --- a/services/core/java/com/android/server/am/NativeCrashListener.java +++ b/services/core/java/com/android/server/am/NativeCrashListener.java @@ -64,12 +64,15 @@ final class NativeCrashListener extends Thread { class NativeCrashReporter extends Thread { ProcessRecord mApp; int mSignal; + boolean mGwpAsanRecoverableCrash; String mCrashReport; - NativeCrashReporter(ProcessRecord app, int signal, String report) { + NativeCrashReporter(ProcessRecord app, int signal, boolean gwpAsanRecoverableCrash, + String report) { super("NativeCrashReport"); mApp = app; mSignal = signal; + mGwpAsanRecoverableCrash = gwpAsanRecoverableCrash; mCrashReport = report; } @@ -85,7 +88,9 @@ final class NativeCrashListener extends Thread { ci.stackTrace = mCrashReport; if (DEBUG) Slog.v(TAG, "Calling handleApplicationCrash()"); - mAm.handleApplicationCrashInner("native_crash", mApp, mApp.processName, ci); + mAm.handleApplicationCrashInner( + mGwpAsanRecoverableCrash ? "native_recoverable_crash" : "native_crash", + mApp, mApp.processName, ci); if (DEBUG) Slog.v(TAG, "<-- handleApplicationCrash() returned"); } catch (Exception e) { Slog.e(TAG, "Unable to report native crash", e); @@ -207,9 +212,14 @@ final class NativeCrashListener extends Thread { // permits crash_dump to connect to it. This allows us to trust the // received values. - // first, the pid and signal number - int headerBytes = readExactly(fd, buf, 0, 8); - if (headerBytes != 8) { + // Activity Manager protocol: + // - 32-bit network-byte-order: pid + // - 32-bit network-byte-order: signal number + // - byte: gwpAsanRecoverableCrash + // - bytes: raw text of the dump + // - null terminator + int headerBytes = readExactly(fd, buf, 0, 9); + if (headerBytes != 9) { // protocol failure; give up Slog.e(TAG, "Unable to read from debuggerd"); return; @@ -217,69 +227,76 @@ final class NativeCrashListener extends Thread { int pid = unpackInt(buf, 0); int signal = unpackInt(buf, 4); + boolean gwpAsanRecoverableCrash = buf[8] != 0; if (DEBUG) { - Slog.v(TAG, "Read pid=" + pid + " signal=" + signal); + Slog.v(TAG, "Read pid=" + pid + " signal=" + signal + + " recoverable=" + gwpAsanRecoverableCrash); + } + if (pid < 0) { + Slog.e(TAG, "Bogus pid!"); + return; } // now the text of the dump - if (pid > 0) { - final ProcessRecord pr; - synchronized (mAm.mPidsSelfLocked) { - pr = mAm.mPidsSelfLocked.get(pid); - } - if (pr != null) { - // Don't attempt crash reporting for persistent apps - if (pr.isPersistent()) { - if (DEBUG) { - Slog.v(TAG, "Skipping report for persistent app " + pr); - } - return; - } - - int bytes; - do { - // get some data - bytes = Os.read(fd, buf, 0, buf.length); - if (bytes > 0) { - if (MORE_DEBUG) { - String s = new String(buf, 0, bytes, "UTF-8"); - Slog.v(TAG, "READ=" + bytes + "> " + s); - } - // did we just get the EOD null byte? - if (buf[bytes-1] == 0) { - os.write(buf, 0, bytes-1); // exclude the EOD token - break; - } - // no EOD, so collect it and read more - os.write(buf, 0, bytes); - } - } while (bytes > 0); - - // Okay, we've got the report. - if (DEBUG) Slog.v(TAG, "processing"); - - // Mark the process record as being a native crash so that the - // cleanup mechanism knows we're still submitting the report - // even though the process will vanish as soon as we let - // debuggerd proceed. - synchronized (mAm) { - synchronized (mAm.mProcLock) { - pr.mErrorState.setCrashing(true); - pr.mErrorState.setForceCrashReport(true); - } - } - - // Crash reporting is synchronous but we want to let debuggerd - // go about it business right away, so we spin off the actual - // reporting logic on a thread and let it take it's time. - final String reportString = new String(os.toByteArray(), "UTF-8"); - (new NativeCrashReporter(pr, signal, reportString)).start(); - } else { - Slog.w(TAG, "Couldn't find ProcessRecord for pid " + pid); - } - } else { - Slog.e(TAG, "Bogus pid!"); + final ProcessRecord pr; + synchronized (mAm.mPidsSelfLocked) { + pr = mAm.mPidsSelfLocked.get(pid); } + if (pr == null) { + Slog.w(TAG, "Couldn't find ProcessRecord for pid " + pid); + return; + } + + // Don't attempt crash reporting for persistent apps + if (pr.isPersistent()) { + if (DEBUG) { + Slog.v(TAG, "Skipping report for persistent app " + pr); + } + return; + } + + int bytes; + do { + // get some data + bytes = Os.read(fd, buf, 0, buf.length); + if (bytes > 0) { + if (MORE_DEBUG) { + String s = new String(buf, 0, bytes, "UTF-8"); + Slog.v(TAG, "READ=" + bytes + "> " + s); + } + // did we just get the EOD null byte? + if (buf[bytes - 1] == 0) { + os.write(buf, 0, bytes - 1); // exclude the EOD token + break; + } + // no EOD, so collect it and read more + os.write(buf, 0, bytes); + } + } while (bytes > 0); + + // Okay, we've got the report. + if (DEBUG) Slog.v(TAG, "processing"); + + // Mark the process record as being a native crash so that the + // cleanup mechanism knows we're still submitting the report even + // though the process will vanish as soon as we let debuggerd + // proceed. This isn't relevant for recoverable crashes, as we don't + // show the user an "app crashed" dialogue because the app (by + // design) didn't crash. + if (!gwpAsanRecoverableCrash) { + synchronized (mAm) { + synchronized (mAm.mProcLock) { + pr.mErrorState.setCrashing(true); + pr.mErrorState.setForceCrashReport(true); + } + } + } + + // Crash reporting is synchronous but we want to let debuggerd + // go about it business right away, so we spin off the actual + // reporting logic on a thread and let it take it's time. + final String reportString = new String(os.toByteArray(), "UTF-8"); + (new NativeCrashReporter(pr, signal, gwpAsanRecoverableCrash, reportString)).start(); } catch (Exception e) { Slog.e(TAG, "Exception dealing with report", e); // ugh, fail.