From be47df7395d1ccf1b35f33f963ca0c69a1036de0 Mon Sep 17 00:00:00 2001 From: daqi Date: Tue, 5 Sep 2017 09:53:05 +0800 Subject: [PATCH] Fix TrustedCredentialsSettings NPE [Cause of Defect] TrustedCredentialsSettings#mKeyChainConnectionByProfileId is get/set by more than one thread. Main thread would set it in onDestroy method, and AsyncTask would get and set in the doInBackground method. So mKeyChainConnectionByProfileId.get(profileId) would get null after onDestroy method get called. Bug: N/A Test: Debugger to simulate concurrency Change-Id: I0664d1e9b88b079855354ce0e6fe014a98a22327 Signed-off-by: daqi --- .../settings/TrustedCredentialsSettings.java | 162 +++++++++++---------- 1 file changed, 86 insertions(+), 76 deletions(-) diff --git a/src/com/android/settings/TrustedCredentialsSettings.java b/src/com/android/settings/TrustedCredentialsSettings.java index 4696dd317b..587e814e2f 100644 --- a/src/com/android/settings/TrustedCredentialsSettings.java +++ b/src/com/android/settings/TrustedCredentialsSettings.java @@ -61,6 +61,7 @@ import android.widget.Switch; import android.widget.TabHost; import android.widget.TextView; +import com.android.internal.annotations.GuardedBy; import com.android.internal.app.UnlaunchableAppActivity; import com.android.internal.logging.nano.MetricsProto.MetricsEvent; import com.android.internal.widget.LockPatternUtils; @@ -152,6 +153,7 @@ public class TrustedCredentialsSettings extends OptionsMenuFragment private int mConfirmingCredentialUser; private IntConsumer mConfirmingCredentialListener; private Set mAliasLoaders = new ArraySet(2); + @GuardedBy("mKeyChainConnectionByProfileId") private final SparseArray mKeyChainConnectionByProfileId = new SparseArray(); @@ -256,11 +258,13 @@ public class TrustedCredentialsSettings extends OptionsMenuFragment } private void closeKeyChainConnections() { - final int n = mKeyChainConnectionByProfileId.size(); - for (int i = 0; i < n; ++i) { - mKeyChainConnectionByProfileId.valueAt(i).close(); + synchronized (mKeyChainConnectionByProfileId) { + final int n = mKeyChainConnectionByProfileId.size(); + for (int i = 0; i < n; ++i) { + mKeyChainConnectionByProfileId.valueAt(i).close(); + } + mKeyChainConnectionByProfileId.clear(); } - mKeyChainConnectionByProfileId.clear(); } private void addTab(Tab tab) { @@ -684,62 +688,64 @@ public class TrustedCredentialsSettings extends OptionsMenuFragment SparseArray> certHoldersByProfile = new SparseArray>(); try { - List profiles = mUserManager.getUserProfiles(); - final int n = profiles.size(); - // First we get all aliases for all profiles in order to show progress - // correctly. Otherwise this could all be in a single loop. - SparseArray> aliasesByProfileId = new SparseArray< - List>(n); - int max = 0; - int progress = 0; - for (int i = 0; i < n; ++i) { - UserHandle profile = profiles.get(i); - int profileId = profile.getIdentifier(); - if (shouldSkipProfile(profile)) { - continue; - } - KeyChainConnection keyChainConnection = KeyChain.bindAsUser(mContext, - profile); - // Saving the connection for later use on the certificate dialog. - mKeyChainConnectionByProfileId.put(profileId, keyChainConnection); - IKeyChainService service = keyChainConnection.getService(); - List aliases = mTab.getAliases(service); - if (isCancelled()) { - return new SparseArray>(); - } - max += aliases.size(); - aliasesByProfileId.put(profileId, aliases); - } - for (int i = 0; i < n; ++i) { - UserHandle profile = profiles.get(i); - int profileId = profile.getIdentifier(); - List aliases = aliasesByProfileId.get(profileId); - if (isCancelled()) { - return new SparseArray>(); - } - KeyChainConnection keyChainConnection = mKeyChainConnectionByProfileId.get( - profileId); - if (shouldSkipProfile(profile) || aliases == null - || keyChainConnection == null) { - certHoldersByProfile.put(profileId, new ArrayList(0)); - continue; + synchronized(mKeyChainConnectionByProfileId) { + List profiles = mUserManager.getUserProfiles(); + final int n = profiles.size(); + // First we get all aliases for all profiles in order to show progress + // correctly. Otherwise this could all be in a single loop. + SparseArray> aliasesByProfileId = new SparseArray< + List>(n); + int max = 0; + int progress = 0; + for (int i = 0; i < n; ++i) { + UserHandle profile = profiles.get(i); + int profileId = profile.getIdentifier(); + if (shouldSkipProfile(profile)) { + continue; + } + KeyChainConnection keyChainConnection = KeyChain.bindAsUser(mContext, + profile); + // Saving the connection for later use on the certificate dialog. + mKeyChainConnectionByProfileId.put(profileId, keyChainConnection); + IKeyChainService service = keyChainConnection.getService(); + List aliases = mTab.getAliases(service); + if (isCancelled()) { + return new SparseArray>(); + } + max += aliases.size(); + aliasesByProfileId.put(profileId, aliases); } - IKeyChainService service = keyChainConnection.getService(); - List certHolders = new ArrayList(max); - final int aliasMax = aliases.size(); - for (int j = 0; j < aliasMax; ++j) { - String alias = aliases.get(j); - byte[] encodedCertificate = service.getEncodedCaCertificate(alias, - true); - X509Certificate cert = KeyChain.toCertificate(encodedCertificate); - certHolders.add(new CertHolder(service, mAdapter, - mTab, alias, cert, profileId)); - publishProgress(++progress, max); + for (int i = 0; i < n; ++i) { + UserHandle profile = profiles.get(i); + int profileId = profile.getIdentifier(); + List aliases = aliasesByProfileId.get(profileId); + if (isCancelled()) { + return new SparseArray>(); + } + KeyChainConnection keyChainConnection = mKeyChainConnectionByProfileId.get( + profileId); + if (shouldSkipProfile(profile) || aliases == null + || keyChainConnection == null) { + certHoldersByProfile.put(profileId, new ArrayList(0)); + continue; + } + IKeyChainService service = keyChainConnection.getService(); + List certHolders = new ArrayList(max); + final int aliasMax = aliases.size(); + for (int j = 0; j < aliasMax; ++j) { + String alias = aliases.get(j); + byte[] encodedCertificate = service.getEncodedCaCertificate(alias, + true); + X509Certificate cert = KeyChain.toCertificate(encodedCertificate); + certHolders.add(new CertHolder(service, mAdapter, + mTab, alias, cert, profileId)); + publishProgress(++progress, max); + } + Collections.sort(certHolders); + certHoldersByProfile.put(profileId, certHolders); } - Collections.sort(certHolders); - certHoldersByProfile.put(profileId, certHolders); + return certHoldersByProfile; } - return certHoldersByProfile; } catch (RemoteException e) { Log.e(TAG, "Remote exception while loading aliases.", e); return new SparseArray>(); @@ -936,16 +942,18 @@ public class TrustedCredentialsSettings extends OptionsMenuFragment public List getX509CertsFromCertHolder(CertHolder certHolder) { List certificates = null; try { - KeyChainConnection keyChainConnection = mKeyChainConnectionByProfileId.get( - certHolder.mProfileId); - IKeyChainService service = keyChainConnection.getService(); - List chain = service.getCaCertificateChainAliases(certHolder.mAlias, true); - final int n = chain.size(); - certificates = new ArrayList(n); - for (int i = 0; i < n; ++i) { - byte[] encodedCertificate = service.getEncodedCaCertificate(chain.get(i), true); - X509Certificate certificate = KeyChain.toCertificate(encodedCertificate); - certificates.add(certificate); + synchronized (mKeyChainConnectionByProfileId) { + KeyChainConnection keyChainConnection = mKeyChainConnectionByProfileId.get( + certHolder.mProfileId); + IKeyChainService service = keyChainConnection.getService(); + List chain = service.getCaCertificateChainAliases(certHolder.mAlias, true); + final int n = chain.size(); + certificates = new ArrayList(n); + for (int i = 0; i < n; ++i) { + byte[] encodedCertificate = service.getEncodedCaCertificate(chain.get(i), true); + X509Certificate certificate = KeyChain.toCertificate(encodedCertificate); + certificates.add(certificate); + } } } catch (RemoteException ex) { Log.e(TAG, "RemoteException while retrieving certificate chain for root " @@ -985,15 +993,17 @@ public class TrustedCredentialsSettings extends OptionsMenuFragment @Override protected Boolean doInBackground(Void... params) { try { - KeyChainConnection keyChainConnection = mKeyChainConnectionByProfileId.get( - mCertHolder.mProfileId); - IKeyChainService service = keyChainConnection.getService(); - if (mCertHolder.mDeleted) { - byte[] bytes = mCertHolder.mX509Cert.getEncoded(); - service.installCaCertificate(bytes); - return true; - } else { - return service.deleteCaCertificate(mCertHolder.mAlias); + synchronized (mKeyChainConnectionByProfileId) { + KeyChainConnection keyChainConnection = mKeyChainConnectionByProfileId.get( + mCertHolder.mProfileId); + IKeyChainService service = keyChainConnection.getService(); + if (mCertHolder.mDeleted) { + byte[] bytes = mCertHolder.mX509Cert.getEncoded(); + service.installCaCertificate(bytes); + return true; + } else { + return service.deleteCaCertificate(mCertHolder.mAlias); + } } } catch (CertificateEncodingException | SecurityException | IllegalStateException | RemoteException e) { -- 2.11.0