binder: fix FD handling in continueWrite am: e32c1ab

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/native/+/29868448

Change-Id: I68e89c1ffb49122d1e9693e068bda3746c2ce588
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

Author: fkm3

libs/binder/Parcel.cpp

@@ -2505,13 +2505,17 @@ const flat_binder_object* Parcel::readObject(bool nullMetaData) const
 #endif // BINDER_WITH_KERNEL_IPC
 
 void Parcel::closeFileDescriptors() {
+    truncateFileDescriptors(0);
+}
+
+void Parcel::truncateFileDescriptors(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]);
@@ -2521,6 +2525,7 @@ void Parcel::closeFileDescriptors() {
             }
         }
 #else  // BINDER_WITH_KERNEL_IPC
+        (void)newObjectsSize;
         LOG_ALWAYS_FATAL("Binder kernel driver disabled at build time");
 #endif // BINDER_WITH_KERNEL_IPC
     } else if (auto* rpcFields = maybeRpcFields()) {
@@ -2870,13 +2875,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--;
                 }
             }
@@ -2925,15 +2955,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();
+            truncateFileDescriptors(objectsSize);
         }
+#endif // BINDER_WITH_KERNEL_IPC
         mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,
                kernelFields ? kernelFields->mObjectsSize : 0);
         mOwner = nullptr;
@@ -3060,11 +3099,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",
@@ -3074,6 +3121,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;
 }

libs/binder/include/binder/Parcel.h

@@ -602,6 +602,9 @@ class Parcel {
     void print(std::ostream& to, uint32_t flags = 0) const;
 
 private:
+    // Close all file descriptors in the parcel at object positions >= newObjectsSize.
+    __attribute__((__visibility__("hidden"))) void truncateFileDescriptors(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,
                                  size_t objectsSize);

libs/binder/tests/binderLibTest.cpp

@@ -27,6 +27,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
+#include <android-base/logging.h>
 #include <android-base/properties.h>
 #include <android-base/result-gmock.h>
 #include <android-base/result.h>
@@ -43,6 +44,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>
@@ -57,6 +59,7 @@ using namespace std::string_literals;
 using namespace std::chrono_literals;
 using android::base::testing::HasValue;
 using android::base::testing::Ok;
+using android::base::unique_fd;
 using testing::ExplainMatchResult;
 using testing::Matcher;
 using testing::Not;
@@ -106,6 +109,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,
@@ -445,6 +450,35 @@ class TestDeathRecipient : public IBinder::DeathRecipient, public BinderLibTestE
         };
 };
 
+ssize_t countFds() {
+    DIR* dir = opendir("/proc/self/fd/");
+    if (dir == nullptr) return -1;
+    ssize_t ret = 0;
+    dirent* ent;
+    while ((ent = readdir(dir)) != nullptr) ret++;
+    closedir(dir);
+    return ret;
+}
+
+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");
@@ -872,6 +906,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;
@@ -1800,6 +1928,40 @@ class BinderLibTestService : public BBinder {
                 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()) {
+                    PLOG(ERROR) << "memfd_create failed";
+                    return UNKNOWN_ERROR;
+                }
+                unique_fd fd2(memfd_create("memfd2", MFD_CLOEXEC));
+                if (!fd2.ok()) {
+                    PLOG(ERROR) << "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;