Merge 25Q1 (ab/12770256) to aosp-main-future

Bug: 385190204
Merged-In: I0fb567cbcca67a2fc6c088f652c8af570b8d7e53
Change-Id: Iaae8cd491ff963cf422f4b19c54be33e1244a9a1

10 files changed, 464 insertions, 82 deletions

diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index 1e83c350b1..9f5d822c55 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -180,7 +180,7 @@ static void acquire_object(const sp<ProcessState>& proc, const flat_binder_objec
         }
     }
 
-    ALOGD("Invalid object type 0x%08x", obj.hdr.type);
+    ALOGE("Invalid object type 0x%08x to acquire", obj.hdr.type);
 }
 
 static void release_object(const sp<ProcessState>& proc, const flat_binder_object& obj,
@@ -210,7 +210,7 @@ static void release_object(const sp<ProcessState>& proc, const flat_binder_objec
         }
     }
 
-    ALOGE("Invalid object type 0x%08x", obj.hdr.type);
+    ALOGE("Invalid object type 0x%08x to release", obj.hdr.type);
 }
 #endif // BINDER_WITH_KERNEL_IPC
 
@@ -1162,31 +1162,6 @@ status_t Parcel::finishWrite(size_t len)
     return NO_ERROR;
 }
 
-status_t Parcel::writeUnpadded(const void* data, size_t len)
-{
-    if (len > INT32_MAX) {
-        // don't accept size_t values which may have come from an
-        // inadvertent conversion from a negative int.
-        return BAD_VALUE;
-    }
-
-    size_t end = mDataPos + len;
-    if (end < mDataPos) {
-        // integer overflow
-        return BAD_VALUE;
-    }
-
-    if (end <= mDataCapacity) {
-restart_write:
-        memcpy(mData+mDataPos, data, len);
-        return finishWrite(len);
-    }
-
-    status_t err = growData(len);
-    if (err == NO_ERROR) goto restart_write;
-    return err;
-}
-
 status_t Parcel::write(const void* data, size_t len)
 {
     if (len > INT32_MAX) {
@@ -1223,6 +1198,10 @@ restart_write:
         //printf("Writing %ld bytes, padded to %ld\n", len, padded);
         uint8_t* const data = mData+mDataPos;
 
+        if (status_t status = validateReadData(mDataPos + padded); status != OK) {
+            return nullptr; // drops status
+        }
+
         // Need to pad at end?
         if (padded != len) {
 #if BYTE_ORDER == BIG_ENDIAN
@@ -1803,6 +1782,10 @@ status_t Parcel::writeObject(const flat_binder_object& val, bool nullMetaData)
     const bool enoughObjects = kernelFields->mObjectsSize < kernelFields->mObjectsCapacity;
     if (enoughData && enoughObjects) {
 restart_write:
+        if (status_t status = validateReadData(mDataPos + sizeof(val)); status != OK) {
+            return status;
+        }
+
         *reinterpret_cast<flat_binder_object*>(mData+mDataPos) = val;
 
         // remember if it's a file descriptor
@@ -1878,7 +1861,10 @@ status_t Parcel::validateReadData(size_t upperBound) const
                 if (mDataPos < kernelFields->mObjects[nextObject] + sizeof(flat_binder_object)) {
                     // Requested info overlaps with an object
                     if (!mServiceFuzzing) {
-                        ALOGE("Attempt to read from protected data in Parcel %p", this);
+                        ALOGE("Attempt to read or write from protected data in Parcel %p. pos: "
+                              "%zu, nextObject: %zu, object offset: %llu, object size: %zu",
+                              this, mDataPos, nextObject, kernelFields->mObjects[nextObject],
+                              sizeof(flat_binder_object));
                     }
                     return PERMISSION_DENIED;
                 }
@@ -2046,6 +2032,10 @@ status_t Parcel::writeAligned(T val) {
 
     if ((mDataPos+sizeof(val)) <= mDataCapacity) {
 restart_write:
+        if (status_t status = validateReadData(mDataPos + sizeof(val)); status != OK) {
+            return status;
+        }
+
         memcpy(mData + mDataPos, &val, sizeof(val));
         return finishWrite(sizeof(val));
     }
@@ -2236,9 +2226,7 @@ const char* Parcel::readCString() const
         const char* eos = reinterpret_cast<const char*>(memchr(str, 0, avail));
         if (eos) {
             const size_t len = eos - str;
-            mDataPos += pad_size(len+1);
-            ALOGV("readCString Setting data pos of %p to %zu", this, mDataPos);
-            return str;
+            return static_cast<const char*>(readInplace(len + 1));
         }
     }
     return nullptr;
@@ -2682,14 +2670,14 @@ const flat_binder_object* Parcel::readObject(bool nullMetaData) const
 }
 #endif // BINDER_WITH_KERNEL_IPC
 
-void Parcel::closeFileDescriptors() {
+void Parcel::closeFileDescriptors(size_t newObjectsSize) {
     if (auto* kernelFields = maybeKernelFields()) {
 #ifdef BINDER_WITH_KERNEL_IPC
         size_t i = kernelFields->mObjectsSize;
         if (i > 0) {
             // ALOGI("Closing file descriptors for %zu objects...", i);
         }
-        while (i > 0) {
+        while (i > newObjectsSize) {
             i--;
             const flat_binder_object* flat =
                     reinterpret_cast<flat_binder_object*>(mData + kernelFields->mObjects[i]);
@@ -2700,6 +2688,7 @@ void Parcel::closeFileDescriptors() {
             }
         }
 #else  // BINDER_WITH_KERNEL_IPC
+        (void)newObjectsSize;
         LOG_ALWAYS_FATAL("Binder kernel driver disabled at build time");
         (void)kernelFields;
 #endif // BINDER_WITH_KERNEL_IPC
@@ -2925,7 +2914,7 @@ void Parcel::freeDataNoInit()
         //ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
         auto* kernelFields = maybeKernelFields();
         // Close FDs before freeing, otherwise they will leak for kernel binder.
-        closeFileDescriptors();
+        closeFileDescriptors(/*newObjectsSize=*/0);
         mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,
                kernelFields ? kernelFields->mObjectsSize : 0);
     } else {
@@ -2953,6 +2942,14 @@ status_t Parcel::growData(size_t len)
         return BAD_VALUE;
     }
 
+    if (mDataPos > mDataSize) {
+        // b/370831157 - this case used to abort. We also don't expect mDataPos < mDataSize, but
+        // this would only waste a bit of memory, so it's okay.
+        ALOGE("growData only expected at the end of a Parcel. pos: %zu, size: %zu, capacity: %zu",
+              mDataPos, len, mDataCapacity);
+        return BAD_VALUE;
+    }
+
     if (len > SIZE_MAX - mDataSize) return NO_MEMORY; // overflow
     if (mDataSize + len > SIZE_MAX / 3) return NO_MEMORY; // overflow
     size_t newSize = ((mDataSize+len)*3)/2;
@@ -3054,13 +3051,38 @@ status_t Parcel::continueWrite(size_t desired)
             objectsSize = 0;
         } else {
             if (kernelFields) {
+#ifdef BINDER_WITH_KERNEL_IPC
+                validateReadData(mDataSize); // hack to sort the objects
                 while (objectsSize > 0) {
-                    if (kernelFields->mObjects[objectsSize - 1] < desired) break;
+                    if (kernelFields->mObjects[objectsSize - 1] + sizeof(flat_binder_object) <=
+                        desired)
+                        break;
                     objectsSize--;
                 }
+#endif // BINDER_WITH_KERNEL_IPC
             } else {
                 while (objectsSize > 0) {
-                    if (rpcFields->mObjectPositions[objectsSize - 1] < desired) break;
+                    // Object size varies by type.
+                    uint32_t pos = rpcFields->mObjectPositions[objectsSize - 1];
+                    size_t size = sizeof(RpcFields::ObjectType);
+                    uint32_t minObjectEnd;
+                    if (__builtin_add_overflow(pos, sizeof(RpcFields::ObjectType), &minObjectEnd) ||
+                        minObjectEnd > mDataSize) {
+                        return BAD_VALUE;
+                    }
+                    const auto type = *reinterpret_cast<const RpcFields::ObjectType*>(mData + pos);
+                    switch (type) {
+                        case RpcFields::TYPE_BINDER_NULL:
+                            break;
+                        case RpcFields::TYPE_BINDER:
+                            size += sizeof(uint64_t); // address
+                            break;
+                        case RpcFields::TYPE_NATIVE_FILE_DESCRIPTOR:
+                            size += sizeof(int32_t); // fd index
+                            break;
+                    }
+
+                    if (pos + size <= desired) break;
                     objectsSize--;
                 }
             }
@@ -3109,15 +3131,24 @@ status_t Parcel::continueWrite(size_t desired)
         if (mData) {
             memcpy(data, mData, mDataSize < desired ? mDataSize : desired);
         }
+#ifdef BINDER_WITH_KERNEL_IPC
         if (objects && kernelFields && kernelFields->mObjects) {
             memcpy(objects, kernelFields->mObjects, objectsSize * sizeof(binder_size_t));
+            // All FDs are owned when `mOwner`, even when `cookie == 0`. When
+            // we switch to `!mOwner`, we need to explicitly mark the FDs as
+            // owned.
+            for (size_t i = 0; i < objectsSize; i++) {
+                flat_binder_object* flat = reinterpret_cast<flat_binder_object*>(data + objects[i]);
+                if (flat->hdr.type == BINDER_TYPE_FD) {
+                    flat->cookie = 1;
+                }
+            }
         }
         // ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
         if (kernelFields) {
-            // TODO(b/239222407): This seems wrong. We should only free FDs when
-            // they are in a truncated section of the parcel.
-            closeFileDescriptors();
+            closeFileDescriptors(objectsSize);
         }
+#endif // BINDER_WITH_KERNEL_IPC
         mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,
                kernelFields ? kernelFields->mObjectsSize : 0);
         mOwner = nullptr;
@@ -3244,11 +3275,19 @@ status_t Parcel::truncateRpcObjects(size_t newObjectsSize) {
     }
     while (rpcFields->mObjectPositions.size() > newObjectsSize) {
         uint32_t pos = rpcFields->mObjectPositions.back();
-        rpcFields->mObjectPositions.pop_back();
+        uint32_t minObjectEnd;
+        if (__builtin_add_overflow(pos, sizeof(RpcFields::ObjectType), &minObjectEnd) ||
+            minObjectEnd > mDataSize) {
+            return BAD_VALUE;
+        }
         const auto type = *reinterpret_cast<const RpcFields::ObjectType*>(mData + pos);
         if (type == RpcFields::TYPE_NATIVE_FILE_DESCRIPTOR) {
-            const auto fdIndex =
-                    *reinterpret_cast<const int32_t*>(mData + pos + sizeof(RpcFields::ObjectType));
+            uint32_t objectEnd;
+            if (__builtin_add_overflow(minObjectEnd, sizeof(int32_t), &objectEnd) ||
+                objectEnd > mDataSize) {
+                return BAD_VALUE;
+            }
+            const auto fdIndex = *reinterpret_cast<const int32_t*>(mData + minObjectEnd);
             if (rpcFields->mFds == nullptr || fdIndex < 0 ||
                 static_cast<size_t>(fdIndex) >= rpcFields->mFds->size()) {
                 ALOGE("RPC Parcel contains invalid file descriptor index. index=%d fd_count=%zu",
@@ -3258,6 +3297,7 @@ status_t Parcel::truncateRpcObjects(size_t newObjectsSize) {
             // In practice, this always removes the last element.
             rpcFields->mFds->erase(rpcFields->mFds->begin() + fdIndex);
         }
+        rpcFields->mObjectPositions.pop_back();
     }
     return OK;
 }
diff --git a/libs/binder/include/binder/Parcel.h b/libs/binder/include/binder/Parcel.h
index 9cd2ae9c25..1154211582 100644
--- a/libs/binder/include/binder/Parcel.h
+++ b/libs/binder/include/binder/Parcel.h
@@ -172,14 +172,14 @@ public:
 
     LIBBINDER_EXPORTED status_t write(const void* data, size_t len);
     LIBBINDER_EXPORTED void* writeInplace(size_t len);
-    LIBBINDER_EXPORTED status_t writeUnpadded(const void* data, size_t len);
     LIBBINDER_EXPORTED status_t writeInt32(int32_t val);
     LIBBINDER_EXPORTED status_t writeUint32(uint32_t val);
     LIBBINDER_EXPORTED status_t writeInt64(int64_t val);
     LIBBINDER_EXPORTED status_t writeUint64(uint64_t val);
     LIBBINDER_EXPORTED status_t writeFloat(float val);
     LIBBINDER_EXPORTED status_t writeDouble(double val);
-    LIBBINDER_EXPORTED status_t writeCString(const char* str);
+    LIBBINDER_EXPORTED status_t writeCString(const char* str)
+            __attribute__((deprecated("use AIDL, writeString* instead")));
     LIBBINDER_EXPORTED status_t writeString8(const String8& str);
     LIBBINDER_EXPORTED status_t writeString8(const char* str, size_t len);
     LIBBINDER_EXPORTED status_t writeString16(const String16& str);
@@ -435,7 +435,8 @@ public:
     LIBBINDER_EXPORTED status_t readUtf8FromUtf16(std::unique_ptr<std::string>* str) const
             __attribute__((deprecated("use std::optional version instead")));
 
-    LIBBINDER_EXPORTED const char* readCString() const;
+    LIBBINDER_EXPORTED const char* readCString() const
+            __attribute__((deprecated("use AIDL, use readString*")));
     LIBBINDER_EXPORTED String8 readString8() const;
     LIBBINDER_EXPORTED status_t readString8(String8* pArg) const;
     LIBBINDER_EXPORTED const char* readString8Inplace(size_t* outLen) const;
@@ -651,8 +652,8 @@ public:
     LIBBINDER_EXPORTED void print(std::ostream& to, uint32_t flags = 0) const;
 
 private:
-    // Explicitly close all file descriptors in the parcel.
-    void closeFileDescriptors();
+    // Close all file descriptors in the parcel at object positions >= newObjectsSize.
+    void closeFileDescriptors(size_t newObjectsSize);
 
     // `objects` and `objectsSize` always 0 for RPC Parcels.
     typedef void (*release_func)(const uint8_t* data, size_t dataSize, const binder_size_t* objects,
diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp
index 391a57010a..891c0a290c 100644
--- a/libs/binder/tests/binderLibTest.cpp
+++ b/libs/binder/tests/binderLibTest.cpp
@@ -48,6 +48,7 @@
 
 #include <linux/sched.h>
 #include <sys/epoll.h>
+#include <sys/mman.h>
 #include <sys/prctl.h>
 #include <sys/socket.h>
 #include <sys/un.h>
@@ -112,6 +113,8 @@ enum BinderLibTestTranscationCode {
     BINDER_LIB_TEST_LINK_DEATH_TRANSACTION,
     BINDER_LIB_TEST_WRITE_FILE_TRANSACTION,
     BINDER_LIB_TEST_WRITE_PARCEL_FILE_DESCRIPTOR_TRANSACTION,
+    BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION,
+    BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION,
     BINDER_LIB_TEST_EXIT_TRANSACTION,
     BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION,
     BINDER_LIB_TEST_GET_PTR_SIZE_TRANSACTION,
@@ -548,6 +551,30 @@ class TestDeathRecipient : public IBinder::DeathRecipient, public BinderLibTestE
         };
 };
 
+ssize_t countFds() {
+    return std::distance(std::filesystem::directory_iterator("/proc/self/fd"),
+                         std::filesystem::directory_iterator{});
+}
+
+struct FdLeakDetector {
+    int startCount;
+
+    FdLeakDetector() {
+        // This log statement is load bearing. We have to log something before
+        // counting FDs to make sure the logging system is initialized, otherwise
+        // the sockets it opens will look like a leak.
+        ALOGW("FdLeakDetector counting FDs.");
+        startCount = countFds();
+    }
+    ~FdLeakDetector() {
+        int endCount = countFds();
+        if (startCount != endCount) {
+            ADD_FAILURE() << "fd count changed (" << startCount << " -> " << endCount
+                          << ") fd leak?";
+        }
+    }
+};
+
 TEST_F(BinderLibTest, CannotUseBinderAfterFork) {
     // EXPECT_DEATH works by forking the process
     EXPECT_DEATH({ ProcessState::self(); }, "libbinder ProcessState can not be used after fork");
@@ -1182,6 +1209,100 @@ TEST_F(BinderLibTest, PassParcelFileDescriptor) {
     EXPECT_EQ(0, read(read_end.get(), readbuf.data(), datasize));
 }
 
+TEST_F(BinderLibTest, RecvOwnedFileDescriptors) {
+    FdLeakDetector fd_leak_detector;
+
+    Parcel data;
+    Parcel reply;
+    EXPECT_EQ(NO_ERROR,
+              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
+                                 &reply));
+    unique_fd a, b;
+    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
+    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
+}
+
+// Used to trigger fdsan error (b/239222407).
+TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndWriteInt) {
+    GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
+
+    FdLeakDetector fd_leak_detector;
+
+    Parcel data;
+    Parcel reply;
+    EXPECT_EQ(NO_ERROR,
+              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
+                                 &reply));
+    reply.setDataPosition(reply.dataSize());
+    reply.writeInt32(0);
+    reply.setDataPosition(0);
+    unique_fd a, b;
+    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
+    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
+}
+
+// Used to trigger fdsan error (b/239222407).
+TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndTruncate) {
+    GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
+
+    FdLeakDetector fd_leak_detector;
+
+    Parcel data;
+    Parcel reply;
+    EXPECT_EQ(NO_ERROR,
+              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
+                                 &reply));
+    reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
+    unique_fd a, b;
+    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
+    EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
+}
+
+TEST_F(BinderLibTest, RecvUnownedFileDescriptors) {
+    FdLeakDetector fd_leak_detector;
+
+    Parcel data;
+    Parcel reply;
+    EXPECT_EQ(NO_ERROR,
+              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
+                                 &reply));
+    unique_fd a, b;
+    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
+    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
+}
+
+// Used to trigger fdsan error (b/239222407).
+TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndWriteInt) {
+    FdLeakDetector fd_leak_detector;
+
+    Parcel data;
+    Parcel reply;
+    EXPECT_EQ(NO_ERROR,
+              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
+                                 &reply));
+    reply.setDataPosition(reply.dataSize());
+    reply.writeInt32(0);
+    reply.setDataPosition(0);
+    unique_fd a, b;
+    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
+    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
+}
+
+// Used to trigger fdsan error (b/239222407).
+TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndTruncate) {
+    FdLeakDetector fd_leak_detector;
+
+    Parcel data;
+    Parcel reply;
+    EXPECT_EQ(NO_ERROR,
+              m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
+                                 &reply));
+    reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
+    unique_fd a, b;
+    EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
+    EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
+}
+
 TEST_F(BinderLibTest, PromoteLocal) {
     sp<IBinder> strong = new BBinder();
     wp<IBinder> weak = strong;
@@ -2261,6 +2382,40 @@ public:
                 if (ret != size) return UNKNOWN_ERROR;
                 return NO_ERROR;
             }
