Skip to content

Commit b9e4ef8

Browse files
fkm3Android (Google) Code Review
fkm3
authored and
Android (Google) Code Review
committed
Merge "binder: fix FD handling in continueWrite" into sc-v2-dev
2 parents 009229d + aff1772 commit b9e4ef8

5 files changed

Lines changed: 189 additions & 4 deletions

File tree

‎libs/binder/IPCThreadState.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1438,7 +1438,7 @@ status_t IPCThreadState::freeze(pid_t pid, bool enable, uint32_t timeout_ms) {
14381438
return ret;
14391439
}
14401440

1441-
void IPCThreadState::freeBuffer(Parcel* parcel, const uint8_t* data,
1441+
void IPCThreadState::freeBuffer(Parcel* /*parcel*/, const uint8_t* data,
14421442
size_t /*dataSize*/,
14431443
const binder_size_t* /*objects*/,
14441444
size_t /*objectsSize*/)
@@ -1448,7 +1448,6 @@ void IPCThreadState::freeBuffer(Parcel* parcel, const uint8_t* data,
14481448
alog << "Writing BC_FREE_BUFFER for " << data << endl;
14491449
}
14501450
ALOG_ASSERT(data != NULL, "Called with NULL data");
1451-
if (parcel != nullptr) parcel->closeFileDescriptors();
14521451
IPCThreadState* state = self();
14531452
state->mOut.writeInt32(BC_FREE_BUFFER);
14541453
state->mOut.writePointer((uintptr_t)data);

‎libs/binder/Parcel.cpp‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2076,11 +2076,15 @@ const flat_binder_object* Parcel::readObject(bool nullMetaData) const
20762076

20772077
void Parcel::closeFileDescriptors()
20782078
{
2079+
truncateFileDescriptors(0);
2080+
}
2081+
2082+
void Parcel::truncateFileDescriptors(size_t newObjectsSize) {
20792083
size_t i = mObjectsSize;
20802084
if (i > 0) {
20812085
//ALOGI("Closing file descriptors for %zu objects...", i);
20822086
}
2083-
while (i > 0) {
2087+
while (i > newObjectsSize) {
20842088
i--;
20852089
const flat_binder_object* flat
20862090
= reinterpret_cast<flat_binder_object*>(mData+mObjects[i]);
@@ -2228,6 +2232,7 @@ void Parcel::freeDataNoInit()
22282232
if (mOwner) {
22292233
LOG_ALLOC("Parcel %p: freeing other owner data", this);
22302234
//ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
2235+
closeFileDescriptors();
22312236
mOwner(this, mData, mDataSize, mObjects, mObjectsSize);
22322237
} else {
22332238
LOG_ALLOC("Parcel %p: freeing allocated data", this);
@@ -2351,8 +2356,9 @@ status_t Parcel::continueWrite(size_t desired)
23512356
if (desired == 0) {
23522357
objectsSize = 0;
23532358
} else {
2359+
validateReadData(mDataSize); // hack to sort the objects
23542360
while (objectsSize > 0) {
2355-
if (mObjects[objectsSize-1] < desired)
2361+
if (mObjects[objectsSize-1] + sizeof(flat_binder_object) <= desired)
23562362
break;
23572363
objectsSize--;
23582364
}
@@ -2397,8 +2403,18 @@ status_t Parcel::continueWrite(size_t desired)
23972403
}
23982404
if (objects && mObjects) {
23992405
memcpy(objects, mObjects, objectsSize*sizeof(binder_size_t));
2406+
// All FDs are owned when `mOwner`, even when `cookie == 0`. When
2407+
// we switch to `!mOwner`, we need to explicitly mark the FDs as
2408+
// owned.
2409+
for (size_t i = 0; i < objectsSize; i++) {
2410+
flat_binder_object* flat = reinterpret_cast<flat_binder_object*>(data + objects[i]);
2411+
if (flat->hdr.type == BINDER_TYPE_FD) {
2412+
flat->cookie = 1;
2413+
}
2414+
}
24002415
}
24012416
//ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
2417+
truncateFileDescriptors(objectsSize);
24022418
mOwner(this, mData, mDataSize, mObjects, mObjectsSize);
24032419
mOwner = nullptr;
24042420

‎libs/binder/include/binder/Parcel.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,9 @@ class Parcel {
528528
void print(TextOutput& to, uint32_t flags = 0) const;
529529

530530
private:
531+
// Close all file descriptors in the parcel at object positions >= newObjectsSize.
532+
__attribute__((__visibility__("hidden"))) void truncateFileDescriptors(size_t newObjectsSize);
533+
531534
typedef void (*release_func)(Parcel* parcel,
532535
const uint8_t* data, size_t dataSize,
533536
const binder_size_t* objects, size_t objectsSize);

‎libs/binder/tests/Android.bp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,9 @@ cc_test {
6060
defaults: ["binder_test_defaults"],
6161
srcs: ["binderLibTest.cpp"],
6262
shared_libs: [
63+
"libbase",
6364
"libbinder",
65+
"liblog",
6466
"libutils",
6567
],
6668
static_libs: [
@@ -101,7 +103,9 @@ cc_test {
101103

102104
srcs: ["binderLibTest.cpp"],
103105
shared_libs: [
106+
"libbase",
104107
"libbinder",
108+
"liblog",
105109
"libutils",
106110
],
107111
static_libs: [

‎libs/binder/tests/binderLibTest.cpp‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,16 @@
2626
#include <gmock/gmock.h>
2727
#include <gtest/gtest.h>
2828

29+
#include <android-base/logging.h>
30+
#include <android-base/unique_fd.h>
2931
#include <binder/Binder.h>
3032
#include <binder/IBinder.h>
3133
#include <binder/IPCThreadState.h>
3234
#include <binder/IServiceManager.h>
3335

3436
#include <linux/sched.h>
3537
#include <sys/epoll.h>
38+
#include <sys/mman.h>
3639
#include <sys/prctl.h>
3740

3841
#include "../binder_module.h"
@@ -41,6 +44,7 @@
4144
#define ARRAY_SIZE(array) (sizeof array / sizeof array[0])
4245

4346
using namespace android;
47+
using android::base::unique_fd;
4448
using testing::Not;
4549

4650
// e.g. EXPECT_THAT(expr, StatusEq(OK)) << "additional message";
@@ -85,6 +89,8 @@ enum BinderLibTestTranscationCode {
8589
BINDER_LIB_TEST_LINK_DEATH_TRANSACTION,
8690
BINDER_LIB_TEST_WRITE_FILE_TRANSACTION,
8791
BINDER_LIB_TEST_WRITE_PARCEL_FILE_DESCRIPTOR_TRANSACTION,
92+
BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION,
93+
BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION,
8894
BINDER_LIB_TEST_EXIT_TRANSACTION,
8995
BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION,
9096
BINDER_LIB_TEST_GET_PTR_SIZE_TRANSACTION,
@@ -404,6 +410,35 @@ class TestDeathRecipient : public IBinder::DeathRecipient, public BinderLibTestE
404410
};
405411
};
406412

413+
ssize_t countFds() {
414+
DIR* dir = opendir("/proc/self/fd/");
415+
if (dir == nullptr) return -1;
416+
ssize_t ret = 0;
417+
dirent* ent;
418+
while ((ent = readdir(dir)) != nullptr) ret++;
419+
closedir(dir);
420+
return ret;
421+
}
422+
423+
struct FdLeakDetector {
424+
int startCount;
425+
426+
FdLeakDetector() {
427+
// This log statement is load bearing. We have to log something before
428+
// counting FDs to make sure the logging system is initialized, otherwise
429+
// the sockets it opens will look like a leak.
430+
ALOGW("FdLeakDetector counting FDs.");
431+
startCount = countFds();
432+
}
433+
~FdLeakDetector() {
434+
int endCount = countFds();
435+
if (startCount != endCount) {
436+
ADD_FAILURE() << "fd count changed (" << startCount << " -> " << endCount
437+
<< ") fd leak?";
438+
}
439+
}
440+
};
441+
407442
TEST_F(BinderLibTest, NopTransaction) {
408443
Parcel data, reply;
409444
EXPECT_THAT(m_server->transact(BINDER_LIB_TEST_NOP_TRANSACTION, data, &reply),
@@ -812,6 +847,100 @@ TEST_F(BinderLibTest, PassParcelFileDescriptor) {
812847
EXPECT_EQ(0, read(read_end.get(), readbuf.data(), datasize));
813848
}
814849

850+
TEST_F(BinderLibTest, RecvOwnedFileDescriptors) {
851+
FdLeakDetector fd_leak_detector;
852+
853+
Parcel data;
854+
Parcel reply;
855+
EXPECT_EQ(NO_ERROR,
856+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
857+
&reply));
858+
unique_fd a, b;
859+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
860+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
861+
}
862+
863+
// Used to trigger fdsan error (b/239222407).
864+
TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndWriteInt) {
865+
GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
866+
867+
FdLeakDetector fd_leak_detector;
868+
869+
Parcel data;
870+
Parcel reply;
871+
EXPECT_EQ(NO_ERROR,
872+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
873+
&reply));
874+
reply.setDataPosition(reply.dataSize());
875+
reply.writeInt32(0);
876+
reply.setDataPosition(0);
877+
unique_fd a, b;
878+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
879+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
880+
}
881+
882+
// Used to trigger fdsan error (b/239222407).
883+
TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndTruncate) {
884+
GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
885+
886+
FdLeakDetector fd_leak_detector;
887+
888+
Parcel data;
889+
Parcel reply;
890+
EXPECT_EQ(NO_ERROR,
891+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
892+
&reply));
893+
reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
894+
unique_fd a, b;
895+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
896+
EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
897+
}
898+
899+
TEST_F(BinderLibTest, RecvUnownedFileDescriptors) {
900+
FdLeakDetector fd_leak_detector;
901+
902+
Parcel data;
903+
Parcel reply;
904+
EXPECT_EQ(NO_ERROR,
905+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
906+
&reply));
907+
unique_fd a, b;
908+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
909+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
910+
}
911+
912+
// Used to trigger fdsan error (b/239222407).
913+
TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndWriteInt) {
914+
FdLeakDetector fd_leak_detector;
915+
916+
Parcel data;
917+
Parcel reply;
918+
EXPECT_EQ(NO_ERROR,
919+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
920+
&reply));
921+
reply.setDataPosition(reply.dataSize());
922+
reply.writeInt32(0);
923+
reply.setDataPosition(0);
924+
unique_fd a, b;
925+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
926+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
927+
}
928+
929+
// Used to trigger fdsan error (b/239222407).
930+
TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndTruncate) {
931+
FdLeakDetector fd_leak_detector;
932+
933+
Parcel data;
934+
Parcel reply;
935+
EXPECT_EQ(NO_ERROR,
936+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
937+
&reply));
938+
reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
939+
unique_fd a, b;
940+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
941+
EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
942+
}
943+
815944
TEST_F(BinderLibTest, PromoteLocal) {
816945
sp<IBinder> strong = new BBinder();
817946
wp<IBinder> weak = strong;
@@ -1445,6 +1574,40 @@ class BinderLibTestService : public BBinder
14451574
if (ret != size) return UNKNOWN_ERROR;
14461575
return NO_ERROR;
14471576
}
1577+
case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION: {
1578+
unique_fd fd1(memfd_create("memfd1", MFD_CLOEXEC));
1579+
if (!fd1.ok()) {
1580+
PLOG(ERROR) << "memfd_create failed";
1581+
return UNKNOWN_ERROR;
1582+
}
1583+
unique_fd fd2(memfd_create("memfd2", MFD_CLOEXEC));
1584+
if (!fd2.ok()) {
1585+
PLOG(ERROR) << "memfd_create failed";
1586+
return UNKNOWN_ERROR;
1587+
}
1588+
status_t ret;
1589+
ret = reply->writeFileDescriptor(fd1.release(), true);
1590+
if (ret != NO_ERROR) {
1591+
return ret;
1592+
}
1593+
ret = reply->writeFileDescriptor(fd2.release(), true);
1594+
if (ret != NO_ERROR) {
1595+
return ret;
1596+
}
1597+
return NO_ERROR;
1598+
}
1599+
case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION: {
1600+
status_t ret;
1601+
ret = reply->writeFileDescriptor(STDOUT_FILENO, false);
1602+
if (ret != NO_ERROR) {
1603+
return ret;
1604+
}
1605+
ret = reply->writeFileDescriptor(STDERR_FILENO, false);
1606+
if (ret != NO_ERROR) {
1607+
return ret;
1608+
}
1609+
return NO_ERROR;
1610+
}
14481611
case BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION:
14491612
alarm(10);
14501613
return NO_ERROR;

0 commit comments

Comments
 (0)