commit fc22150c32d1133b05bccf6979e5c9712562b725
author Andrei Homescu <ahomescu@google.com> Sat Oct 08 03:51:17 2022 +0000
committer Andrei Homescu <ahomescu@google.com> Tue Nov 29 03:01:20 2022 +0000
tree 3b6ea6d29c608016b32e42eeca63bb6e6ab2d9d8
parent 52f7b14988e406564c3bf3b2a9c0ae42a3ccfbdb

libbinder: Additional dupFileDescriptor use in Parcel.cpp

Use the new dupFileDescriptor API in one additional place
in Parcel.cpp to make the code more portable.

Bug: 230135749
Test: presubmit
Change-Id: I41c18c1e319eb7ad06c90d6a495bad31bd677a6d

libs/binder/Parcel.cpp

diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index 07d0a65..8870f1a 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -614,11 +614,14 @@
                 if (status_t status = readInt32(&fdIndex); status != OK) {
                     return status;
                 }
-                const auto& oldFd = otherRpcFields->mFds->at(fdIndex);
+                int oldFd = toRawFd(otherRpcFields->mFds->at(fdIndex));
                 // To match kernel binder behavior, we always dup, even if the
                 // FD was unowned in the source parcel.
-                rpcFields->mFds->emplace_back(
-                        base::unique_fd(fcntl(toRawFd(oldFd), F_DUPFD_CLOEXEC, 0)));
+                int newFd = -1;
+                if (status_t status = dupFileDescriptor(oldFd, &newFd); status != OK) {
+                    ALOGW("Failed to duplicate file descriptor %d: %s", oldFd, strerror(-status));
+                }
+                rpcFields->mFds->emplace_back(base::unique_fd(newFd));
                 // Fixup the index in the data.
                 mDataPos = newDataPos + 4;
                 if (status_t status = writeInt32(rpcFields->mFds->size() - 1); status != OK) {

libs/binder/tests/binderRpcTest.cpp

diff --git a/libs/binder/tests/binderRpcTest.cpp b/libs/binder/tests/binderRpcTest.cpp
index 79bd9d4..89526a6 100644
--- a/libs/binder/tests/binderRpcTest.cpp
+++ b/libs/binder/tests/binderRpcTest.cpp
@@ -912,6 +912,33 @@
     EXPECT_EQ(status.transactionError(), BAD_VALUE) << status;
 }
 
+TEST_P(BinderRpc, AppendInvalidFd) {
+    auto proc = createRpcTestSocketServerProcess({
+            .clientFileDescriptorTransportMode = RpcSession::FileDescriptorTransportMode::UNIX,
+            .serverSupportedFileDescriptorTransportModes =
+                    {RpcSession::FileDescriptorTransportMode::UNIX},
+    });
+
+    int badFd = fcntl(STDERR_FILENO, F_DUPFD_CLOEXEC, 0);
+    ASSERT_NE(badFd, -1);
+
+    // Close the file descriptor so it becomes invalid for dup
+    close(badFd);
+
+    Parcel p1;
+    p1.markForBinder(proc.rootBinder);
+    p1.writeInt32(3);
+    EXPECT_EQ(OK, p1.writeFileDescriptor(badFd, false));
+
+    Parcel pRaw;
+    pRaw.markForBinder(proc.rootBinder);
+    EXPECT_EQ(OK, pRaw.appendFrom(&p1, 0, p1.dataSize()));
+
+    pRaw.setDataPosition(0);
+    EXPECT_EQ(3, pRaw.readInt32());
+    ASSERT_EQ(-1, pRaw.readFileDescriptor());
+}
+
 TEST_P(BinderRpc, WorksWithLibbinderNdkPing) {
     if constexpr (!kEnableSharedLibs) {
         GTEST_SKIP() << "Test disabled because Binder was built as a static library";