+            case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION: {
+                unique_fd fd1(memfd_create("memfd1", MFD_CLOEXEC));
+                if (!fd1.ok()) {
+                    PLOGE("memfd_create failed");
+                    return UNKNOWN_ERROR;
+                }
+                unique_fd fd2(memfd_create("memfd2", MFD_CLOEXEC));
+                if (!fd2.ok()) {
+                    PLOGE("memfd_create failed");
+                    return UNKNOWN_ERROR;
+                }
+                status_t ret;
+                ret = reply->writeFileDescriptor(fd1.release(), true);
+                if (ret != NO_ERROR) {
+                    return ret;
+                }
+                ret = reply->writeFileDescriptor(fd2.release(), true);
+                if (ret != NO_ERROR) {
+                    return ret;
+                }
+                return NO_ERROR;
+            }
+            case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION: {
+                status_t ret;
+                ret = reply->writeFileDescriptor(STDOUT_FILENO, false);
+                if (ret != NO_ERROR) {
+                    return ret;
+                }
+                ret = reply->writeFileDescriptor(STDERR_FILENO, false);
+                if (ret != NO_ERROR) {
+                    return ret;
+                }
+                return NO_ERROR;
+            }
             case BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION:
                 alarm(10);
                 return NO_ERROR;
