From 458930753d712a04c746e50b94b4b76d11f1dd32 Mon Sep 17 00:00:00 2001 From: Ytai Ben-Tsvi Date: Tue, 21 Apr 2020 10:40:21 -0700 Subject: [PATCH] Deliver internal server errors to the client Due to a binder limitation (b/150808347), any unexpected exception thrown by a binder service while processing an RPC call, will be silently discarded and the client will get back a default-initialized result parcelable (i.e. in most cases, won't know that anything wrong happened). We work around this issue by throwing a ServiceSpecificException, which does get delivered, having a special code to designate an internal error. Errors resulting from a HAL malfunctions will result in a HAL reboot, which leads to recovery. Bug: 154089179 Test: Manual verification of basic error recovery scenarios by injecting HAL error codes and crashes. Change-Id: Ib5dbe08a362e545501c04204bebad5ab95f5d632 --- .../hardware/soundtrigger/SoundTrigger.java | 2 + .../media/soundtrigger_middleware/Status.aidl | 5 +++ .../InternalServerError.java | 40 ------------------- .../SoundTriggerMiddlewareValidation.java | 17 ++++---- 4 files changed, 15 insertions(+), 49 deletions(-) delete mode 100644 services/core/java/com/android/server/soundtrigger_middleware/InternalServerError.java diff --git a/core/java/android/hardware/soundtrigger/SoundTrigger.java b/core/java/android/hardware/soundtrigger/SoundTrigger.java index 98c4f618be188..d12142c0be791 100644 --- a/core/java/android/hardware/soundtrigger/SoundTrigger.java +++ b/core/java/android/hardware/soundtrigger/SoundTrigger.java @@ -1880,6 +1880,8 @@ public class SoundTrigger { return STATUS_PERMISSION_DENIED; case Status.DEAD_OBJECT: return STATUS_DEAD_OBJECT; + case Status.INTERNAL_ERROR: + return STATUS_ERROR; } return STATUS_ERROR; } diff --git a/media/java/android/media/soundtrigger_middleware/Status.aidl b/media/java/android/media/soundtrigger_middleware/Status.aidl index 85ccacf21c8d7..c7623f5bf4910 100644 --- a/media/java/android/media/soundtrigger_middleware/Status.aidl +++ b/media/java/android/media/soundtrigger_middleware/Status.aidl @@ -30,4 +30,9 @@ enum Status { TEMPORARY_PERMISSION_DENIED = 3, /** The object on which this method is called is dead and all of its state is lost. */ DEAD_OBJECT = 4, + /** + * Unexpected internal error has occurred. Usually this will result in the service rebooting + * shortly after. The client should treat the state of the server as undefined. + */ + INTERNAL_ERROR = 5, } diff --git a/services/core/java/com/android/server/soundtrigger_middleware/InternalServerError.java b/services/core/java/com/android/server/soundtrigger_middleware/InternalServerError.java deleted file mode 100644 index e1fb2266b7c6e..0000000000000 --- a/services/core/java/com/android/server/soundtrigger_middleware/InternalServerError.java +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright (C) 2019 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.server.soundtrigger_middleware; - -import android.annotation.NonNull; - -/** - * An internal server error. - *

- * This exception wraps any exception thrown from a service implementation, which is a result of a - * bug in the server implementation (or any of its dependencies). - *

- * Specifically, this type is excluded from the set of whitelisted exceptions that binder would - * tunnel to the client process, since these exceptions are ambiguous regarding whether the client - * had done something wrong or the server is buggy. For example, a client getting an - * IllegalArgumentException cannot easily determine whether they had provided illegal arguments to - * the method they were calling, or whether the method implementation provided illegal arguments to - * some method it was calling due to a bug. - * - * @hide - */ -public class InternalServerError extends RuntimeException { - public InternalServerError(@NonNull Throwable cause) { - super(cause); - } -} diff --git a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java index bae244179346f..ec394a224cd78 100644 --- a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java +++ b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java @@ -96,12 +96,12 @@ import java.util.Set; * {@link NullPointerException} or {@link * IllegalStateException}, respectively. All those exceptions are treated specially by Binder and * will get sent back to the client.
- * Once this is done, any subsequent fault is considered a server fault. Only {@link - * RecoverableException}s thrown by the implementation are special-cased: they would get sent back - * to the caller as a {@link ServiceSpecificException}, which is the behavior of Binder. Any other - * exception gets wrapped with a {@link InternalServerError}, which is specifically chosen as a type - * that does NOT get forwarded by binder. Those exceptions would be handled by a high-level - * exception handler on the server side, typically resulting in rebooting the server. + * Once this is done, any subsequent fault is considered either a recoverable (expected) or + * unexpected server fault. Those will be delivered to the client as a + * {@link ServiceSpecificException}. {@link RecoverableException}s thrown by the implementation are + * considered recoverable and will include a specific error code to indicate the problem. Any other + * exceptions will use the INTERNAL_ERROR code. They may also cause the module to become invalid + * asynchronously, and the client would be notified via the moduleDied() callback. * * {@hide} */ @@ -159,7 +159,7 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware } Log.wtf(TAG, "Unexpected exception", e); - throw new InternalServerError(e); + throw new ServiceSpecificException(Status.INTERNAL_ERROR, e.getMessage()); } @Override @@ -273,8 +273,7 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware throw new ServiceSpecificException(Status.TEMPORARY_PERMISSION_DENIED, String.format("Caller must have the %s permission.", permission)); default: - throw new InternalServerError( - new RuntimeException("Unexpected perimission check result.")); + throw new RuntimeException("Unexpected perimission check result."); } }