From f68c4e2ffaec7128f0acd68319b0c0f20da7aaeb Mon Sep 17 00:00:00 2001 From: Chris Wailes Date: Wed, 5 Jun 2019 16:07:50 -0700 Subject: [PATCH] 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 fb329ba7c858bd73f5a57b2b700c5d042ab8675d) --- core/java/com/android/internal/os/Zygote.java | 168 ++++++++++-------- .../android/internal/os/ZygoteConnection.java | 5 +- core/jni/com_android_internal_os_Zygote.cpp | 52 ++++-- 3 files changed, 140 insertions(+), 85 deletions(-) diff --git a/core/java/com/android/internal/os/Zygote.java b/core/java/com/android/internal/os/Zygote.java index 95726834601a6..ee81868da25b9 100644 --- a/core/java/com/android/internal/os/Zygote.java +++ b/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,95 +540,116 @@ 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"); - - applyUidSecurityPolicy(args, peerCredentials); - applyDebuggerSystemProperty(args); - - int[][] rlimits = null; - - if (args.mRLimits != null) { - rlimits = args.mRLimits.toArray(INT_ARRAY_2D); - } - - // This must happen before the SELinux policy for this process is - // changed when specializing. try { - // Used by ZygoteProcess.zygoteSendArgsAndGetResult to fill in a - // Process.ProcessStartResult object. - usapOutputStream.writeInt(pid); - } catch (IOException ioEx) { - Log.e("USAP", "Failed to write response to session socket: " + ioEx.getMessage()); - System.exit(-1); - } finally { - IoUtils.closeQuietly(sessionSocket); + // 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); + + int[][] rlimits = null; + + if (args.mRLimits != null) { + rlimits = args.mRLimits.toArray(INT_ARRAY_2D); + } + + // This must happen before the SELinux policy for this process is + // changed when specializing. + try { + // Used by ZygoteProcess.zygoteSendArgsAndGetResult to fill in a + // Process.ProcessStartResult object. + usapOutputStream.writeInt(pid); + } catch (IOException ioEx) { + Log.e("USAP", "Failed to write response to session socket: " + + 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 LocalSocketImp.close(). Because the raw FD is created by init and then + // loaded from an environment variable (as opposed to being created by the + // 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"); + throw new RuntimeException(ex); + } + } try { - // This socket is closed using Os.close due to an issue with the implementation of - // LocalSocketImp.close. Because the raw FD is created by init and then loaded from - // an environment variable (as opposed to being created by the 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()); + ByteArrayOutputStream buffer = + new ByteArrayOutputStream(Zygote.USAP_MANAGEMENT_MESSAGE_BYTES); + 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 send/receive 8 bytes. + outputStream.writeLong(pid); + outputStream.flush(); + + Os.write(writePipe, buffer.toByteArray(), 0, buffer.size()); + } catch (Exception ex) { + Log.e("USAP", + String.format("Failed to write PID (%d) to pipe (%d): %s", + pid, writePipe.getInt$(), ex.getMessage())); + throw new RuntimeException(ex); + } finally { + IoUtils.closeQuietly(writePipe); } - } - try { - ByteArrayOutputStream buffer = - new ByteArrayOutputStream(Zygote.USAP_MANAGEMENT_MESSAGE_BYTES); - DataOutputStream outputStream = new DataOutputStream(buffer); + specializeAppProcess(args.mUid, args.mGid, args.mGids, + args.mRuntimeFlags, rlimits, args.mMountExternal, + args.mSeInfo, args.mNiceName, args.mStartChildZygote, + args.mInstructionSet, args.mAppDataDir); - // 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 - // send/receive 8 bytes. - outputStream.writeLong(pid); - outputStream.flush(); + disableExecuteOnly(args.mTargetSdkVersion); - Os.write(writePipe, buffer.toByteArray(), 0, buffer.size()); - } catch (Exception ex) { - Log.e("USAP", - String.format("Failed to write PID (%d) to pipe (%d): %s", - pid, writePipe.getInt$(), ex.getMessage())); - System.exit(-1); + 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 { - IoUtils.closeQuietly(writePipe); - } - - specializeAppProcess(args.mUid, args.mGid, args.mGids, - args.mRuntimeFlags, rlimits, args.mMountExternal, - args.mSeInfo, args.mNiceName, args.mStartChildZygote, - args.mInstructionSet, args.mAppDataDir); - - disableExecuteOnly(args.mTargetSdkVersion); - - // End of the postFork event. - Trace.traceEnd(Trace.TRACE_TAG_ACTIVITY_MANAGER); - - return ZygoteInit.zygoteInit(args.mTargetSdkVersion, - args.mRemainingArgs, - null /* classLoader */); - } - - static void setAppProcessName(ZygoteArguments args, String loggingTag) { - if (args.mNiceName != null) { - Process.setArgV0(args.mNiceName); - } else if (args.mPackageName != null) { - Process.setArgV0(args.mPackageName); - } else { - Log.w(loggingTag, "Unable to set package name."); + // Unblock SIGTERM to restore the process to default behavior. + unblockSigTerm(); } } - + + private static void blockSigTerm() { + nativeBlockSigTerm(); + } + + 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: "; /** diff --git a/core/java/com/android/internal/os/ZygoteConnection.java b/core/java/com/android/internal/os/ZygoteConnection.java index 885f522cd22b0..78f82ea100a4f 100644 --- a/core/java/com/android/internal/os/ZygoteConnection.java +++ b/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) { diff --git a/core/jni/com_android_internal_os_Zygote.cpp b/core/jni/com_android_internal_os_Zygote.cpp index 8ff16912e932b..4783a257755fb 100644 --- a/core/jni/com_android_internal_os_Zygote.cpp +++ b/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 - * return true. + * In all three cases the goal of the caller has been met, but only in + * 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) {