diff --git a/libs/binder/tests/binderParcelUnitTest.cpp b/libs/binder/tests/binderParcelUnitTest.cpp
index 32a70e5b11..6259d9d2d2 100644
--- a/libs/binder/tests/binderParcelUnitTest.cpp
+++ b/libs/binder/tests/binderParcelUnitTest.cpp
@@ -33,6 +33,38 @@ using android::String8;
 using android::binder::Status;
 using android::binder::unique_fd;
 
+static void checkCString(const char* str) {
+    for (size_t i = 0; i < 3; i++) {
+        Parcel p;
+
+        for (size_t j = 0; j < i; j++) p.writeInt32(3);
+
+        p.writeCString(str);
+        int32_t pos = p.dataPosition();
+
+        p.setDataPosition(0);
+
+        for (size_t j = 0; j < i; j++) p.readInt32();
+        const char* str2 = p.readCString();
+
+        ASSERT_EQ(std::string(str), str2);
+        ASSERT_EQ(pos, p.dataPosition());
+    }
+}
+
+TEST(Parcel, TestReadCString) {
+    // we should remove the *CString APIs, but testing them until
+    // they are deleted.
+    checkCString("");
+    checkCString("a");
+    checkCString("\n");
+    checkCString("32");
+    checkCString("321");
+    checkCString("3210");
+    checkCString("3210b");
+    checkCString("123434");
+}
+
 TEST(Parcel, NonNullTerminatedString8) {
     String8 kTestString = String8("test-is-good");
 
diff --git a/libs/binder/tests/parcel_fuzzer/binder.cpp b/libs/binder/tests/parcel_fuzzer/binder.cpp
index 20b6b44c62..a41726c313 100644
--- a/libs/binder/tests/parcel_fuzzer/binder.cpp
+++ b/libs/binder/tests/parcel_fuzzer/binder.cpp
@@ -25,6 +25,8 @@
 #include <binder/ParcelableHolder.h>
 #include <binder/PersistableBundle.h>
 #include <binder/Status.h>
+#include <fuzzbinder/random_binder.h>
+#include <fuzzbinder/random_fd.h>
 #include <utils/Flattenable.h>
 
 #include "../../Utils.h"
@@ -115,14 +117,6 @@ std::vector<ParcelRead<::android::Parcel>> BINDER_PARCEL_READ_FUNCTIONS {
         p.setDataPosition(pos);
         FUZZ_LOG() << "setDataPosition done";
     },
-    [] (const ::android::Parcel& p, FuzzedDataProvider& provider) {
-        size_t len = provider.ConsumeIntegralInRange<size_t>(0, 1024);
-        std::vector<uint8_t> bytes = provider.ConsumeBytes<uint8_t>(len);
-        FUZZ_LOG() << "about to setData: " <<(bytes.data() ? HexString(bytes.data(), bytes.size()) : "null");
-        // TODO: allow all read and write operations
-        (*const_cast<::android::Parcel*>(&p)).setData(bytes.data(), bytes.size());
-        FUZZ_LOG() << "setData done";
-    },
     PARCEL_READ_NO_STATUS(size_t, allowFds),
     PARCEL_READ_NO_STATUS(size_t, hasFileDescriptors),
     PARCEL_READ_NO_STATUS(std::vector<android::sp<android::IBinder>>, debugReadAllStrongBinders),
@@ -402,5 +396,113 @@ std::vector<ParcelRead<::android::Parcel>> BINDER_PARCEL_READ_FUNCTIONS {
         FUZZ_LOG() << " toString() result: " << toString;
     },
 };
