From 5855ce28cff87ee35e0533c840fbce3ec5690e27 Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Tue, 28 May 2019 17:29:11 +0900 Subject: [PATCH 1/2] Proper handling of NetworkStack crash Instead of always crashing on userdebug builds, do the following on all builds: - If the device did not observe a NetworkStack crash in the last 6h crash the system server. This is to handle spurious crashes of the NetworkStack, so that the system can recover instead of staying without connectivity. - Otherwise, the device has had a recent crash. Notify the listeners (watchdog listener to be added) that something is seriously wrong, but do not crash to avoid bootlooping the device. This allows the watchdog to do its job, and avoids bricking the device in situations where the user could need to make emergency calls. Bug: 133725814 Test: Killed the network stack, observe reboot of framework. Test: Kill network stack again, observe nothing. Test: Wipe data, install new network stack, kill it twice: rollback. (with patch on top applied) Change-Id: If1fa00bed769eb60ca4832609006bdf15ceddb80 --- .../java/android/net/NetworkStackClient.java | 129 ++++++++++++++++-- 1 file changed, 115 insertions(+), 14 deletions(-) diff --git a/services/net/java/android/net/NetworkStackClient.java b/services/net/java/android/net/NetworkStackClient.java index 6b5842ff90656..09c9b6d360a9d 100644 --- a/services/net/java/android/net/NetworkStackClient.java +++ b/services/net/java/android/net/NetworkStackClient.java @@ -26,22 +26,27 @@ import android.content.ComponentName; import android.content.Context; import android.content.Intent; import android.content.ServiceConnection; +import android.content.SharedPreferences; import android.content.pm.PackageManager; import android.net.dhcp.DhcpServingParamsParcel; import android.net.dhcp.IDhcpServerCallbacks; import android.net.ip.IIpClientCallbacks; import android.net.util.SharedLog; import android.os.Binder; -import android.os.Build; +import android.os.Environment; import android.os.IBinder; import android.os.Process; import android.os.RemoteException; import android.os.ServiceManager; +import android.os.SystemClock; import android.os.UserHandle; +import android.provider.DeviceConfig; +import android.util.ArraySet; import android.util.Slog; import com.android.internal.annotations.GuardedBy; +import java.io.File; import java.io.PrintWriter; import java.util.ArrayList; @@ -54,6 +59,15 @@ public class NetworkStackClient { private static final int NETWORKSTACK_TIMEOUT_MS = 10_000; private static final String IN_PROCESS_SUFFIX = ".InProcess"; + private static final String PREFS_FILE = "NetworkStackClientPrefs.xml"; + private static final String PREF_KEY_LAST_CRASH_UPTIME = "lastcrash"; + private static final String CONFIG_MIN_CRASH_INTERVAL_MS = "min_crash_interval"; + + // Even if the network stack is lost, do not crash the system more often than this. + // Connectivity would be broken, but if the user needs the device for something urgent + // (like calling emergency services) we should not bootloop the device. + // This is the default value: the actual value can be adjusted via device config. + private static final long DEFAULT_MIN_CRASH_INTERVAL_MS = 6 * 3_600_000L; private static NetworkStackClient sInstance; @@ -67,12 +81,34 @@ public class NetworkStackClient { @GuardedBy("mLog") private final SharedLog mLog = new SharedLog(TAG); - private volatile boolean mNetworkStackStartRequested = false; + private volatile boolean mWasSystemServerInitialized = false; + + /** + * If non-zero, indicates that the last framework start happened after a crash of the + * NetworkStack which was at the specified uptime. + */ + private volatile long mLastCrashUptime = 0L; + + @GuardedBy("mHealthListeners") + private final ArraySet mHealthListeners = new ArraySet<>(); private interface NetworkStackCallback { void onNetworkStackConnected(INetworkStackConnector connector); } + /** + * Callback interface for severe failures of the NetworkStack. + * + *

Useful for health monitors such as PackageWatchdog. + */ + public interface NetworkStackHealthListener { + /** + * Called when there is a severe failure of the network stack. + * @param packageName Package name of the network stack. + */ + void onNetworkStackFailure(@NonNull String packageName); + } + private NetworkStackClient() { } /** @@ -85,6 +121,15 @@ public class NetworkStackClient { return sInstance; } + /** + * Add a {@link NetworkStackHealthListener} to listen to network stack health events. + */ + public void registerHealthListener(@NonNull NetworkStackHealthListener listener) { + synchronized (mHealthListeners) { + mHealthListeners.add(listener); + } + } + /** * Create a DHCP server according to the specified parameters. * @@ -147,6 +192,16 @@ public class NetworkStackClient { } private class NetworkStackConnection implements ServiceConnection { + @NonNull + private final Context mContext; + @NonNull + private final String mPackageName; + + private NetworkStackConnection(@NonNull Context context, @NonNull String packageName) { + mContext = context; + mPackageName = packageName; + } + @Override public void onServiceConnected(ComponentName name, IBinder service) { logi("Network stack service connected"); @@ -155,14 +210,14 @@ public class NetworkStackClient { @Override public void onServiceDisconnected(ComponentName name) { - // The system has lost its network stack (probably due to a crash in the - // network stack process): better crash rather than stay in a bad state where all - // networking is broken. // onServiceDisconnected is not being called on device shutdown, so this method being // called always indicates a bad state for the system server. - maybeCrashWithTerribleFailure("Lost network stack"); + // This code path is only run by the system server: only the system server binds + // to the NetworkStack as a service. Other processes get the NetworkStack from + // the ServiceManager. + maybeCrashWithTerribleFailure("Lost network stack", mContext, mPackageName); } - }; + } private void registerNetworkStackService(@NonNull IBinder service) { final INetworkStackConnector connector = INetworkStackConnector.Stub.asInterface(service); @@ -189,7 +244,7 @@ public class NetworkStackClient { */ public void init() { log("Network stack init"); - mNetworkStackStartRequested = true; + mWasSystemServerInitialized = true; } /** @@ -202,6 +257,13 @@ public class NetworkStackClient { */ public void start(Context context) { log("Starting network stack"); + + final SharedPreferences prefs = getSharedPreferences(context); + mLastCrashUptime = prefs.getLong(PREF_KEY_LAST_CRASH_UPTIME, 0L); + // Remove the preference after getting the last crash uptime, so mLastCrashUptime always + // indicates this is the first start since the last crash. + prefs.edit().remove(PREF_KEY_LAST_CRASH_UPTIME).commit(); + final PackageManager pm = context.getPackageManager(); // Try to bind in-process if the device was shipped with an in-process version @@ -216,16 +278,19 @@ public class NetworkStackClient { } if (intent == null) { - maybeCrashWithTerribleFailure("Could not resolve the network stack"); + maybeCrashWithTerribleFailure("Could not resolve the network stack", context, null); return; } + final String packageName = intent.getComponent().getPackageName(); + // Start the network stack. The service will be added to the service manager in // NetworkStackConnection.onServiceConnected(). - if (!context.bindServiceAsUser(intent, new NetworkStackConnection(), + if (!context.bindServiceAsUser(intent, new NetworkStackConnection(context, packageName), Context.BIND_AUTO_CREATE | Context.BIND_IMPORTANT, UserHandle.SYSTEM)) { maybeCrashWithTerribleFailure( - "Could not bind to network stack in-process, or in app with " + intent); + "Could not bind to network stack in-process, or in app with " + intent, + context, packageName); return; } @@ -274,11 +339,47 @@ public class NetworkStackClient { } } - private void maybeCrashWithTerribleFailure(@NonNull String message) { + private void maybeCrashWithTerribleFailure(@NonNull String message, + @NonNull Context context, @Nullable String packageName) { logWtf(message, null); - if (Build.IS_DEBUGGABLE) { + // uptime is monotonic even after a framework restart + final long uptime = SystemClock.elapsedRealtime(); + final long minCrashIntervalMs = DeviceConfig.getLong(DeviceConfig.NAMESPACE_CONNECTIVITY, + CONFIG_MIN_CRASH_INTERVAL_MS, DEFAULT_MIN_CRASH_INTERVAL_MS); + + // Either the framework was not restarted after a crash of the NetworkStack, or the min + // crash interval has passed since then. + if (mLastCrashUptime == 0L || uptime - mLastCrashUptime > minCrashIntervalMs) { + // The system is not bound to its network stack (for example due to a crash in the + // network stack process): better crash rather than stay in a bad state where all + // networking is broken. + // Using device-encrypted SharedPreferences as DeviceConfig does not have a synchronous + // API to persist settings before a crash. + final SharedPreferences prefs = getSharedPreferences(context); + if (!prefs.edit().putLong(PREF_KEY_LAST_CRASH_UPTIME, uptime).commit()) { + logWtf("Could not persist last crash uptime", null); + } throw new IllegalStateException(message); } + + // Here the system crashed recently already. Inform listeners that something is + // definitely wrong. + if (packageName != null) { + final ArraySet listeners; + synchronized (mHealthListeners) { + listeners = new ArraySet<>(mHealthListeners); + } + for (NetworkStackHealthListener listener : listeners) { + listener.onNetworkStackFailure(packageName); + } + } + } + + private SharedPreferences getSharedPreferences(Context context) { + final File prefsFile = new File( + Environment.getDataSystemDeDirectory(UserHandle.USER_SYSTEM), PREFS_FILE); + return context.createDeviceProtectedStorageContext() + .getSharedPreferences(prefsFile, Context.MODE_PRIVATE); } /** @@ -350,7 +451,7 @@ public class NetworkStackClient { "Only the system server should try to bind to the network stack."); } - if (!mNetworkStackStartRequested) { + if (!mWasSystemServerInitialized) { // The network stack is not being started in this process, e.g. this process is not // the system server. Get a remote connector registered by the system server. final INetworkStackConnector connector = getRemoteConnector(); From 6a135107ff27e09c71723d7aed0517b2571d4c81 Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Wed, 29 May 2019 18:30:27 +0900 Subject: [PATCH 2/2] PackageWatchdog listens for NetworkStack failures In addition to the NetworkStack app monitoring, have PackageWatchdog register an observer to NetworkStackClient to receive severe failure notifications, and attempt a rollback if available. The callback is registered in onPackagesReady(), which is called in the boot sequence just before starting the NetworkStack. Test: installed new networkstack, killed it twice, observe rollback Test: unit test in change on top Bug: 133725814 Change-Id: I2cb4200b78c2482cacc4bfe2ace1581b869be512 --- .../com/android/server/PackageWatchdog.java | 58 +++++++++++++++---- tests/PackageWatchdog/Android.bp | 7 +++ .../android/server/PackageWatchdogTest.java | 9 ++- 3 files changed, 63 insertions(+), 11 deletions(-) diff --git a/services/core/java/com/android/server/PackageWatchdog.java b/services/core/java/com/android/server/PackageWatchdog.java index 32671144aee39..dee89e5d5c4d5 100644 --- a/services/core/java/com/android/server/PackageWatchdog.java +++ b/services/core/java/com/android/server/PackageWatchdog.java @@ -25,6 +25,7 @@ import android.annotation.Nullable; import android.content.Context; import android.content.pm.PackageManager; import android.content.pm.VersionedPackage; +import android.net.NetworkStackClient; import android.os.Environment; import android.os.Handler; import android.os.Looper; @@ -115,6 +116,7 @@ public class PackageWatchdog { // File containing the XML data of monitored packages /data/system/package-watchdog.xml private final AtomicFile mPolicyFile; private final ExplicitHealthCheckController mHealthCheckController; + private final NetworkStackClient mNetworkStackClient; @GuardedBy("mLock") private boolean mIsPackagesReady; // Flag to control whether explicit health checks are supported or not @@ -135,7 +137,8 @@ public class PackageWatchdog { new File(new File(Environment.getDataDirectory(), "system"), "package-watchdog.xml")), new Handler(Looper.myLooper()), BackgroundThread.getHandler(), - new ExplicitHealthCheckController(context)); + new ExplicitHealthCheckController(context), + NetworkStackClient.getInstance()); } /** @@ -143,12 +146,14 @@ public class PackageWatchdog { */ @VisibleForTesting PackageWatchdog(Context context, AtomicFile policyFile, Handler shortTaskHandler, - Handler longTaskHandler, ExplicitHealthCheckController controller) { + Handler longTaskHandler, ExplicitHealthCheckController controller, + NetworkStackClient networkStackClient) { mContext = context; mPolicyFile = policyFile; mShortTaskHandler = shortTaskHandler; mLongTaskHandler = longTaskHandler; mHealthCheckController = controller; + mNetworkStackClient = networkStackClient; loadFromFile(); } @@ -174,6 +179,7 @@ public class PackageWatchdog { () -> syncRequestsAsync()); setPropertyChangedListenerLocked(); updateConfigs(); + registerNetworkStackHealthListener(); } } @@ -630,29 +636,40 @@ public class PackageWatchdog { synchronized (mLock) { PackageHealthObserver registeredObserver = observer.mRegisteredObserver; if (registeredObserver != null) { - PackageManager pm = mContext.getPackageManager(); Iterator it = failedPackages.iterator(); while (it.hasNext()) { String failedPackage = it.next().getName(); - long versionCode = 0; Slog.i(TAG, "Explicit health check failed for package " + failedPackage); - try { - versionCode = pm.getPackageInfo( - failedPackage, 0 /* flags */).getLongVersionCode(); - } catch (PackageManager.NameNotFoundException e) { + VersionedPackage versionedPkg = getVersionedPackage(failedPackage); + if (versionedPkg == null) { Slog.w(TAG, "Explicit health check failed but could not find package " + failedPackage); // TODO(b/120598832): Skip. We only continue to pass tests for now since // the tests don't install any packages + versionedPkg = new VersionedPackage(failedPackage, 0L); } - registeredObserver.execute( - new VersionedPackage(failedPackage, versionCode)); + registeredObserver.execute(versionedPkg); } } } }); } + @Nullable + private VersionedPackage getVersionedPackage(String packageName) { + final PackageManager pm = mContext.getPackageManager(); + if (pm == null) { + return null; + } + try { + final long versionCode = pm.getPackageInfo( + packageName, 0 /* flags */).getLongVersionCode(); + return new VersionedPackage(packageName, versionCode); + } catch (PackageManager.NameNotFoundException e) { + return null; + } + } + /** * Loads mAllObservers from file. * @@ -726,6 +743,27 @@ public class PackageWatchdog { } } + private void registerNetworkStackHealthListener() { + // TODO: have an internal method to trigger a rollback by reporting high severity errors, + // and rely on ActivityManager to inform the watchdog of severe network stack crashes + // instead of having this listener in parallel. + mNetworkStackClient.registerHealthListener( + packageName -> { + final VersionedPackage pkg = getVersionedPackage(packageName); + if (pkg == null) { + Slog.wtf(TAG, "NetworkStack failed but could not find its package"); + return; + } + // This is a severe failure and recovery should be attempted immediately. + // TODO: have a better way to handle such failures. + final List pkgList = Collections.singletonList(pkg); + final long failureCount = getTriggerFailureCount(); + for (int i = 0; i < failureCount; i++) { + onPackageFailure(pkgList); + } + }); + } + /** * Persists mAllObservers to file. Threshold information is ignored. */ diff --git a/tests/PackageWatchdog/Android.bp b/tests/PackageWatchdog/Android.bp index b07996568839e..88d92c4d94fe8 100644 --- a/tests/PackageWatchdog/Android.bp +++ b/tests/PackageWatchdog/Android.bp @@ -18,11 +18,18 @@ android_test { srcs: ["src/**/*.java"], static_libs: [ "junit", + "mockito-target-extended-minus-junit4", "frameworks-base-testutils", "androidx.test.rules", "services.core", + "services.net", ], libs: ["android.test.runner"], + jni_libs: [ + // mockito-target-extended dependencies + "libdexmakerjvmtiagent", + "libstaticjvmtiagent", + ], platform_apis: true, test_suites: ["device-tests"], } diff --git a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java index eb19361d86a3c..97716377ea2e2 100644 --- a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java +++ b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java @@ -27,6 +27,7 @@ import static org.junit.Assert.fail; import android.Manifest; import android.content.Context; import android.content.pm.VersionedPackage; +import android.net.NetworkStackClient; import android.os.Handler; import android.os.test.TestLooper; import android.provider.DeviceConfig; @@ -41,6 +42,8 @@ import com.android.server.PackageWatchdog.PackageHealthObserverImpact; import org.junit.After; import org.junit.Before; import org.junit.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; import java.io.File; import java.util.ArrayList; @@ -70,9 +73,12 @@ public class PackageWatchdogTest { private static final long SHORT_DURATION = TimeUnit.SECONDS.toMillis(1); private static final long LONG_DURATION = TimeUnit.SECONDS.toMillis(5); private TestLooper mTestLooper; + @Mock + private NetworkStackClient mNetworkStackClient; @Before public void setUp() throws Exception { + MockitoAnnotations.initMocks(this); new File(InstrumentationRegistry.getContext().getFilesDir(), "package-watchdog.xml").delete(); adoptShellPermissions(Manifest.permission.READ_DEVICE_CONFIG); @@ -732,7 +738,8 @@ public class PackageWatchdogTest { new AtomicFile(new File(context.getFilesDir(), "package-watchdog.xml")); Handler handler = new Handler(mTestLooper.getLooper()); PackageWatchdog watchdog = - new PackageWatchdog(context, policyFile, handler, handler, controller); + new PackageWatchdog(context, policyFile, handler, handler, controller, + mNetworkStackClient); // Verify controller is not automatically started assertFalse(controller.mIsEnabled); if (withPackagesReady) {