Skip to content

Commit 155e128

Browse files
fkm3android-build-merge-worker-robot
fkm3
authored and
android-build-merge-worker-robot
committed
Merge "binder: fix FD handling in continueWrite" into tm-dev am: b4e50ff
Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/native/+/29867657 Change-Id: I2fb1902b89570e39080c60c2731119c3da8827ea Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
2 parents e95795f + b4e50ff commit 155e128

4 files changed

Lines changed: 184 additions & 4 deletions

File tree

‎libs/binder/IPCThreadState.cpp‎

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

1446-
void IPCThreadState::freeBuffer(Parcel* parcel, const uint8_t* data,
1446+
void IPCThreadState::freeBuffer(Parcel* /*parcel*/, const uint8_t* data,
14471447
size_t /*dataSize*/,
14481448
const binder_size_t* /*objects*/,
14491449
size_t /*objectsSize*/)
@@ -1453,7 +1453,6 @@ void IPCThreadState::freeBuffer(Parcel* parcel, const uint8_t* data,
14531453
alog << "Writing BC_FREE_BUFFER for " << data << endl;
14541454
}
14551455
ALOG_ASSERT(data != NULL, "Called with NULL data");
1456-
if (parcel != nullptr) parcel->closeFileDescriptors();
14571456
IPCThreadState* state = self();
14581457
state->mOut.writeInt32(BC_FREE_BUFFER);
14591458
state->mOut.writePointer((uintptr_t)data);

‎libs/binder/Parcel.cpp‎

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

21942194
void Parcel::closeFileDescriptors()
21952195
{
2196+
truncateFileDescriptors(0);
2197+
}
2198+
2199+
void Parcel::truncateFileDescriptors(size_t newObjectsSize) {
21962200
size_t i = mObjectsSize;
21972201
if (i > 0) {
21982202
//ALOGI("Closing file descriptors for %zu objects...", i);
21992203
}
2200-
while (i > 0) {
2204+
while (i > newObjectsSize) {
22012205
i--;
22022206
const flat_binder_object* flat
22032207
= reinterpret_cast<flat_binder_object*>(mData+mObjects[i]);
@@ -2345,6 +2349,7 @@ void Parcel::freeDataNoInit()
23452349
if (mOwner) {
23462350
LOG_ALLOC("Parcel %p: freeing other owner data", this);
23472351
//ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
2352+
closeFileDescriptors();
23482353
mOwner(this, mData, mDataSize, mObjects, mObjectsSize);
23492354
} else {
23502355
LOG_ALLOC("Parcel %p: freeing allocated data", this);
@@ -2468,8 +2473,9 @@ status_t Parcel::continueWrite(size_t desired)
24682473
if (desired == 0) {
24692474
objectsSize = 0;
24702475
} else {
2476+
validateReadData(mDataSize); // hack to sort the objects
24712477
while (objectsSize > 0) {
2472-
if (mObjects[objectsSize-1] < desired)
2478+
if (mObjects[objectsSize-1] + sizeof(flat_binder_object) <= desired)
24732479
break;
24742480
objectsSize--;
24752481
}
@@ -2514,8 +2520,18 @@ status_t Parcel::continueWrite(size_t desired)
25142520
}
25152521
if (objects && mObjects) {
25162522
memcpy(objects, mObjects, objectsSize*sizeof(binder_size_t));
2523+
// All FDs are owned when `mOwner`, even when `cookie == 0`. When
2524+
// we switch to `!mOwner`, we need to explicitly mark the FDs as
2525+
// owned.
2526+
for (size_t i = 0; i < objectsSize; i++) {
2527+
flat_binder_object* flat = reinterpret_cast<flat_binder_object*>(data + objects[i]);
2528+
if (flat->hdr.type == BINDER_TYPE_FD) {
2529+
flat->cookie = 1;
2530+
}
2531+
}
25172532
}
25182533
//ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
2534+
truncateFileDescriptors(objectsSize);
25192535
mOwner(this, mData, mDataSize, mObjects, mObjectsSize);
25202536
mOwner = nullptr;
25212537

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

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

594594
private:
595+
// Close all file descriptors in the parcel at object positions >= newObjectsSize.
596+
__attribute__((__visibility__("hidden"))) void truncateFileDescriptors(size_t newObjectsSize);
597+
595598
typedef void (*release_func)(Parcel* parcel,
596599
const uint8_t* data, size_t dataSize,
597600
const binder_size_t* objects, size_t objectsSize);

‎libs/binder/tests/binderLibTest.cpp‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
#include <gmock/gmock.h>
2828
#include <gtest/gtest.h>
2929

30+
#include <android-base/logging.h>
3031
#include <android-base/properties.h>
3132
#include <android-base/result-gmock.h>
3233
#include <android-base/result.h>
@@ -42,6 +43,7 @@
4243

4344
#include <linux/sched.h>
4445
#include <sys/epoll.h>
46+
#include <sys/mman.h>
4547
#include <sys/prctl.h>
4648
#include <sys/socket.h>
4749
#include <sys/un.h>
@@ -56,6 +58,7 @@ using namespace std::string_literals;
5658
using namespace std::chrono_literals;
5759
using android::base::testing::HasValue;
5860
using android::base::testing::Ok;
61+
using android::base::unique_fd;
5962
using testing::ExplainMatchResult;
6063
using testing::Matcher;
6164
using testing::Not;
@@ -104,6 +107,8 @@ enum BinderLibTestTranscationCode {
104107
BINDER_LIB_TEST_LINK_DEATH_TRANSACTION,
105108
BINDER_LIB_TEST_WRITE_FILE_TRANSACTION,
106109
BINDER_LIB_TEST_WRITE_PARCEL_FILE_DESCRIPTOR_TRANSACTION,
110+
BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION,
111+
BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION,
107112
BINDER_LIB_TEST_EXIT_TRANSACTION,
108113
BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION,
109114
BINDER_LIB_TEST_GET_PTR_SIZE_TRANSACTION,
@@ -437,6 +442,35 @@ class TestDeathRecipient : public IBinder::DeathRecipient, public BinderLibTestE
437442
};
438443
};
439444

445+
ssize_t countFds() {
446+
DIR* dir = opendir("/proc/self/fd/");
447+
if (dir == nullptr) return -1;
448+
ssize_t ret = 0;
449+
dirent* ent;
450+
while ((ent = readdir(dir)) != nullptr) ret++;
451+
closedir(dir);
452+
return ret;
453+
}
454+
455+
struct FdLeakDetector {
456+
int startCount;
457+
458+
FdLeakDetector() {
459+
// This log statement is load bearing. We have to log something before
460+
// counting FDs to make sure the logging system is initialized, otherwise
461+
// the sockets it opens will look like a leak.
462+
ALOGW("FdLeakDetector counting FDs.");
463+
startCount = countFds();
464+
}
465+
~FdLeakDetector() {
466+
int endCount = countFds();
467+
if (startCount != endCount) {
468+
ADD_FAILURE() << "fd count changed (" << startCount << " -> " << endCount
469+
<< ") fd leak?";
470+
}
471+
}
472+
};
473+
440474
TEST_F(BinderLibTest, CannotUseBinderAfterFork) {
441475
// EXPECT_DEATH works by forking the process
442476
EXPECT_DEATH({ ProcessState::self(); }, "libbinder ProcessState can not be used after fork");
@@ -852,6 +886,100 @@ TEST_F(BinderLibTest, PassParcelFileDescriptor) {
852886
EXPECT_EQ(0, read(read_end.get(), readbuf.data(), datasize));
853887
}
854888

889+
TEST_F(BinderLibTest, RecvOwnedFileDescriptors) {
890+
FdLeakDetector fd_leak_detector;
891+
892+
Parcel data;
893+
Parcel reply;
894+
EXPECT_EQ(NO_ERROR,
895+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
896+
&reply));
897+
unique_fd a, b;
898+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
899+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
900+
}
901+
902+
// Used to trigger fdsan error (b/239222407).
903+
TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndWriteInt) {
904+
GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
905+
906+
FdLeakDetector fd_leak_detector;
907+
908+
Parcel data;
909+
Parcel reply;
910+
EXPECT_EQ(NO_ERROR,
911+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
912+
&reply));
913+
reply.setDataPosition(reply.dataSize());
914+
reply.writeInt32(0);
915+
reply.setDataPosition(0);
916+
unique_fd a, b;
917+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
918+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
919+
}
920+
921+
// Used to trigger fdsan error (b/239222407).
922+
TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndTruncate) {
923+
GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
924+
925+
FdLeakDetector fd_leak_detector;
926+
927+
Parcel data;
928+
Parcel reply;
929+
EXPECT_EQ(NO_ERROR,
930+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
931+
&reply));
932+
reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
933+
unique_fd a, b;
934+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
935+
EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
936+
}
937+
938+
TEST_F(BinderLibTest, RecvUnownedFileDescriptors) {
939+
FdLeakDetector fd_leak_detector;
940+
941+
Parcel data;
942+
Parcel reply;
943+
EXPECT_EQ(NO_ERROR,
944+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
945+
&reply));
946+
unique_fd a, b;
947+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
948+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
949+
}
950+
951+
// Used to trigger fdsan error (b/239222407).
952+
TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndWriteInt) {
953+
FdLeakDetector fd_leak_detector;
954+
955+
Parcel data;
956+
Parcel reply;
957+
EXPECT_EQ(NO_ERROR,
958+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
959+
&reply));
960+
reply.setDataPosition(reply.dataSize());
961+
reply.writeInt32(0);
962+
reply.setDataPosition(0);
963+
unique_fd a, b;
964+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
965+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
966+
}
967+
968+
// Used to trigger fdsan error (b/239222407).
969+
TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndTruncate) {
970+
FdLeakDetector fd_leak_detector;
971+
972+
Parcel data;
973+
Parcel reply;
974+
EXPECT_EQ(NO_ERROR,
975+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
976+
&reply));
977+
reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
978+
unique_fd a, b;
979+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
980+
EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
981+
}
982+
855983
TEST_F(BinderLibTest, PromoteLocal) {
856984
sp<IBinder> strong = new BBinder();
857985
wp<IBinder> weak = strong;
@@ -1600,6 +1728,40 @@ class BinderLibTestService : public BBinder {
16001728
if (ret != size) return UNKNOWN_ERROR;
16011729
return NO_ERROR;
16021730
}
1731+
case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION: {
1732+
unique_fd fd1(memfd_create("memfd1", MFD_CLOEXEC));
1733+
if (!fd1.ok()) {
1734+
PLOG(ERROR) << "memfd_create failed";
1735+
return UNKNOWN_ERROR;
1736+
}
1737+
unique_fd fd2(memfd_create("memfd2", MFD_CLOEXEC));
1738+
if (!fd2.ok()) {
1739+
PLOG(ERROR) << "memfd_create failed";
1740+
return UNKNOWN_ERROR;
1741+
}
1742+
status_t ret;
1743+
ret = reply->writeFileDescriptor(fd1.release(), true);
1744+
if (ret != NO_ERROR) {
1745+
return ret;
1746+
}
1747+
ret = reply->writeFileDescriptor(fd2.release(), true);
1748+
if (ret != NO_ERROR) {
1749+
return ret;
1750+
}
1751+
return NO_ERROR;
1752+
}
1753+
case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION: {
1754+
status_t ret;
1755+
ret = reply->writeFileDescriptor(STDOUT_FILENO, false);
1756+
if (ret != NO_ERROR) {
1757+
return ret;
1758+
}
1759+
ret = reply->writeFileDescriptor(STDERR_FILENO, false);
1760+
if (ret != NO_ERROR) {
1761+
return ret;
1762+
}
1763+
return NO_ERROR;
1764+
}
16031765
case BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION:
16041766
alarm(10);
16051767
return NO_ERROR;

0 commit comments

Comments
 (0)