+
+std::vector<ParcelWrite<::android::Parcel>> BINDER_PARCEL_WRITE_FUNCTIONS {
+    [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call setDataSize";
+        size_t len = provider.ConsumeIntegralInRange<size_t>(0, 1024);
+        p.setDataSize(len);
+    },
+    [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call setDataCapacity";
+        size_t len = provider.ConsumeIntegralInRange<size_t>(0, 1024);
+        p.setDataCapacity(len);
+    },
+    [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call setData";
+        size_t len = provider.ConsumeIntegralInRange<size_t>(0, 1024);
+        std::vector<uint8_t> bytes = provider.ConsumeBytes<uint8_t>(len);
+        p.setData(bytes.data(), bytes.size());
+    },
+    [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* options) {
+        FUZZ_LOG() << "about to call appendFrom";
+
+        std::vector<uint8_t> bytes = provider.ConsumeBytes<uint8_t>(provider.ConsumeIntegralInRange<size_t>(0, 4096));
+        ::android::Parcel p2;
+        fillRandomParcel(&p2, FuzzedDataProvider(bytes.data(), bytes.size()), options);
+
+        int32_t start = provider.ConsumeIntegral<int32_t>();
+        int32_t len = provider.ConsumeIntegral<int32_t>();
+        p.appendFrom(&p2, start, len);
+    },
+    [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call pushAllowFds";
+        bool val = provider.ConsumeBool();
+        p.pushAllowFds(val);
+    },
+    [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call restoreAllowFds";
+        bool val = provider.ConsumeBool();
+        p.restoreAllowFds(val);
+    },
+    // markForBinder - covered by fillRandomParcel, aborts if called multiple times
+    // markForRpc - covered by fillRandomParcel, aborts if called multiple times
+    [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call writeInterfaceToken";
+        std::string interface = provider.ConsumeRandomLengthString();
+        p.writeInterfaceToken(android::String16(interface.c_str()));
+    },
+    [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call setEnforceNoDataAvail";
+        p.setEnforceNoDataAvail(provider.ConsumeBool());
+    },
+    [] (::android::Parcel& p, FuzzedDataProvider& /* provider */, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call setServiceFuzzing";
+        p.setServiceFuzzing();
+    },
+    [] (::android::Parcel& p, FuzzedDataProvider& /* provider */, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call freeData";
+        p.freeData();
+    },
+    [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call write";
+        size_t len = provider.ConsumeIntegralInRange<size_t>(0, 256);
+        std::vector<uint8_t> bytes = provider.ConsumeBytes<uint8_t>(len);
+        p.write(bytes.data(), bytes.size());
+    },
+    // write* - write functions all implemented by calling 'write' itself.
+    [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* options) {
+        FUZZ_LOG() << "about to call writeStrongBinder";
+
+        // TODO: this logic is somewhat duplicated with random parcel
+       android::sp<android::IBinder> binder;
+       if (provider.ConsumeBool() && options->extraBinders.size() > 0) {
+            binder = options->extraBinders.at(
+                    provider.ConsumeIntegralInRange<size_t>(0, options->extraBinders.size() - 1));
+        } else {
+            binder = android::getRandomBinder(&provider);
+            options->extraBinders.push_back(binder);
+        }
+
+        p.writeStrongBinder(binder);
+    },
+    [] (::android::Parcel& p, FuzzedDataProvider& /* provider */, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call writeFileDescriptor (no ownership)";
+        p.writeFileDescriptor(STDERR_FILENO, false /* takeOwnership */);
+    },
+    [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* options) {
+        FUZZ_LOG() << "about to call writeFileDescriptor (take ownership)";
+        std::vector<unique_fd> fds = android::getRandomFds(&provider);
+        if (fds.size() == 0) return;
+
+        p.writeDupFileDescriptor(fds.at(0).get());
+        options->extraFds.insert(options->extraFds.end(),
+             std::make_move_iterator(fds.begin() + 1),
+             std::make_move_iterator(fds.end()));
+    },
+    // TODO: writeBlob
+    // TODO: writeDupImmutableBlobFileDescriptor
+    // TODO: writeObject (or make the API private more likely)
+    [] (::android::Parcel& p, FuzzedDataProvider& /* provider */, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call writeNoException";
+        p.writeNoException();
+    },
+    [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) {
+        FUZZ_LOG() << "about to call replaceCallingWorkSourceUid";
+        uid_t uid = provider.ConsumeIntegral<uid_t>();
+        p.replaceCallingWorkSourceUid(uid);
+    },
+};
+
 // clang-format on
 #pragma clang diagnostic pop
