Merge "Fixes two data races in USAP pool management." into qt-dev-plus-aosp

core/java/com/android/internal/os/Zygote.java

@@ -519,6 +519,9 @@ public final class Zygote {
             try {
                 sessionSocket = usapPoolSocket.accept();
 
+                // Block SIGTERM so we won't be killed if the Zygote flushes the USAP pool.
+                blockSigTerm();
+
                 BufferedReader usapReader =
                         new BufferedReader(new InputStreamReader(sessionSocket.getInputStream()));
                 usapOutputStream =
@@ -537,14 +540,22 @@ public final class Zygote {
                 } else {
                     Log.e("USAP", "Truncated command received.");
                     IoUtils.closeQuietly(sessionSocket);
+
+                    // Re-enable SIGTERM so the USAP can be flushed from the pool if necessary.
+                    unblockSigTerm();
                 }
             } catch (Exception ex) {
                 Log.e("USAP", ex.getMessage());
                 IoUtils.closeQuietly(sessionSocket);
+
+                // Re-enable SIGTERM so the USAP can be flushed from the pool if necessary.
+                unblockSigTerm();
             }
         }
 
-        setAppProcessName(args, "USAP");
+        try {
+            // SIGTERM is blocked on loop exit.  This prevents a USAP that is specializing from
+            // being killed during a pool flush.
 
             applyUidSecurityPolicy(args, peerCredentials);
             applyDebuggerSystemProperty(args);
@@ -562,21 +573,24 @@ public final class Zygote {
                 // Process.ProcessStartResult object.
                 usapOutputStream.writeInt(pid);
             } catch (IOException ioEx) {
-            Log.e("USAP", "Failed to write response to session socket: " + ioEx.getMessage());
+                Log.e("USAP", "Failed to write response to session socket: "
-            System.exit(-1);
+                        + ioEx.getMessage());
+                throw new RuntimeException(ioEx);
             } finally {
                 IoUtils.closeQuietly(sessionSocket);
 
                 try {
-                // This socket is closed using Os.close due to an issue with the implementation of
+                    // This socket is closed using Os.close due to an issue with the implementation
-                // LocalSocketImp.close.  Because the raw FD is created by init and then loaded from
+                    // of LocalSocketImp.close().  Because the raw FD is created by init and then
-                // an environment variable (as opposed to being created by the LocalSocketImpl
+                    // loaded from an environment variable (as opposed to being created by the
-                // itself) the current implementation will not actually close the underlying FD.
+                    // LocalSocketImpl itself) the current implementation will not actually close
+                    // the underlying FD.
                     //
                     // See b/130309968 for discussion of this issue.
                     Os.close(usapPoolSocket.getFileDescriptor());
                 } catch (ErrnoException ex) {
-                Log.e("USAP", "Failed to close USAP pool socket: " + ex.getMessage());
+                    Log.e("USAP", "Failed to close USAP pool socket");
+                    throw new RuntimeException(ex);
                 }
             }
 
@@ -586,8 +600,8 @@ public final class Zygote {
                 DataOutputStream outputStream = new DataOutputStream(buffer);
 
                 // This is written as a long so that the USAP reporting pipe and USAP pool event FD
-            // handlers in ZygoteServer.runSelectLoop can be unified.  These two cases should both
+                // handlers in ZygoteServer.runSelectLoop can be unified.  These two cases should
-            // send/receive 8 bytes.
+                // both send/receive 8 bytes.
                 outputStream.writeLong(pid);
                 outputStream.flush();
 
@@ -596,7 +610,7 @@ public final class Zygote {
                 Log.e("USAP",
                         String.format("Failed to write PID (%d) to pipe (%d): %s",
                                 pid, writePipe.getInt$(), ex.getMessage()));
-            System.exit(-1);
+                throw new RuntimeException(ex);
             } finally {
                 IoUtils.closeQuietly(writePipe);
             }
@@ -608,24 +622,34 @@ public final class Zygote {
 
             disableExecuteOnly(args.mTargetSdkVersion);
 
+            if (args.mNiceName != null) {
+                Process.setArgV0(args.mNiceName);
+            }
+
             // End of the postFork event.
             Trace.traceEnd(Trace.TRACE_TAG_ACTIVITY_MANAGER);
 
             return ZygoteInit.zygoteInit(args.mTargetSdkVersion,
                                          args.mRemainingArgs,
                                          null /* classLoader */);
+        } finally {
+            // Unblock SIGTERM to restore the process to default behavior.
+            unblockSigTerm();
+        }
     }
 
-    static void setAppProcessName(ZygoteArguments args, String loggingTag) {
+    private static void blockSigTerm() {
-        if (args.mNiceName != null) {
+        nativeBlockSigTerm();
-            Process.setArgV0(args.mNiceName);
-        } else if (args.mPackageName != null) {
-            Process.setArgV0(args.mPackageName);
-        } else {
-            Log.w(loggingTag, "Unable to set package name.");
     }
+
+    private static native void nativeBlockSigTerm();
+
+    private static void unblockSigTerm() {
+        nativeUnblockSigTerm();
     }
 
+    private static native void nativeUnblockSigTerm();
+
     private static final String USAP_ERROR_PREFIX = "Invalid command to USAP: ";
 
     /**

core/java/com/android/internal/os/ZygoteConnection.java

@@ -322,6 +322,7 @@ class ZygoteConnection {
             Runnable stateChangeCode) {
         try {
             if (zygoteServer.isUsapPoolEnabled()) {
+                Log.i(TAG, "Emptying USAP Pool due to state change.");
                 Zygote.emptyUsapPool();
             }
 
@@ -335,6 +336,8 @@ class ZygoteConnection {
                 if (fpResult != null) {
                     zygoteServer.setForkChild();
                     return fpResult;
+                } else {
+                    Log.i(TAG, "Finished refilling USAP Pool after state change.");
                 }
             }
 
@@ -467,8 +470,6 @@ class ZygoteConnection {
 
         closeSocket();
 
-        Zygote.setAppProcessName(parsedArgs, TAG);
-
         // End of the postFork event.
         Trace.traceEnd(Trace.TRACE_TAG_ACTIVITY_MANAGER);
         if (parsedArgs.mInvokeWith != null) {

core/jni/com_android_internal_os_Zygote.cpp

@@ -198,7 +198,7 @@ class UsapTableEntry {
    * PIDs don't match nothing will happen.
    *
    * @param pid The ID of the process who's entry we want to clear.
-   * @return True if the entry was cleared; false otherwise
+   * @return True if the entry was cleared by this call; false otherwise
    */
   bool ClearForPID(int32_t pid) {
     EntryStorage storage = mStorage.load();
@@ -212,14 +212,16 @@ class UsapTableEntry {
        *   3) It fails and the new value isn't INVALID_ENTRY_VALUE, in which
        *      case the entry has already been cleared and re-used.
        *
-       * In all three cases the goal of the caller has been met and we can
+       * In all three cases the goal of the caller has been met, but only in
-       * return true.
+       * the first case do we need to decrement the pool count.
        */
       if (mStorage.compare_exchange_strong(storage, INVALID_ENTRY_VALUE)) {
         close(storage.read_pipe_fd);
+        return true;
+      } else {
+        return false;
       }
 
-      return true;
     } else {
       return false;
     }
@@ -331,11 +333,24 @@ static void SigChldHandler(int /*signal_number*/) {
     if (WIFEXITED(status)) {
       async_safe_format_log(ANDROID_LOG_INFO, LOG_TAG,
                             "Process %d exited cleanly (%d)", pid, WEXITSTATUS(status));
+
+      // Check to see if the PID is in the USAP pool and remove it if it is.
+      if (RemoveUsapTableEntry(pid)) {
+        ++usaps_removed;
+      }
     } else if (WIFSIGNALED(status)) {
       async_safe_format_log(ANDROID_LOG_INFO, LOG_TAG,
                             "Process %d exited due to signal %d (%s)%s", pid,
                             WTERMSIG(status), strsignal(WTERMSIG(status)),
                             WCOREDUMP(status) ? "; core dumped" : "");
+
+      // If the process exited due to a signal other than SIGTERM, check to see
+      // if the PID is in the USAP pool and remove it if it is.  If the process
+      // was closed by the Zygote using SIGTERM then the USAP pool entry will
+      // have already been removed (see nativeEmptyUsapPool()).
+      if (WTERMSIG(status) != SIGTERM && RemoveUsapTableEntry(pid)) {
+        ++usaps_removed;
+      }
     }
 
     // If the just-crashed process is the system_server, bring down zygote
@@ -346,11 +361,6 @@ static void SigChldHandler(int /*signal_number*/) {
                             "Exit zygote because system server (pid %d) has terminated", pid);
       kill(getpid(), SIGKILL);
     }
-
-    // Check to see if the PID is in the USAP pool and remove it if it is.
-    if (RemoveUsapTableEntry(pid)) {
-      ++usaps_removed;
-    }
   }
 
   // Note that we shouldn't consider ECHILD an error because
@@ -1648,7 +1658,13 @@ static void com_android_internal_os_Zygote_nativeEmptyUsapPool(JNIEnv* env, jcla
     auto entry_storage = entry.GetValues();
 
     if (entry_storage.has_value()) {
-      kill(entry_storage.value().pid, SIGKILL);
+      kill(entry_storage.value().pid, SIGTERM);
+
+      // Clean up the USAP table entry here.  This avoids a potential race
+      // where a newly created USAP might not be able to find a valid table
+      // entry if signal handler (which would normally do the cleanup) doesn't
+      // run between now and when the new process is created.
+
       close(entry_storage.value().read_pipe_fd);
 
       // Avoid a second atomic load by invalidating instead of clearing.
@@ -1678,6 +1694,16 @@ static jboolean com_android_internal_os_Zygote_nativeDisableExecuteOnly(JNIEnv*
   return dl_iterate_phdr(disable_execute_only, nullptr) == 0;
 }
 
+static void com_android_internal_os_Zygote_nativeBlockSigTerm(JNIEnv* env, jclass) {
+  auto fail_fn = std::bind(ZygoteFailure, env, "usap", nullptr, _1);
+  BlockSignal(SIGTERM, fail_fn);
+}
+
+static void com_android_internal_os_Zygote_nativeUnblockSigTerm(JNIEnv* env, jclass) {
+  auto fail_fn = std::bind(ZygoteFailure, env, "usap", nullptr, _1);
+  UnblockSignal(SIGTERM, fail_fn);
+}
+
 static const JNINativeMethod gMethods[] = {
     { "nativeForkAndSpecialize",
       "(II[II[[IILjava/lang/String;Ljava/lang/String;[I[IZLjava/lang/String;Ljava/lang/String;)I",
@@ -1708,7 +1734,11 @@ static const JNINativeMethod gMethods[] = {
     { "nativeEmptyUsapPool", "()V",
       (void *) com_android_internal_os_Zygote_nativeEmptyUsapPool },
     { "nativeDisableExecuteOnly", "()Z",
-      (void *) com_android_internal_os_Zygote_nativeDisableExecuteOnly }
+      (void *) com_android_internal_os_Zygote_nativeDisableExecuteOnly },
+    { "nativeBlockSigTerm", "()V",
+      (void* ) com_android_internal_os_Zygote_nativeBlockSigTerm },
+    { "nativeUnblockSigTerm", "()V",
+      (void* ) com_android_internal_os_Zygote_nativeUnblockSigTerm }
 };
 
 int register_com_android_internal_os_Zygote(JNIEnv* env) {