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

src/core/ext/transport/binder/wire_format/binder.h

@@ -74,8 +74,7 @@ class ReadableParcel {
   virtual absl::Status ReadBinder(std::unique_ptr<Binder>* 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 {

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<std::string*>(stringData);
+  str->resize(static_cast<size_t>(length) - 1);
+  *buffer = &(*str)[0];
+  return true;
+}
+
+binder_status_t AParcelReadString(const AParcel* parcel, std::string* str) {
+  void* stringData = static_cast<void*>(str);
+  return AParcel_readString(parcel, stringData, StdStringAllocator);
+}
+
+template <typename T>
+bool StdVectorAllocator(void* vectorData, int32_t length, T** outBuffer) {
+  if (length < 0) return false;
+
+  std::vector<T>* vec = static_cast<std::vector<T>*>(vectorData);
+  if (static_cast<size_t>(length) > vec->max_size()) return false;
+
+  vec->resize(static_cast<size_t>(length));
+  *outBuffer = vec->data();
+  return true;
+}
+
+binder_status_t AParcelReadVector(const AParcel* parcel,
+                                  std::vector<uint8_t>* vec) {
+  void* vectorData = static_cast<void*>(vec);
+  return AParcel_readByteArray(parcel, vectorData, StdVectorAllocator<int8_t>);
+}
+
 }  // 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<std::string*>(arrayData) = tmp;
-  *outBuffer = reinterpret_cast<int8_t*>(
-      &(*reinterpret_cast<std::string*>(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<char*>(stringData);
+absl::Status ReadableParcelAndroid::ReadByteArray(std::string* data) const {
+  std::vector<uint8_t> vec;
+  if (AParcelReadVector(parcel_, &vec) == STATUS_OK) {
+    data->resize(vec.size());
+    if (!vec.empty()) {
+      memcpy(&((*data)[0]), vec.data(), vec.size());
     }
-  return true;
+    return absl::OkStatus();
   }
-
-}  // 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");
+  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");
 }

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

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<Metadata> 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_) {

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<std::string>(data_[data_position_])) {
     return absl::InternalError("ReadString failed");
   }
-  const std::string& s = absl::get<std::string>(data_[data_position_++]);
-  if (s.size() >= 100) return absl::InternalError("ReadString failed");
-  std::memcpy(data, s.data(), s.size());
+  *str = absl::get<std::string>(data_[data_position_++]);
   return absl::OkStatus();
 }
 

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<Binder>* 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_;

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();
       });

test/core/transport/binder/mock_objects.h

@@ -47,7 +47,7 @@ class MockReadableParcel : public ReadableParcel {
   MOCK_METHOD(absl::Status, ReadBinder, (std::unique_ptr<Binder>*),
               (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();
 };