diff --git a/libs/binder/tests/parcel_fuzzer/binder.h b/libs/binder/tests/parcel_fuzzer/binder.h
index 0c51d68a37..b0ac140d3f 100644
--- a/libs/binder/tests/parcel_fuzzer/binder.h
+++ b/libs/binder/tests/parcel_fuzzer/binder.h
@@ -21,3 +21,4 @@
 #include "parcel_fuzzer.h"
 
 extern std::vector<ParcelRead<::android::Parcel>> BINDER_PARCEL_READ_FUNCTIONS;
+extern std::vector<ParcelWrite<::android::Parcel>> BINDER_PARCEL_WRITE_FUNCTIONS;
diff --git a/libs/binder/tests/parcel_fuzzer/binder_ndk.cpp b/libs/binder/tests/parcel_fuzzer/binder_ndk.cpp
index e3a337171f..3f8d71d1f9 100644
--- a/libs/binder/tests/parcel_fuzzer/binder_ndk.cpp
+++ b/libs/binder/tests/parcel_fuzzer/binder_ndk.cpp
@@ -20,8 +20,11 @@
 #include "aidl/parcelables/GenericDataParcelable.h"
 #include "aidl/parcelables/SingleDataParcelable.h"
 
+#include <android/binder_libbinder.h>
 #include <android/binder_parcel_utils.h>
 #include <android/binder_parcelable_utils.h>
