summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--libs/binder/IPCThreadState.cpp3
-rw-r--r--libs/binder/Parcel.cpp20
-rw-r--r--libs/binder/include/binder/Parcel.h3
-rw-r--r--libs/binder/tests/Android.bp4
-rw-r--r--libs/binder/tests/binderLibTest.cpp163
5 files changed, 189 insertions, 4 deletions
diff --git a/libs/binder/IPCThreadState.cpp b/libs/binder/IPCThreadState.cpp
index 445df9eeff..b08568a61c 100644
--- a/libs/binder/IPCThreadState.cpp
+++ b/libs/binder/IPCThreadState.cpp
@@ -1419,7 +1419,7 @@ status_t IPCThreadState::freeze(pid_t pid, bool enable, uint32_t timeout_ms) {
return ret;
}
-void IPCThreadState::freeBuffer(Parcel* parcel, const uint8_t* data,
+void IPCThreadState::freeBuffer(Parcel* /*parcel*/, const uint8_t* data,
size_t /*dataSize*/,
const binder_size_t* /*objects*/,
size_t /*objectsSize*/)
@@ -1429,7 +1429,6 @@ void IPCThreadState::freeBuffer(Parcel* parcel, const uint8_t* data,
alog << "Writing BC_FREE_BUFFER for " << data << endl;
}
ALOG_ASSERT(data != NULL, "Called with NULL data");
- if (parcel != nullptr) parcel->closeFileDescriptors();
IPCThreadState* state = self();
state->mOut.writeInt32(BC_FREE_BUFFER);
state->mOut.writePointer((uintptr_t)data);
diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index abdd4875af..4d0eb48942 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -2076,11 +2076,15 @@ const flat_binder_object* Parcel::readObject(bool nullMetaData) const
void Parcel::closeFileDescriptors()
{
+ truncateFileDescriptors(0);
+}
+
+void Parcel::truncateFileDescriptors(size_t newObjectsSize) {
size_t i = 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+mObjects[i]);
@@ -2228,6 +2232,7 @@ void Parcel::freeDataNoInit()
if (mOwner) {
LOG_ALLOC("Parcel %p: freeing other owner data", this);
//ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
+ closeFileDescriptors();
mOwner(this, mData, mDataSize, mObjects, mObjectsSize);
} else {
LOG_ALLOC("Parcel %p: freeing allocated data", this);
@@ -2351,8 +2356,9 @@ status_t Parcel::continueWrite(size_t desired)
if (desired == 0) {
objectsSize = 0;
} else {
+ validateReadData(mDataSize); // hack to sort the objects
while (objectsSize > 0) {
- if (mObjects[objectsSize-1] < desired)
+ if (mObjects[objectsSize-1] + sizeof(flat_binder_object) <= desired)
break;
objectsSize--;
}
@@ -2397,8 +2403,18 @@ status_t Parcel::continueWrite(size_t desired)
}
if (objects && mObjects) {
memcpy(objects, 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());
+ truncateFileDescriptors(objectsSize);
mOwner(this, mData, mDataSize, mObjects, mObjectsSize);
mOwner = nullptr;
diff --git a/libs/binder/include/binder/Parcel.h b/libs/binder/include/binder/Parcel.h
index 5aaaa0c3d2..ef8014367e 100644
--- a/libs/binder/include/binder/Parcel.h
+++ b/libs/binder/include/binder/Parcel.h
@@ -528,6 +528,9 @@ public:
void print(TextOutput& 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);
+
typedef void (*release_func)(Parcel* parcel,
const uint8_t* data, size_t dataSize,
const binder_size_t* objects, size_t objectsSize);
diff --git a/libs/binder/tests/Android.bp b/libs/binder/tests/Android.bp
index fb84f04fde..71e9d3d39b 100644
--- a/libs/binder/tests/Android.bp
+++ b/libs/binder/tests/Android.bp
@@ -60,7 +60,9 @@ cc_test {
defaults: ["binder_test_defaults"],
srcs: ["binderLibTest.cpp"],
shared_libs: [
+ "libbase",
"libbinder",
+ "liblog",
"libutils",
],
static_libs: [
@@ -101,7 +103,9 @@ cc_test {
srcs: ["binderLibTest.cpp"],
shared_libs: [
+ "libbase",
"libbinder",
+ "liblog",
"libutils",
],
static_libs: [
diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp
index 3db0a8e793..bc6ec2f85f 100644
--- a/libs/binder/tests/binderLibTest.cpp
+++ b/libs/binder/tests/binderLibTest.cpp
@@ -26,6 +26,8 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
+#include <android-base/logging.h>
+#include <android-base/unique_fd.h>
#include <binder/Binder.h>
#include <binder/IBinder.h>
#include <binder/IPCThreadState.h>
@@ -33,6 +35,7 @@
#include <linux/sched.h>
#include <sys/epoll.h>
+#include <sys/mman.h>
#include <sys/prctl.h>
#include "../binder_module.h"
@@ -41,6 +44,7 @@
#define ARRAY_SIZE(array) (sizeof array / sizeof array[0])
using namespace android;
+using android::base::unique_fd;
using testing::Not;
// e.g. EXPECT_THAT(expr, StatusEq(OK)) << "additional message";
@@ -85,6 +89,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,
@@ -404,6 +410,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, NopTransaction) {
Parcel data, reply;
EXPECT_THAT(m_server->transact(BINDER_LIB_TEST_NOP_TRANSACTION, data, &reply),
@@ -805,6 +840,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;
@@ -1438,6 +1567,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;
generated by cgit v1.2.3-59-g8ed1b (git 2.39.0) at 2025-11-02 23:52:26 +0000