Fixes two data races in USAP pool management.

The USAP pool management code contained two data races.  One could cause
a double decrement if a runtime thread executed the SIGCHLD handler
while the server was responding to a pool exit message from a USAP.  The
other data race could occur when the SIGCHLD handler executed in the
middle of a USAP pool flush.

The solution to the first race is to change the return value from a
helper function to ensure that the decrement only occurs when the entry
is invalidated through that specific invocation of the helper.

The second data race was fixed by using SIGTERM instead of SIGKILL when
flushing the USAP pool.  This allows the Zygote to clear the table
entries outside of the SIGCHLD handler, and the handler to avoid
duplicate bookkeeping cleanup when this occurs.  SIGTERM is used so that
it can be differentiated from other process termination events and so
that it can be blocked while the USAP is specializing, but hasn't yet
informed the Zygote of it's removal from the pool.  This issue and this
fix will no longer be necessary once the Zygote signal handler has been
replaced with a signalfd.

Bug: 132794985
Test: atest SignedConfigHostTest
Change-Id: Ie01637a10b356b80b5aa62291a97f2c167242827
Merged-In: Ie01637a10b356b80b5aa62291a97f2c167242827
(cherry picked from commit fb329ba7c8)

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) {