+#include <fuzzbinder/random_binder.h>
+#include <fuzzbinder/random_fd.h>
 
 #include "util.h"
 
@@ -211,16 +214,51 @@ std::vector<ParcelRead<NdkParcelAdapter>> BINDER_NDK_PARCEL_READ_FUNCTIONS{
             binder_status_t status = AParcel_marshal(p.aParcel(), buffer, start, len);
             FUZZ_LOG() << "status: " << status;
         },
-        [](const NdkParcelAdapter& /*p*/, FuzzedDataProvider& provider) {
-            FUZZ_LOG() << "about to unmarshal AParcel";
+};
+std::vector<ParcelWrite<NdkParcelAdapter>> BINDER_NDK_PARCEL_WRITE_FUNCTIONS{
+        [] (NdkParcelAdapter& p, FuzzedDataProvider& provider, android::RandomParcelOptions* options) {
+            FUZZ_LOG() << "about to call AParcel_writeStrongBinder";
+
+            // TODO: this logic is somewhat duplicated with random parcel
+            android::sp<android::IBinder> binder;
+            if (provider.ConsumeBool() && options->extraBinders.size() > 0) {
+                binder = options->extraBinders.at(
+                        provider.ConsumeIntegralInRange<size_t>(0, options->extraBinders.size() - 1));
+            } else {
+                binder = android::getRandomBinder(&provider);
+                options->extraBinders.push_back(binder);
+            }
+
+            ndk::SpAIBinder abinder = ndk::SpAIBinder(AIBinder_fromPlatformBinder(binder));
+            AParcel_writeStrongBinder(p.aParcel(), abinder.get());
+        },
+        [] (NdkParcelAdapter& p, FuzzedDataProvider& provider, android::RandomParcelOptions* options) {
+            FUZZ_LOG() << "about to call AParcel_writeParcelFileDescriptor";
+
+            auto fds = android::getRandomFds(&provider);
+            if (fds.size() == 0) return;
+
+            AParcel_writeParcelFileDescriptor(p.aParcel(), fds.at(0).get());
+            options->extraFds.insert(options->extraFds.end(),
+                 std::make_move_iterator(fds.begin() + 1),
+                 std::make_move_iterator(fds.end()));
+        },
+        // all possible data writes can be done as a series of 4-byte reads
+        [] (NdkParcelAdapter& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) {
+            FUZZ_LOG() << "about to call AParcel_writeInt32";
+            int32_t val = provider.ConsumeIntegral<int32_t>();
+            AParcel_writeInt32(p.aParcel(), val);
+        },
+        [] (NdkParcelAdapter& p, FuzzedDataProvider& /* provider */, android::RandomParcelOptions* /*options*/) {
+            FUZZ_LOG() << "about to call AParcel_reset";
+            AParcel_reset(p.aParcel());
+        },
+        [](NdkParcelAdapter& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) {
+            FUZZ_LOG() << "about to call AParcel_unmarshal";
             size_t len = provider.ConsumeIntegralInRange<size_t>(0, provider.remaining_bytes());
-            std::vector<uint8_t> parcelData = provider.ConsumeBytes<uint8_t>(len);
-            const uint8_t* buffer = parcelData.data();
-            const size_t bufferLen = parcelData.size();
-            NdkParcelAdapter adapter;
-            binder_status_t status = AParcel_unmarshal(adapter.aParcel(), buffer, bufferLen);
+            std::vector<uint8_t> data = provider.ConsumeBytes<uint8_t>(len);
+            binder_status_t status = AParcel_unmarshal(p.aParcel(), data.data(), data.size());
             FUZZ_LOG() << "status: " << status;
         },
-
 };
 // clang-format on
