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
diff --git a/Status.cpp b/Status.cpp
index 41fff3d..67f0d59 100644
--- a/Status.cpp
+++ b/Status.cpp
@@ -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 @@
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 @@
}
// 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 @@
}
void Status::setFromStatusT(status_t status) {
- switch (status) {
- case NO_ERROR:
- mException = EX_NONE;
- 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;
- }
+ mException = (status == NO_ERROR) ? EX_NONE : EX_TRANSACTION_FAILED;
+ mErrorCode = status;
+ mMessage.clear();
}
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("'");
}
diff --git a/include/hwbinder/Status.h b/include/hwbinder/Status.h
index 04738f8..d19824d 100644
--- a/include/hwbinder/Status.h
+++ b/include/hwbinder/Status.h
@@ -60,31 +60,31 @@
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,
};
- // Allow authors to explicitly pick whether their integer is a status_t or
- // exception code.
- static Status fromExceptionCode(int32_t exception_code);
- static Status fromStatusT(status_t status);
// 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 exceptionCode);
+ static Status fromExceptionCode(int32_t exceptionCode,
+ const String8& message);
+ static Status fromStatusT(status_t status);
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 @@
// 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 @@
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