Merge "libbinder: remove gMaxFds from Parcel" am: 50219f1d21

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

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

2 files changed, 52 insertions, 15 deletions

diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index 038ce381e2..e67dd7b5e2 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -87,7 +87,8 @@ static_assert(sizeof(Parcel) == 60);
 static std::atomic<size_t> gParcelGlobalAllocCount;
 static std::atomic<size_t> gParcelGlobalAllocSize;
 
-static size_t gMaxFds = 0;
+// Maximum number of file descriptors per Parcel.
+constexpr size_t kMaxFds = 1024;
 
 // Maximum size of a blob to transfer in-place.
 static const size_t BLOB_INPLACE_LIMIT = 16 * 1024;
@@ -1416,7 +1417,7 @@ status_t Parcel::write(const FlattenableHelperInterface& val)
     const size_t len = val.getFlattenedSize();
     const size_t fd_count = val.getFdCount();
 
-    if ((len > INT32_MAX) || (fd_count >= gMaxFds)) {
+    if ((len > INT32_MAX) || (fd_count > kMaxFds)) {
         // don't accept size_t values which may have come from an
         // inadvertent conversion from a negative int.
         return BAD_VALUE;
@@ -2158,7 +2159,7 @@ status_t Parcel::read(FlattenableHelperInterface& val) const
     const size_t len = this->readInt32();
     const size_t fd_count = this->readInt32();
 
-    if ((len > INT32_MAX) || (fd_count >= gMaxFds)) {
+    if ((len > INT32_MAX) || (fd_count > kMaxFds)) {
         // don't accept size_t values which may have come from an
         // inadvertent conversion from a negative int.
         return BAD_VALUE;
@@ -2747,18 +2748,6 @@ void Parcel::initState()
     mAllowFds = true;
     mDeallocZero = false;
     mOwner = nullptr;
-
-    // racing multiple init leads only to multiple identical write
-    if (gMaxFds == 0) {
-        struct rlimit result;
-        if (!getrlimit(RLIMIT_NOFILE, &result)) {
-            gMaxFds = (size_t)result.rlim_cur;
-            //ALOGI("parcel fd limit set to %zu", gMaxFds);
-        } else {
-            ALOGW("Unable to getrlimit: %s", strerror(errno));
-            gMaxFds = 1024;
-        }
-    }
 }
 
 void Parcel::scanForFds() const {
diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp
index cf3e6ca01f..4ed3309e0c 100644
--- a/libs/binder/tests/binderLibTest.cpp
+++ b/libs/binder/tests/binderLibTest.cpp
@@ -30,6 +30,7 @@
 #include <android-base/properties.h>
 #include <android-base/result-gmock.h>
 #include <android-base/result.h>
+#include <android-base/scopeguard.h>
 #include <android-base/strings.h>
 #include <android-base/unique_fd.h>
 #include <binder/Binder.h>
@@ -1232,6 +1233,53 @@ TEST_F(BinderLibTest, GotSid) {
     EXPECT_THAT(server->transact(BINDER_LIB_TEST_CAN_GET_SID, data, nullptr), StatusEq(OK));
 }
 
+struct TooManyFdsFlattenable : Flattenable<TooManyFdsFlattenable> {
+    TooManyFdsFlattenable(size_t fdCount) : mFdCount(fdCount) {}
+
+    // Flattenable protocol
+    size_t getFlattenedSize() const {
+        // Return a valid non-zero size here so we don't get an unintended
+        // BAD_VALUE from Parcel::write
+        return 16;
+    }
+    size_t getFdCount() const { return mFdCount; }
+    status_t flatten(void *& /*buffer*/, size_t & /*size*/, int *&fds, size_t &count) const {
+        for (size_t i = 0; i < count; i++) {
+            fds[i] = STDIN_FILENO;
+        }
+        return NO_ERROR;
+    }
+    status_t unflatten(void const *& /*buffer*/, size_t & /*size*/, int const *& /*fds*/,
+                       size_t & /*count*/) {
+        /* This doesn't get called */
+        return NO_ERROR;
+    }
+
+    size_t mFdCount;
+};
+
+TEST_F(BinderLibTest, TooManyFdsFlattenable) {
+    rlimit origNofile;
+    int ret = getrlimit(RLIMIT_NOFILE, &origNofile);
+    ASSERT_EQ(0, ret);
+
+    // Restore the original file limits when the test finishes
+    base::ScopeGuard guardUnguard([&]() { setrlimit(RLIMIT_NOFILE, &origNofile); });
+
+    rlimit testNofile = {1024, 1024};
+    ret = setrlimit(RLIMIT_NOFILE, &testNofile);
+    ASSERT_EQ(0, ret);
+
+    Parcel parcel;
+    // Try to write more file descriptors than supported by the OS
+    TooManyFdsFlattenable tooManyFds1(1024);
+    EXPECT_THAT(parcel.write(tooManyFds1), StatusEq(-EMFILE));
+
+    // Try to write more file descriptors than the internal limit
+    TooManyFdsFlattenable tooManyFds2(1025);
+    EXPECT_THAT(parcel.write(tooManyFds2), StatusEq(BAD_VALUE));
+}
+
 TEST(ServiceNotifications, Unregister) {
     auto sm = defaultServiceManager();
     using LocalRegistrationCallback = IServiceManager::LocalRegistrationCallback;