diff --git a/libs/binder/tests/parcel_fuzzer/binder_ndk.h b/libs/binder/tests/parcel_fuzzer/binder_ndk.h
index d19f25bc88..0c8b725400 100644
--- a/libs/binder/tests/parcel_fuzzer/binder_ndk.h
+++ b/libs/binder/tests/parcel_fuzzer/binder_ndk.h
@@ -50,3 +50,4 @@ private:
 };
 
 extern std::vector<ParcelRead<NdkParcelAdapter>> BINDER_NDK_PARCEL_READ_FUNCTIONS;
+extern std::vector<ParcelWrite<NdkParcelAdapter>> BINDER_NDK_PARCEL_WRITE_FUNCTIONS;
diff --git a/libs/binder/tests/parcel_fuzzer/main.cpp b/libs/binder/tests/parcel_fuzzer/main.cpp
index a57d07fb24..192f9d5dce 100644
--- a/libs/binder/tests/parcel_fuzzer/main.cpp
+++ b/libs/binder/tests/parcel_fuzzer/main.cpp
@@ -80,6 +80,7 @@ void doTransactFuzz(const char* backend, const sp<B>& binder, FuzzedDataProvider
     (void)binder->transact(code, data, &reply, flag);
 }
 
+// start with a Parcel full of data (e.g. like you get from another process)
 template <typename P>
 void doReadFuzz(const char* backend, const std::vector<ParcelRead<P>>& reads,
                 FuzzedDataProvider&& provider) {
@@ -95,10 +96,10 @@ void doReadFuzz(const char* backend, const std::vector<ParcelRead<P>>& reads,
     RandomParcelOptions options;
 
     P p;
-    fillRandomParcel(&p, std::move(provider), &options);
+    fillRandomParcel(&p, std::move(provider), &options); // consumes provider
 
     // since we are only using a byte to index
-    CHECK(reads.size() <= 255) << reads.size();
+    CHECK_LE(reads.size(), 255u) << reads.size();
 
     FUZZ_LOG() << "backend: " << backend;
     FUZZ_LOG() << "input: " << HexString(p.data(), p.dataSize());
@@ -115,26 +116,29 @@ void doReadFuzz(const char* backend, const std::vector<ParcelRead<P>>& reads,
     }
 }
 
-// Append two random parcels.
 template <typename P>
-void doAppendFuzz(const char* backend, FuzzedDataProvider&& provider) {
-    int32_t start = provider.ConsumeIntegral<int32_t>();
-    int32_t len = provider.ConsumeIntegral<int32_t>();
-
-    std::vector<uint8_t> bytes = provider.ConsumeBytes<uint8_t>(
-            provider.ConsumeIntegralInRange<size_t>(0, provider.remaining_bytes()));
-
-    // same options so that FDs and binders could be shared in both Parcels
+void doReadWriteFuzz(const char* backend, const std::vector<ParcelRead<P>>& reads,
+                     const std::vector<ParcelWrite<P>>& writes, FuzzedDataProvider&& provider) {
     RandomParcelOptions options;
+    P p;
 
-    P p0, p1;
-    fillRandomParcel(&p0, FuzzedDataProvider(bytes.data(), bytes.size()), &options);
-    fillRandomParcel(&p1, std::move(provider), &options);
+    // since we are only using a byte to index
+    CHECK_LE(reads.size() + writes.size(), 255u) << reads.size();
 
     FUZZ_LOG() << "backend: " << backend;
-    FUZZ_LOG() << "start: " << start << " len: " << len;
 
-    p0.appendFrom(&p1, start, len);
+    while (provider.remaining_bytes() > 0) {
+        uint8_t idx = provider.ConsumeIntegralInRange<uint8_t>(0, reads.size() + writes.size() - 1);
+
+        FUZZ_LOG() << "Instruction " << idx << " avail: " << p.dataAvail()
+                   << " pos: " << p.dataPosition() << " cap: " << p.dataCapacity();
+
+        if (idx < reads.size()) {
+            reads.at(idx)(p, provider);
+        } else {
+            writes.at(idx - reads.size())(p, provider, &options);
+        }
+    }
 }
 
 void* NothingClass_onCreate(void* args) {
@@ -156,7 +160,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
 
     FuzzedDataProvider provider = FuzzedDataProvider(data, size);
 
-    const std::function<void(FuzzedDataProvider &&)> fuzzBackend[] = {
+    const std::function<void(FuzzedDataProvider&&)> fuzzBackend[] = {
             [](FuzzedDataProvider&& provider) {
                 doTransactFuzz<
                         ::android::hardware::Parcel>("hwbinder",
@@ -187,10 +191,14 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
                                              std::move(provider));
             },
             [](FuzzedDataProvider&& provider) {
-                doAppendFuzz<::android::Parcel>("binder", std::move(provider));
+                doReadWriteFuzz<::android::Parcel>("binder", BINDER_PARCEL_READ_FUNCTIONS,
+                                                   BINDER_PARCEL_WRITE_FUNCTIONS,
+                                                   std::move(provider));
             },
             [](FuzzedDataProvider&& provider) {
-                doAppendFuzz<NdkParcelAdapter>("binder_ndk", std::move(provider));
+                doReadWriteFuzz<NdkParcelAdapter>("binder_ndk", BINDER_NDK_PARCEL_READ_FUNCTIONS,
+                                                  BINDER_NDK_PARCEL_WRITE_FUNCTIONS,
+                                                  std::move(provider));
             },
     };
 
diff --git a/libs/binder/tests/parcel_fuzzer/parcel_fuzzer.h b/libs/binder/tests/parcel_fuzzer/parcel_fuzzer.h
index 765a93e8c9..dbd0caea01 100644
--- a/libs/binder/tests/parcel_fuzzer/parcel_fuzzer.h
+++ b/libs/binder/tests/parcel_fuzzer/parcel_fuzzer.h
@@ -15,9 +15,13 @@
  */
 #pragma once
 
+#include <fuzzbinder/random_parcel.h>
 #include <fuzzer/FuzzedDataProvider.h>
 
 #include <functional>
 
 template <typename P>
 using ParcelRead = std::function<void(const P& p, FuzzedDataProvider& provider)>;
+template <typename P>
+using ParcelWrite = std::function<void(P& p, FuzzedDataProvider& provider,
+                                       android::RandomParcelOptions* options)>;