From 14de6500387eba13a537a67807b91c5b89b36b05 Mon Sep 17 00:00:00 2001 From: Atneya Nair Date: Wed, 5 Apr 2023 11:19:40 -0700 Subject: [PATCH 1/2] Protect STHelper from calls after detach Now that we expose detach to upper layers, we should ensure that calling a method on a detached STHelper strongly errors. Fixes: 277390523 Bug: 272147641 Test: atest AlwaysOnHotwordDetectorTest#isSessionInvalid_afterDetach Change-Id: I32cf30d711e27debed0d643a9627663830e2ccf6 --- .../soundtrigger/SoundTriggerHelper.java | 94 ++++++++++++++----- 1 file changed, 70 insertions(+), 24 deletions(-) diff --git a/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerHelper.java b/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerHelper.java index 07dc1c66bc4df..18d0c5a2d05f4 100644 --- a/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerHelper.java +++ b/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerHelper.java @@ -49,6 +49,7 @@ import android.telephony.PhoneStateListener; import android.telephony.TelephonyManager; import android.util.Slog; +import com.android.internal.annotations.GuardedBy; import com.android.internal.logging.MetricsLogger; import java.io.FileDescriptor; @@ -129,6 +130,9 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { private final Function mModuleProvider; private final Supplier> mModulePropertiesProvider; + @GuardedBy("mLock") + private boolean mIsDetached = false; + SoundTriggerHelper(Context context, @NonNull Function moduleProvider, int moduleId, @@ -184,7 +188,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { * recognition. * @return One of {@link #STATUS_ERROR} or {@link #STATUS_OK}. */ - int startGenericRecognition(UUID modelId, GenericSoundModel soundModel, + public int startGenericRecognition(UUID modelId, GenericSoundModel soundModel, IRecognitionStatusCallback callback, RecognitionConfig recognitionConfig, boolean runInBatterySaverMode) { MetricsLogger.count(mContext, "sth_start_recognition", 1); @@ -195,6 +199,9 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { } synchronized (mLock) { + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } ModelData modelData = getOrCreateGenericModelDataLocked(modelId); if (modelData == null) { Slog.w(TAG, "Irrecoverable error occurred, check UUID / sound model data."); @@ -214,7 +221,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { * @param callback The callback for the recognition events related to the given keyphrase. * @return One of {@link #STATUS_ERROR} or {@link #STATUS_OK}. */ - int startKeyphraseRecognition(int keyphraseId, KeyphraseSoundModel soundModel, + public int startKeyphraseRecognition(int keyphraseId, KeyphraseSoundModel soundModel, IRecognitionStatusCallback callback, RecognitionConfig recognitionConfig, boolean runInBatterySaverMode) { synchronized (mLock) { @@ -223,6 +230,10 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { return STATUS_ERROR; } + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } + if (DBG) { Slog.d(TAG, "startKeyphraseRecognition for keyphraseId=" + keyphraseId + " soundModel=" + soundModel + ", callback=" + callback.asBinder() @@ -311,7 +322,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { * for the recognition. * @return One of {@link #STATUS_ERROR} or {@link #STATUS_OK}. */ - int startRecognition(SoundModel soundModel, ModelData modelData, + private int startRecognition(SoundModel soundModel, ModelData modelData, IRecognitionStatusCallback callback, RecognitionConfig recognitionConfig, int keyphraseId, boolean runInBatterySaverMode) { synchronized (mLock) { @@ -385,7 +396,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { * * @return One of {@link #STATUS_ERROR} or {@link #STATUS_OK}. */ - int stopGenericRecognition(UUID modelId, IRecognitionStatusCallback callback) { + public int stopGenericRecognition(UUID modelId, IRecognitionStatusCallback callback) { synchronized (mLock) { MetricsLogger.count(mContext, "sth_stop_recognition", 1); if (callback == null || modelId == null) { @@ -393,7 +404,9 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { modelId); return STATUS_ERROR; } - + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } ModelData modelData = mModelDataMap.get(modelId); if (modelData == null || !modelData.isGenericModel()) { Slog.w(TAG, "Attempting stopRecognition on invalid model with id:" + modelId); @@ -418,7 +431,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { * * @return One of {@link #STATUS_ERROR} or {@link #STATUS_OK}. */ - int stopKeyphraseRecognition(int keyphraseId, IRecognitionStatusCallback callback) { + public int stopKeyphraseRecognition(int keyphraseId, IRecognitionStatusCallback callback) { synchronized (mLock) { MetricsLogger.count(mContext, "sth_stop_recognition", 1); if (callback == null) { @@ -426,7 +439,9 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { keyphraseId); return STATUS_ERROR; } - + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } ModelData modelData = getKeyphraseModelDataLocked(keyphraseId); if (modelData == null || !modelData.isKeyphraseModel()) { Slog.w(TAG, "No model exists for given keyphrase Id " + keyphraseId); @@ -538,6 +553,11 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { } public ModuleProperties getModuleProperties() { + synchronized (mLock) { + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } + } for (ModuleProperties moduleProperties : mModulePropertiesProvider.get()) { if (moduleProperties.getId() == mModuleId) { return moduleProperties; @@ -547,7 +567,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { return null; } - int unloadKeyphraseSoundModel(int keyphraseId) { + public int unloadKeyphraseSoundModel(int keyphraseId) { synchronized (mLock) { MetricsLogger.count(mContext, "sth_unload_keyphrase_sound_model", 1); ModelData modelData = getKeyphraseModelDataLocked(keyphraseId); @@ -555,7 +575,9 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { || !modelData.isKeyphraseModel()) { return STATUS_ERROR; } - + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } // Stop recognition if it's the current one. modelData.setRequested(false); int status = updateRecognitionLocked(modelData, false); @@ -574,12 +596,15 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { } } - int unloadGenericSoundModel(UUID modelId) { + public int unloadGenericSoundModel(UUID modelId) { synchronized (mLock) { MetricsLogger.count(mContext, "sth_unload_generic_sound_model", 1); if (modelId == null || mModule == null) { return STATUS_ERROR; } + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } ModelData modelData = mModelDataMap.get(modelId); if (modelData == null || !modelData.isGenericModel()) { Slog.w(TAG, "Unload error: Attempting unload invalid generic model with id:" + @@ -615,19 +640,25 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { } } - boolean isRecognitionRequested(UUID modelId) { + public boolean isRecognitionRequested(UUID modelId) { synchronized (mLock) { + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } ModelData modelData = mModelDataMap.get(modelId); return modelData != null && modelData.isRequested(); } } - int getGenericModelState(UUID modelId) { + public int getGenericModelState(UUID modelId) { synchronized (mLock) { MetricsLogger.count(mContext, "sth_get_generic_model_state", 1); if (modelId == null || mModule == null) { return STATUS_ERROR; } + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } ModelData modelData = mModelDataMap.get(modelId); if (modelData == null || !modelData.isGenericModel()) { Slog.w(TAG, "GetGenericModelState error: Invalid generic model id:" + @@ -647,19 +678,20 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { } } - int getKeyphraseModelState(UUID modelId) { - Slog.w(TAG, "GetKeyphraseModelState error: Not implemented"); - return STATUS_ERROR; - } - - int setParameter(UUID modelId, @ModelParams int modelParam, int value) { + public int setParameter(UUID modelId, @ModelParams int modelParam, int value) { synchronized (mLock) { + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } return setParameterLocked(mModelDataMap.get(modelId), modelParam, value); } } - int setKeyphraseParameter(int keyphraseId, @ModelParams int modelParam, int value) { + public int setKeyphraseParameter(int keyphraseId, @ModelParams int modelParam, int value) { synchronized (mLock) { + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } return setParameterLocked(getKeyphraseModelDataLocked(keyphraseId), modelParam, value); } } @@ -678,14 +710,20 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { return mModule.setParameter(modelData.getHandle(), modelParam, value); } - int getParameter(@NonNull UUID modelId, @ModelParams int modelParam) { + public int getParameter(@NonNull UUID modelId, @ModelParams int modelParam) { synchronized (mLock) { + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } return getParameterLocked(mModelDataMap.get(modelId), modelParam); } } - int getKeyphraseParameter(int keyphraseId, @ModelParams int modelParam) { + public int getKeyphraseParameter(int keyphraseId, @ModelParams int modelParam) { synchronized (mLock) { + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } return getParameterLocked(getKeyphraseModelDataLocked(keyphraseId), modelParam); } } @@ -707,15 +745,21 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { } @Nullable - ModelParamRange queryParameter(@NonNull UUID modelId, @ModelParams int modelParam) { + public ModelParamRange queryParameter(@NonNull UUID modelId, @ModelParams int modelParam) { synchronized (mLock) { + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } return queryParameterLocked(mModelDataMap.get(modelId), modelParam); } } @Nullable - ModelParamRange queryKeyphraseParameter(int keyphraseId, @ModelParams int modelParam) { + public ModelParamRange queryKeyphraseParameter(int keyphraseId, @ModelParams int modelParam) { synchronized (mLock) { + if (mIsDetached) { + throw new IllegalStateException("SoundTriggerHelper has been detached"); + } return queryParameterLocked(getKeyphraseModelDataLocked(keyphraseId), modelParam); } } @@ -1115,12 +1159,14 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { */ public void detach() { synchronized (mLock) { + if (mIsDetached) return; for (ModelData model : mModelDataMap.values()) { forceStopAndUnloadModelLocked(model, null); } mModelDataMap.clear(); internalClearGlobalStateLocked(); if (mModule != null) { + mIsDetached = true; mModule.detach(); mModule = null; } @@ -1289,7 +1335,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { * @param modelData Model data to be used for recognition * @return True if device state allows recognition to run, false if not. */ - boolean isRecognitionAllowedByPowerState(ModelData modelData) { + private boolean isRecognitionAllowedByPowerState(ModelData modelData) { return mSoundTriggerPowerSaveMode == PowerManager.SOUND_TRIGGER_MODE_ALL_ENABLED || (mSoundTriggerPowerSaveMode == PowerManager.SOUND_TRIGGER_MODE_CRITICAL_ONLY && modelData.shouldRunInBatterySaverMode()); From 7040c1291b8b6800b289d6e301b550ae8f90f9b4 Mon Sep 17 00:00:00 2001 From: Felix Oghina Date: Tue, 4 Apr 2023 15:43:00 +0000 Subject: [PATCH 2/2] [hotword] detach session when detector is destroyed Bug: 272147641 Bug: 274806716 Test: atest AlwaysOnHotwordDetectorTest Change-Id: I2297806b6d6161aa1bd88eaf780ae2266f4f8f7a --- .../voice/AlwaysOnHotwordDetector.java | 19 ++++++++++++------- .../IVoiceInteractionSoundTriggerSession.aidl | 5 +++++ .../android/server/SoundTriggerInternal.java | 6 ++++++ .../soundtrigger/SoundTriggerService.java | 5 +++++ .../SoundTriggerSessionBinderProxy.java | 5 +++++ ...undTriggerSessionPermissionsDecorator.java | 9 +++++++++ .../VoiceInteractionManagerService.java | 5 +++++ 7 files changed, 47 insertions(+), 7 deletions(-) diff --git a/core/java/android/service/voice/AlwaysOnHotwordDetector.java b/core/java/android/service/voice/AlwaysOnHotwordDetector.java index 24c96eae03cb5..91c350aa9abaa 100644 --- a/core/java/android/service/voice/AlwaysOnHotwordDetector.java +++ b/core/java/android/service/voice/AlwaysOnHotwordDetector.java @@ -1334,13 +1334,7 @@ public class AlwaysOnHotwordDetector extends AbstractDetector { @Override public void destroy() { synchronized (mLock) { - if (mAvailability == STATE_KEYPHRASE_ENROLLED) { - try { - stopRecognition(); - } catch (Exception e) { - Log.i(TAG, "failed to stopRecognition in destroy", e); - } - } + detachSessionLocked(); mAvailability = STATE_INVALID; mIsAvailabilityOverriddenByTestApi = false; @@ -1349,6 +1343,17 @@ public class AlwaysOnHotwordDetector extends AbstractDetector { super.destroy(); } + private void detachSessionLocked() { + try { + if (DBG) Slog.d(TAG, "detachSessionLocked() " + mSoundTriggerSession); + if (mSoundTriggerSession != null) { + mSoundTriggerSession.detach(); + } + } catch (RemoteException e) { + e.rethrowFromSystemServer(); + } + } + /** * @hide */ diff --git a/core/java/com/android/internal/app/IVoiceInteractionSoundTriggerSession.aidl b/core/java/com/android/internal/app/IVoiceInteractionSoundTriggerSession.aidl index 1ccc71a9e79cf..23de50c567a10 100644 --- a/core/java/com/android/internal/app/IVoiceInteractionSoundTriggerSession.aidl +++ b/core/java/com/android/internal/app/IVoiceInteractionSoundTriggerSession.aidl @@ -94,4 +94,9 @@ interface IVoiceInteractionSoundTriggerSession { */ @nullable SoundTrigger.ModelParamRange queryParameter(int keyphraseId, in ModelParams modelParam); + /** + * Invalidates the sound trigger session and clears any associated resources. Subsequent calls + * to this object will throw IllegalStateException. + */ + void detach(); } diff --git a/services/core/java/com/android/server/SoundTriggerInternal.java b/services/core/java/com/android/server/SoundTriggerInternal.java index e6c1750c4a1de..65294652b92de 100644 --- a/services/core/java/com/android/server/SoundTriggerInternal.java +++ b/services/core/java/com/android/server/SoundTriggerInternal.java @@ -141,6 +141,12 @@ public interface SoundTriggerInternal { ModelParamRange queryParameter(int keyphraseId, @ModelParams int modelParam); + /** + * Invalidates the sound trigger session and clears any associated resources. Subsequent + * calls to this object will throw IllegalStateException. + */ + void detach(); + /** * Unloads (and stops if running) the given keyphraseId */ diff --git a/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerService.java b/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerService.java index 790be8dacd984..46e634fa72732 100644 --- a/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerService.java +++ b/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerService.java @@ -1662,6 +1662,11 @@ public class SoundTriggerService extends SystemService { return mSoundTriggerHelper.queryKeyphraseParameter(keyphraseId, modelParam); } + @Override + public void detach() { + mSoundTriggerHelper.detach(); + } + @Override public int unloadKeyphraseModel(int keyphraseId) { return mSoundTriggerHelper.unloadKeyphraseSoundModel(keyphraseId); diff --git a/services/voiceinteraction/java/com/android/server/voiceinteraction/SoundTriggerSessionBinderProxy.java b/services/voiceinteraction/java/com/android/server/voiceinteraction/SoundTriggerSessionBinderProxy.java index dd9fee3887cb3..0ef2f06b66845 100644 --- a/services/voiceinteraction/java/com/android/server/voiceinteraction/SoundTriggerSessionBinderProxy.java +++ b/services/voiceinteraction/java/com/android/server/voiceinteraction/SoundTriggerSessionBinderProxy.java @@ -69,4 +69,9 @@ final class SoundTriggerSessionBinderProxy extends IVoiceInteractionSoundTrigger public SoundTrigger.ModelParamRange queryParameter(int i, int i1) throws RemoteException { return mDelegate.queryParameter(i, i1); } + + @Override + public void detach() throws RemoteException { + mDelegate.detach(); + } } diff --git a/services/voiceinteraction/java/com/android/server/voiceinteraction/SoundTriggerSessionPermissionsDecorator.java b/services/voiceinteraction/java/com/android/server/voiceinteraction/SoundTriggerSessionPermissionsDecorator.java index c0c3e6f530dbd..0f8a945ec4612 100644 --- a/services/voiceinteraction/java/com/android/server/voiceinteraction/SoundTriggerSessionPermissionsDecorator.java +++ b/services/voiceinteraction/java/com/android/server/voiceinteraction/SoundTriggerSessionPermissionsDecorator.java @@ -113,6 +113,15 @@ final class SoundTriggerSessionPermissionsDecorator implements "This object isn't intended to be used as a Binder."); } + @Override + public void detach() { + try { + mDelegate.detach(); + } catch (RemoteException e) { + e.rethrowFromSystemServer(); + } + } + // TODO: Share this code with SoundTriggerMiddlewarePermission. private boolean isHoldingPermissions() { try { diff --git a/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerService.java b/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerService.java index 1d7b966bab516..bb50c792c4f8e 100644 --- a/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerService.java +++ b/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerService.java @@ -1856,6 +1856,11 @@ public class VoiceInteractionManagerService extends SystemService { "This object isn't intended to be used as a Binder."); } + @Override + public void detach() { + mSession.detach(); + } + private int unloadKeyphraseModel(int keyphraseId) { final long caller = Binder.clearCallingIdentity(); try {