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

Commit cff7f175 authored by Christopher Wiley's avatar Christopher Wiley
Browse files

libbinder: Handle transaction failures correctly 

Java code expects status_t != OK to be caught at the JNI level in
android_util_Binder.cpp (see signalExceptionForError).  We were
incorrectly mapping this kind of failure to a special exception type
and writing that exception type to parcels.

Instead, refuse to write EX_TRANSACTION_FAILED to a parcel and return
the status value instead.

While here, remove non-trivial constructors to push authors toward the
more explicit factory methods.  Remove getException() and push authors
toward using the simpler getter methods. Fix minor camelCase issues.

Bug: 25615695
Test: system/tools/aidl integration tests still pass

Change-Id: I7cad3ac8ae8300b5ac0b466606f4934d01e503c5
parent b2e3095a

include/binder/Status.h

+25 −16
Original line number Diff line number Diff line
@@ -60,31 +60,31 @@ public: 
        EX_ILLEGAL_STATE = -5,

        EX_NETWORK_MAIN_THREAD = -6,

        EX_UNSUPPORTED_OPERATION = -7,

        EX_TRANSACTION_FAILED = -8,



        // This is special and Java specific; see Parcel.java.

        EX_HAS_REPLY_HEADER = -128,

        // This is special, and indicates to C++ binder proxies that the

        // transaction has failed at a low level.

        EX_TRANSACTION_FAILED = -129,

    };



    // A more readable alias for the default constructor.

    static Status ok();

    // Allow authors to explicitly pick whether their integer is a status_t or

    // exception code.

    static Status fromExceptionCode(int32_t exception_code);

    static Status fromExceptionCode(int32_t exceptionCode);

    static Status fromExceptionCode(int32_t exceptionCode,

                                    const String8& message);

    static Status fromStatusT(status_t status);

    // A more readable alias for the default constructor.

    static Status ok();



    Status() = default;

    Status(int32_t exception_code, const String8& message);

    Status(int32_t exception_code, const char* message);



    ~Status() = default;



    // Status objects are copyable and contain just simple data.

    Status(const Status& status) = default;

    Status(Status&& status) = default;

    Status& operator=(const Status& status) = default;



    ~Status() = default;



    // Bear in mind that if the client or service is a Java endpoint, this

    // is not the logic which will provide/interpret the data here.

    status_t readFromParcel(const Parcel& parcel);
@@ -92,16 +92,17 @@ public: 


    // Set one of the pre-defined exception types defined above.

    void setException(int32_t ex, const String8& message);

    // A few of the status_t values map to exception codes, but most of them

    // simply map to "transaction failed."

    // Setting a |status| != OK causes generated code to return |status|

    // from Binder transactions, rather than writing an exception into the

    // reply Parcel.

    void setFromStatusT(status_t status);



    // Get information about an exception.

    // Any argument may be given as nullptr.

    void getException(int32_t* returned_exception,

                      String8* returned_message) const;

    int32_t exceptionCode() const  { return mException; }

    const String8& exceptionMessage() const { return mMessage; }

    status_t transactionError() const {

        return mException == EX_TRANSACTION_FAILED ? mErrorCode : OK;

    }



    bool isOk() const { return mException == EX_NONE; }
@@ -109,9 +110,17 @@ public: 
    String8 toString8() const;



private:

    // We always write |mException| to the parcel.

    // If |mException| !=  EX_NONE, we write message as well.

    Status(int32_t exception_code);

    Status(int32_t exception_code, const String8& message);



    // If |mException| == EX_TRANSACTION_FAILED, generated code will return

    // |mErrorCode| as the result of the transaction rather than write an

    // exception to the reply parcel.

    //

    // Otherwise, we always write |mException| to the parcel.

    // If |mException| !=  EX_NONE, we write |mMessage| as well.

    int32_t mException = EX_NONE;

    int32_t mErrorCode = 0;

    String8 mMessage;

};  // class Status

libs/binder/Status.cpp

+34 −37
Original line number Diff line number Diff line
@@ -19,8 +19,17 @@ 
namespace android {

namespace binder {



Status Status::fromExceptionCode(int32_t exception_code) {

    return Status(exception_code, "");

Status Status::ok() {

    return Status();

}



Status Status::fromExceptionCode(int32_t exceptionCode) {

    return Status(exceptionCode);

}



Status Status::fromExceptionCode(int32_t exceptionCode,

                                 const String8& message) {

    return Status(exceptionCode, message);

}



Status Status::fromStatusT(status_t status) {
@@ -29,16 +38,9 @@ Status Status::fromStatusT(status_t status) { 
    return ret;

}



Status Status::ok() {

    return Status();

}



Status::Status(int32_t exception_code, const String8& message)

    : mException(exception_code),

      mMessage(message) {}



Status::Status(int32_t exception_code, const char* message)

    : mException(exception_code),

Status::Status(int32_t exceptionCode) : mException(exceptionCode) {}

Status::Status(int32_t exceptionCode, const String8& message)

    : mException(exceptionCode),

      mMessage(message) {}



status_t Status::readFromParcel(const Parcel& parcel) {
@@ -69,12 +71,24 @@ status_t Status::readFromParcel(const Parcel& parcel) { 
    }



    // The remote threw an exception.  Get the message back.

    mMessage = String8(parcel.readString16());

    String16 message;

    status = parcel.readString16(&message);

    if (status != OK) {

        setFromStatusT(status);

        return status;

    }

    mMessage = String8(message);



    return status;

}



status_t Status::writeToParcel(Parcel* parcel) const {

    // Something really bad has happened, and we're not going to even

    // try returning rich error data.

    if (mException == EX_TRANSACTION_FAILED) {

        return mErrorCode;

    }



    status_t status = parcel->writeInt32(mException);

    if (status != OK) { return status; }

    if (mException == EX_NONE) {
@@ -86,43 +100,26 @@ status_t Status::writeToParcel(Parcel* parcel) const { 
}



void Status::setFromStatusT(status_t status) {

    switch (status) {

        case NO_ERROR:

            mException = EX_NONE;

    mException = (status == NO_ERROR) ? EX_NONE : EX_TRANSACTION_FAILED;

    mErrorCode = status;

    mMessage.clear();

            break;

        case UNEXPECTED_NULL:

            mException = EX_NULL_POINTER;

            mMessage.setTo("Unexpected null reference in Parcel");

            break;

        default:

            mException = EX_TRANSACTION_FAILED;

            mMessage.setTo("Transaction failed");

            break;

    }

}



void Status::setException(int32_t ex, const String8& message) {

    mException = ex;

    mErrorCode = NO_ERROR;  // an exception, not a transaction failure.

    mMessage.setTo(message);

}



void Status::getException(int32_t* returned_exception,

                          String8* returned_message) const {

    if (returned_exception) {

        *returned_exception = mException;

    }

    if (returned_message) {

        returned_message->setTo(mMessage);

    }

}



String8 Status::toString8() const {

    String8 ret;

    if (mException == EX_NONE) {

        ret.append("No error");

    } else {

        ret.appendFormat("Status(%d): '", mException);

        if (mException == EX_TRANSACTION_FAILED) {

            ret.appendFormat("%d: ", mErrorCode);

        }

        ret.append(String8(mMessage));

        ret.append("'");

    }