From 4de5a3ac6655f76b67af38712ae5aeb6d7c15938 Mon Sep 17 00:00:00 2001 From: Dianne Hackborn Date: Mon, 20 Jun 2016 15:20:15 -0700 Subject: [PATCH] Reduce race when connecting to an existing content provider. We lost the code that checks to see if the target process still exists and aborts trying to use it if so. To reduce the race there, we have a new explicit check of the state of the process. Hopefully fixes some of issue #28737082. Change-Id: I37a7a6e9767516d564168ef5e110c4adafe3fb76 --- core/java/android/os/Process.java | 2 ++ core/jni/android_util_Process.cpp | 10 ++++-- .../android/server/am/ActivityManagerService.java | 42 +++++++++++++++++++++- .../java/com/android/server/am/ProcessRecord.java | 5 ++- 4 files changed, 55 insertions(+), 4 deletions(-) diff --git a/core/java/android/os/Process.java b/core/java/android/os/Process.java index 4506f51fe38a..f664e70cf7be 100644 --- a/core/java/android/os/Process.java +++ b/core/java/android/os/Process.java @@ -1184,6 +1184,8 @@ public class Process { /** @hide */ public static final int PROC_QUOTES = 0x400; /** @hide */ + public static final int PROC_CHAR = 0x800; + /** @hide */ public static final int PROC_OUT_STRING = 0x1000; /** @hide */ public static final int PROC_OUT_LONG = 0x2000; diff --git a/core/jni/android_util_Process.cpp b/core/jni/android_util_Process.cpp index f7a5e8a07fc0..3d952b0f713f 100644 --- a/core/jni/android_util_Process.cpp +++ b/core/jni/android_util_Process.cpp @@ -832,6 +832,7 @@ enum { PROC_COMBINE = 0x100, PROC_PARENS = 0x200, PROC_QUOTES = 0x400, + PROC_CHAR = 0x800, PROC_OUT_STRING = 0x1000, PROC_OUT_LONG = 0x2000, PROC_OUT_FLOAT = 0x4000, @@ -933,8 +934,13 @@ jboolean android_os_Process_parseProcLineArray(JNIEnv* env, jobject clazz, floatsData[di] = strtof(buffer+start, &end); } if ((mode&PROC_OUT_LONG) != 0 && di < NL) { - char* end; - longsData[di] = strtoll(buffer+start, &end, 10); + if ((mode&PROC_CHAR) != 0) { + // Caller wants single first character returned as one long. + longsData[di] = buffer[start]; + } else { + char* end; + longsData[di] = strtoll(buffer+start, &end, 10); + } } if ((mode&PROC_OUT_STRING) != 0 && di < NS) { jstring str = env->NewStringUTF(buffer+start); diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index e9841129c4c3..c1a9d8555df9 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -273,6 +273,10 @@ import static android.content.pm.PackageManager.MATCH_SYSTEM_ONLY; import static android.content.pm.PackageManager.MATCH_UNINSTALLED_PACKAGES; import static android.content.pm.PackageManager.PERMISSION_GRANTED; import static android.content.res.Configuration.UI_MODE_TYPE_TELEVISION; +import static android.os.Process.PROC_CHAR; +import static android.os.Process.PROC_OUT_LONG; +import static android.os.Process.PROC_PARENS; +import static android.os.Process.PROC_SPACE_TERM; import static android.provider.Settings.Global.ALWAYS_FINISH_ACTIVITIES; import static android.provider.Settings.Global.DEBUG_APP; import static android.provider.Settings.Global.DEVELOPMENT_ENABLE_FREEFORM_WINDOWS_SUPPORT; @@ -6412,7 +6416,7 @@ public final class ActivityManagerService extends ActivityManagerNative EventLog.writeEvent(EventLogTags.AM_PROC_BOUND, app.userId, app.pid, app.processName); app.makeActive(thread, mProcessStats); - app.curAdj = app.setAdj = ProcessList.INVALID_ADJ; + app.curAdj = app.setAdj = app.verifiedAdj = ProcessList.INVALID_ADJ; app.curSchedGroup = app.setSchedGroup = ProcessList.SCHED_GROUP_DEFAULT; app.forcingToForeground = null; updateProcessForegroundLocked(app, false, false); @@ -10535,6 +10539,30 @@ public final class ActivityManagerService extends ActivityManagerNative } } + private static final int[] PROCESS_STATE_STATS_FORMAT = new int[] { + PROC_SPACE_TERM, + PROC_SPACE_TERM|PROC_PARENS, + PROC_SPACE_TERM|PROC_CHAR|PROC_OUT_LONG, // 3: process state + }; + + private final long[] mProcessStateStatsLongs = new long[1]; + + boolean isProcessAliveLocked(ProcessRecord proc) { + if (proc.procStatFile == null) { + proc.procStatFile = "/proc/" + proc.pid + "/stat"; + } + mProcessStateStatsLongs[0] = 0; + if (!Process.readProcFile(proc.procStatFile, PROCESS_STATE_STATS_FORMAT, null, + mProcessStateStatsLongs, null)) { + if (DEBUG_OOM_ADJ) Slog.d(TAG, "UNABLE TO RETRIEVE STATE FOR " + proc.procStatFile); + return false; + } + final long state = mProcessStateStatsLongs[0]; + if (DEBUG_OOM_ADJ) Slog.d(TAG, "RETRIEVED STATE FOR " + proc.procStatFile + ": " + + (char)state); + return state != 'Z' && state != 'X' && state != 'x' && state != 'K'; + } + private ContentProviderHolder getContentProviderImpl(IApplicationThread caller, String name, IBinder token, boolean stable, int userId) { ContentProviderRecord cpr; @@ -10622,7 +10650,16 @@ public final class ActivityManagerService extends ActivityManagerNative } checkTime(startTime, "getContentProviderImpl: before updateOomAdj"); + final int verifiedAdj = cpr.proc.verifiedAdj; boolean success = updateOomAdjLocked(cpr.proc); + // XXX things have changed so updateOomAdjLocked doesn't actually tell us + // if the process has been successfully adjusted. So to reduce races with + // it, we will check whether the process still exists. Note that this doesn't + // completely get rid of races with LMK killing the process, but should make + // them much smaller. + if (success && verifiedAdj != cpr.proc.setAdj && !isProcessAliveLocked(cpr.proc)) { + success = false; + } maybeUpdateProviderUsageStatsLocked(r, cpr.info.packageName, name); checkTime(startTime, "getContentProviderImpl: after updateOomAdj"); if (DEBUG_PROVIDER) Slog.i(TAG_PROVIDER, "Adjust success: " + success); @@ -10648,6 +10685,8 @@ public final class ActivityManagerService extends ActivityManagerNative } providerRunning = false; conn = null; + } else { + cpr.proc.verifiedAdj = cpr.proc.setAdj; } Binder.restoreCallingIdentity(origId); @@ -20069,6 +20108,7 @@ public final class ActivityManagerService extends ActivityManagerNative "Set " + app.pid + " " + app.processName + " adj " + app.curAdj + ": " + app.adjType); app.setAdj = app.curAdj; + app.verifiedAdj = ProcessList.INVALID_ADJ; } if (app.setSchedGroup != app.curSchedGroup) { diff --git a/services/core/java/com/android/server/am/ProcessRecord.java b/services/core/java/com/android/server/am/ProcessRecord.java index 691fd2abe0b3..8911a3e94979 100644 --- a/services/core/java/com/android/server/am/ProcessRecord.java +++ b/services/core/java/com/android/server/am/ProcessRecord.java @@ -75,6 +75,7 @@ final class ProcessRecord { ProcessState baseProcessTracker; BatteryStatsImpl.Uid.Proc curProcBatteryStats; int pid; // The process of this application; 0 if none + String procStatFile; // path to /proc//stat int[] gids; // The gids this process was launched with String requiredAbi; // The ABI this process was launched with String instructionSet; // The instruction set this process was launched with @@ -93,6 +94,7 @@ final class ProcessRecord { int setRawAdj; // Last set OOM unlimited adjustment for this process int curAdj; // Current OOM adjustment for this process int setAdj; // Last set OOM adjustment for this process + int verifiedAdj; // The last adjustment that was verified as actually being set int curSchedGroup; // Currently desired scheduling class int setSchedGroup; // Last set to background scheduling class int trimMemoryLevel; // Last selected memory trimming level @@ -441,7 +443,7 @@ final class ProcessRecord { pkgList.put(_info.packageName, new ProcessStats.ProcessStateHolder(_info.versionCode)); maxAdj = ProcessList.UNKNOWN_ADJ; curRawAdj = setRawAdj = ProcessList.INVALID_ADJ; - curAdj = setAdj = ProcessList.INVALID_ADJ; + curAdj = setAdj = verifiedAdj = ProcessList.INVALID_ADJ; persistent = false; removed = false; lastStateTime = lastPssTime = nextPssTime = SystemClock.uptimeMillis(); @@ -449,6 +451,7 @@ final class ProcessRecord { public void setPid(int _pid) { pid = _pid; + procStatFile = null; shortStringName = null; stringName = null; } -- 2.11.0