From 129aa44896dfe7dc5c26866aa8aace019d6a3b9c Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Thu, 22 Apr 2021 19:03:21 +0800 Subject: Add a synchronized lock into the SplitDependencyLoaderImpl - There's a potential race condition going on when components from the isolated split are creating the split class loader. Add a synchronized lock for the array of cached class loaders and resource paths to prevent the race. - Remove the synchronized class reference of LoadedApk. Using an explicit mLock object to synchronize the #createOrUpdateClassLoaderLocked() and cached split class loader instead. Bug: 172602571 Test: atest IsolatedSplitsTests Test: atest ClassloaderSplitsTest Test: atest SplitTests Change-Id: Ifb77d067ddf06b5192d7ce9d4e78958b56faac7c --- core/java/android/app/LoadedApk.java | 65 ++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 25 deletions(-) diff --git a/core/java/android/app/LoadedApk.java b/core/java/android/app/LoadedApk.java index 83d0246744df..ea6c87493316 100644 --- a/core/java/android/app/LoadedApk.java +++ b/core/java/android/app/LoadedApk.java @@ -58,6 +58,7 @@ import android.util.Slog; import android.util.SparseArray; import android.view.DisplayAdjustments; +import com.android.internal.annotations.GuardedBy; import com.android.internal.util.ArrayUtils; import dalvik.system.BaseDexClassLoader; @@ -156,6 +157,7 @@ public final class LoadedApk { private final ArrayMap> mUnboundServices = new ArrayMap<>(); private AppComponentFactory mAppComponentFactory; + private final Object mLock = new Object(); Application getApplication() { return mApplication; @@ -354,7 +356,7 @@ public final class LoadedApk { } else { addedPaths.addAll(newPaths); } - synchronized (this) { + synchronized (mLock) { createOrUpdateClassLoaderLocked(addedPaths); if (mResources != null) { final String[] splitPaths; @@ -589,7 +591,9 @@ public final class LoadedApk { * include the base APK in the list of splits. */ private class SplitDependencyLoaderImpl extends SplitDependencyLoader { + @GuardedBy("mLock") private final String[][] mCachedResourcePaths; + @GuardedBy("mLock") private final ClassLoader[] mCachedClassLoaders; SplitDependencyLoaderImpl(@NonNull SparseArray dependencies) { @@ -600,37 +604,41 @@ public final class LoadedApk { @Override protected boolean isSplitCached(int splitIdx) { - return mCachedClassLoaders[splitIdx] != null; + synchronized (mLock) { + return mCachedClassLoaders[splitIdx] != null; + } } @Override protected void constructSplit(int splitIdx, @NonNull int[] configSplitIndices, int parentSplitIdx) throws NameNotFoundException { - final ArrayList splitPaths = new ArrayList<>(); - if (splitIdx == 0) { - createOrUpdateClassLoaderLocked(null); - mCachedClassLoaders[0] = mClassLoader; + synchronized (mLock) { + final ArrayList splitPaths = new ArrayList<>(); + if (splitIdx == 0) { + createOrUpdateClassLoaderLocked(null); + mCachedClassLoaders[0] = mClassLoader; + + // Never add the base resources here, they always get added no matter what. + for (int configSplitIdx : configSplitIndices) { + splitPaths.add(mSplitResDirs[configSplitIdx - 1]); + } + mCachedResourcePaths[0] = splitPaths.toArray(new String[splitPaths.size()]); + return; + } + + // Since we handled the special base case above, parentSplitIdx is always valid. + final ClassLoader parent = mCachedClassLoaders[parentSplitIdx]; + mCachedClassLoaders[splitIdx] = ApplicationLoaders.getDefault().getClassLoader( + mSplitAppDirs[splitIdx - 1], getTargetSdkVersion(), false, null, + null, parent, mSplitClassLoaderNames[splitIdx - 1]); - // Never add the base resources here, they always get added no matter what. + Collections.addAll(splitPaths, mCachedResourcePaths[parentSplitIdx]); + splitPaths.add(mSplitResDirs[splitIdx - 1]); for (int configSplitIdx : configSplitIndices) { splitPaths.add(mSplitResDirs[configSplitIdx - 1]); } - mCachedResourcePaths[0] = splitPaths.toArray(new String[splitPaths.size()]); - return; + mCachedResourcePaths[splitIdx] = splitPaths.toArray(new String[splitPaths.size()]); } - - // Since we handled the special base case above, parentSplitIdx is always valid. - final ClassLoader parent = mCachedClassLoaders[parentSplitIdx]; - mCachedClassLoaders[splitIdx] = ApplicationLoaders.getDefault().getClassLoader( - mSplitAppDirs[splitIdx - 1], getTargetSdkVersion(), false, null, null, parent, - mSplitClassLoaderNames[splitIdx - 1]); - - Collections.addAll(splitPaths, mCachedResourcePaths[parentSplitIdx]); - splitPaths.add(mSplitResDirs[splitIdx - 1]); - for (int configSplitIdx : configSplitIndices) { - splitPaths.add(mSplitResDirs[configSplitIdx - 1]); - } - mCachedResourcePaths[splitIdx] = splitPaths.toArray(new String[splitPaths.size()]); } private int ensureSplitLoaded(String splitName) throws NameNotFoundException { @@ -648,11 +656,17 @@ public final class LoadedApk { } ClassLoader getClassLoaderForSplit(String splitName) throws NameNotFoundException { - return mCachedClassLoaders[ensureSplitLoaded(splitName)]; + final int idx = ensureSplitLoaded(splitName); + synchronized (mLock) { + return mCachedClassLoaders[idx]; + } } String[] getSplitPathsForSplit(String splitName) throws NameNotFoundException { - return mCachedResourcePaths[ensureSplitLoaded(splitName)]; + final int idx = ensureSplitLoaded(splitName); + synchronized (mLock) { + return mCachedResourcePaths[idx]; + } } } @@ -749,6 +763,7 @@ public final class LoadedApk { } } + @GuardedBy("mLock") private void createOrUpdateClassLoaderLocked(List addedPaths) { if (mPackageName.equals("android")) { // Note: This branch is taken for system server and we don't need to setup @@ -1023,7 +1038,7 @@ public final class LoadedApk { @UnsupportedAppUsage public ClassLoader getClassLoader() { - synchronized (this) { + synchronized (mLock) { if (mClassLoader == null) { createOrUpdateClassLoaderLocked(null /*addedPaths*/); } -- cgit v1.2.3-59-g8ed1b