From 776b452e48a203091b3d3ed50d338fbb6d67af37 Mon Sep 17 00:00:00 2001 From: Ming-Chuan Date: Mon, 13 Sep 2021 11:06:33 +0800 Subject: [PATCH] Fix ReadableParcelAndroid::ReadString interface (#27260) * Fix ReadableParcelAndroid::ReadString interface Also uses implementation from android/binder_parcel_utils.h to read ByteArray and string from Parcel Test example app on device, works correctly --- .../ext/transport/binder/wire_format/binder.h | 3 +- .../binder/wire_format/binder_android.cc | 75 ++++++++++++------- .../binder/wire_format/binder_android.h | 3 +- .../binder/wire_format/wire_reader_impl.cc | 12 ++- .../transport/binder/end2end/fake_binder.cc | 6 +- .../transport/binder/end2end/fake_binder.h | 2 +- .../binder/end2end/fake_binder_test.cc | 14 ++-- test/core/transport/binder/mock_objects.h | 2 +- 8 files changed, 65 insertions(+), 52 deletions(-) diff --git a/src/core/ext/transport/binder/wire_format/binder.h b/src/core/ext/transport/binder/wire_format/binder.h index 4a05fddd733..ead21c7fd67 100644 --- a/src/core/ext/transport/binder/wire_format/binder.h +++ b/src/core/ext/transport/binder/wire_format/binder.h @@ -74,8 +74,7 @@ class ReadableParcel { virtual absl::Status ReadBinder(std::unique_ptr* data) const = 0; // TODO(waynetu): Provide better interfaces. virtual absl::Status ReadByteArray(std::string* data) const = 0; - // FIXME(waynetu): This is just a temporary interface. - virtual absl::Status ReadString(char data[111]) const = 0; + virtual absl::Status ReadString(std::string* str) const = 0; }; class TransactionReceiver : public HasRawBinder { diff --git a/src/core/ext/transport/binder/wire_format/binder_android.cc b/src/core/ext/transport/binder/wire_format/binder_android.cc index 1df139ba5fa..c1f300a7f81 100644 --- a/src/core/ext/transport/binder/wire_format/binder_android.cc +++ b/src/core/ext/transport/binder/wire_format/binder_android.cc @@ -86,6 +86,43 @@ binder_status_t f_onTransact(AIBinder* binder, transaction_code_t code, return STATUS_UNKNOWN_ERROR; } } + +// StdStringAllocator, ReadString, StdVectorAllocator, and ReadVector's +// implementations are copied from android/binder_parcel_utils.h +// We cannot include the header because it does not compile in C++11 + +bool StdStringAllocator(void* stringData, int32_t length, char** buffer) { + if (length <= 0) return false; + + std::string* str = static_cast(stringData); + str->resize(static_cast(length) - 1); + *buffer = &(*str)[0]; + return true; +} + +binder_status_t AParcelReadString(const AParcel* parcel, std::string* str) { + void* stringData = static_cast(str); + return AParcel_readString(parcel, stringData, StdStringAllocator); +} + +template +bool StdVectorAllocator(void* vectorData, int32_t length, T** outBuffer) { + if (length < 0) return false; + + std::vector* vec = static_cast*>(vectorData); + if (static_cast(length) > vec->max_size()) return false; + + vec->resize(static_cast(length)); + *outBuffer = vec->data(); + return true; +} + +binder_status_t AParcelReadVector(const AParcel* parcel, + std::vector* vec) { + void* vectorData = static_cast(vec); + return AParcel_readByteArray(parcel, vectorData, StdVectorAllocator); +} + } // namespace ndk::SpAIBinder FromJavaBinder(JNIEnv* jni_env, jobject binder) { @@ -276,36 +313,20 @@ absl::Status ReadableParcelAndroid::ReadBinder( return absl::OkStatus(); } -namespace { - -bool byte_array_allocator(void* arrayData, int32_t length, int8_t** outBuffer) { - std::string tmp; - tmp.resize(length); - *reinterpret_cast(arrayData) = tmp; - *outBuffer = reinterpret_cast( - &(*reinterpret_cast(arrayData))[0]); - return true; -} - -bool string_allocator(void* stringData, int32_t length, char** outBuffer) { - if (length > 0) { - // TODO(mingcl): Don't fix the length of the string - GPR_ASSERT(length < 100); // call should preallocate 100 bytes - *outBuffer = reinterpret_cast(stringData); - } - return true; -} - -} // namespace - absl::Status ReadableParcelAndroid::ReadByteArray(std::string* data) const { - return AParcel_readByteArray(parcel_, data, byte_array_allocator) == STATUS_OK - ? absl::OkStatus() - : absl::InternalError("AParcel_readByteArray failed"); + std::vector vec; + if (AParcelReadVector(parcel_, &vec) == STATUS_OK) { + data->resize(vec.size()); + if (!vec.empty()) { + memcpy(&((*data)[0]), vec.data(), vec.size()); + } + return absl::OkStatus(); + } + return absl::InternalError("AParcel_readByteArray failed"); } -absl::Status ReadableParcelAndroid::ReadString(char data[111]) const { - return AParcel_readString(parcel_, data, string_allocator) == STATUS_OK +absl::Status ReadableParcelAndroid::ReadString(std::string* str) const { + return AParcelReadString(parcel_, str) == STATUS_OK ? absl::OkStatus() : absl::InternalError("AParcel_readString failed"); } diff --git a/src/core/ext/transport/binder/wire_format/binder_android.h b/src/core/ext/transport/binder/wire_format/binder_android.h index 46fcd53f40f..68ff764803d 100644 --- a/src/core/ext/transport/binder/wire_format/binder_android.h +++ b/src/core/ext/transport/binder/wire_format/binder_android.h @@ -72,8 +72,7 @@ class ReadableParcelAndroid final : public ReadableParcel { absl::Status ReadInt64(int64_t* data) const override; absl::Status ReadBinder(std::unique_ptr* data) const override; absl::Status ReadByteArray(std::string* data) const override; - // FIXME(waynetu): Fix the interface. - absl::Status ReadString(char data[111]) const override; + absl::Status ReadString(std::string* str) const override; private: AParcel* parcel_ = nullptr; diff --git a/src/core/ext/transport/binder/wire_format/wire_reader_impl.cc b/src/core/ext/transport/binder/wire_format/wire_reader_impl.cc index 0c0d52215d6..50a03c4c85a 100644 --- a/src/core/ext/transport/binder/wire_format/wire_reader_impl.cc +++ b/src/core/ext/transport/binder/wire_format/wire_reader_impl.cc @@ -316,10 +316,9 @@ absl::Status WireReaderImpl::ProcessStreamingTransactionImpl( expectation++; gpr_log(GPR_INFO, "sequence number = %d", seq_num); if (flags & kFlagPrefix) { - char method_ref[111]; - memset(method_ref, 0, sizeof(method_ref)); + std::string method_ref; if (!is_client_) { - RETURN_IF_ERROR(parcel->ReadString(method_ref)); + RETURN_IF_ERROR(parcel->ReadString(&method_ref)); } absl::StatusOr initial_metadata_or_error = parse_metadata(parcel); if (!initial_metadata_or_error.ok()) { @@ -353,10 +352,9 @@ absl::Status WireReaderImpl::ProcessStreamingTransactionImpl( if (flags & kFlagSuffix) { if (flags & kFlagStatusDescription) { // FLAG_STATUS_DESCRIPTION set - char desc[111]; - memset(desc, 0, sizeof(desc)); - RETURN_IF_ERROR(parcel->ReadString(desc)); - gpr_log(GPR_INFO, "description = %s", desc); + std::string desc; + RETURN_IF_ERROR(parcel->ReadString(&desc)); + gpr_log(GPR_INFO, "description = %s", desc.c_str()); } Metadata trailing_metadata; if (is_client_) { diff --git a/test/core/transport/binder/end2end/fake_binder.cc b/test/core/transport/binder/end2end/fake_binder.cc index b455f945f5c..37ba57b91ac 100644 --- a/test/core/transport/binder/end2end/fake_binder.cc +++ b/test/core/transport/binder/end2end/fake_binder.cc @@ -106,14 +106,12 @@ absl::Status FakeReadableParcel::ReadBinder( return absl::OkStatus(); } -absl::Status FakeReadableParcel::ReadString(char data[111]) const { +absl::Status FakeReadableParcel::ReadString(std::string* str) const { if (data_position_ >= data_.size() || !absl::holds_alternative(data_[data_position_])) { return absl::InternalError("ReadString failed"); } - const std::string& s = absl::get(data_[data_position_++]); - if (s.size() >= 100) return absl::InternalError("ReadString failed"); - std::memcpy(data, s.data(), s.size()); + *str = absl::get(data_[data_position_++]); return absl::OkStatus(); } diff --git a/test/core/transport/binder/end2end/fake_binder.h b/test/core/transport/binder/end2end/fake_binder.h index c18149b55f0..f1c48b9b539 100644 --- a/test/core/transport/binder/end2end/fake_binder.h +++ b/test/core/transport/binder/end2end/fake_binder.h @@ -127,7 +127,7 @@ class FakeReadableParcel final : public ReadableParcel { absl::Status ReadInt64(int64_t* data) const override; absl::Status ReadBinder(std::unique_ptr* data) const override; absl::Status ReadByteArray(std::string* data) const override; - absl::Status ReadString(char data[111]) const override; + absl::Status ReadString(std::string* str) const override; private: const FakeData data_; diff --git a/test/core/transport/binder/end2end/fake_binder_test.cc b/test/core/transport/binder/end2end/fake_binder_test.cc index b02e8bca7c6..2fdea9b2963 100644 --- a/test/core/transport/binder/end2end/fake_binder_test.cc +++ b/test/core/transport/binder/end2end/fake_binder_test.cc @@ -98,10 +98,9 @@ TEST_P(FakeBinderTest, SendString) { std::tie(sender, tx_receiver) = NewBinderPair( [&](transaction_code_t tx_code, const ReadableParcel* parcel) { EXPECT_EQ(tx_code, kTxCode); - char value[111]; - memset(value, 0, sizeof(value)); - EXPECT_TRUE(parcel->ReadString(value).ok()); - EXPECT_STREQ(value, kValue); + std::string value; + EXPECT_TRUE(parcel->ReadString(&value).ok()); + EXPECT_STREQ(value.c_str(), kValue); called++; return absl::OkStatus(); }); @@ -160,10 +159,9 @@ TEST_P(FakeBinderTest, SendMultipleItems) { std::string byte_array_result; EXPECT_TRUE(parcel->ReadByteArray(&byte_array_result).ok()); EXPECT_EQ(byte_array_result, kByteArray); - char string_result[111]; - memset(string_result, 0, sizeof(string_result)); - EXPECT_TRUE(parcel->ReadString(string_result).ok()); - EXPECT_STREQ(string_result, kString); + std::string string_result; + EXPECT_TRUE(parcel->ReadString(&string_result).ok()); + EXPECT_STREQ(string_result.c_str(), kString); called++; return absl::OkStatus(); }); diff --git a/test/core/transport/binder/mock_objects.h b/test/core/transport/binder/mock_objects.h index 39f70c3096e..c5562eb866a 100644 --- a/test/core/transport/binder/mock_objects.h +++ b/test/core/transport/binder/mock_objects.h @@ -47,7 +47,7 @@ class MockReadableParcel : public ReadableParcel { MOCK_METHOD(absl::Status, ReadBinder, (std::unique_ptr*), (const, override)); MOCK_METHOD(absl::Status, ReadByteArray, (std::string*), (const, override)); - MOCK_METHOD(absl::Status, ReadString, (char[111]), (const, override)); + MOCK_METHOD(absl::Status, ReadString, (std::string*), (const, override)); MockReadableParcel(); };