Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 45893075 authored by Ytai Ben-Tsvi's avatar Ytai Ben-Tsvi
Browse files

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
parent b344e0ac

core/java/android/hardware/soundtrigger/SoundTrigger.java

+2 −0
Original line number Diff line number Diff line
@@ -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;

        }

media/java/android/media/soundtrigger_middleware/Status.aidl

+5 −0
Original line number Diff line number Diff line
@@ -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,

}

services/core/java/com/android/server/soundtrigger_middleware/InternalServerError.java

deleted100644 → 0
+0 −40
Original line number Diff line number Diff line 
/*

 * 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.

 * <p>

 * 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).

 * <p>

 * 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);

    }

}

services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java

+8 −9
Original line number Diff line number Diff line
@@ -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.<br>

 * 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 <b>does NOT</b> 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.");

        }

    }