Skip to content

Commit 4e10596

Browse files
fkm3Android (Google) Code Review
fkm3
authored and
Android (Google) Code Review
committed
Merge "binder: fix FD handling in continueWrite" into main
2 parents 6a3c7c9 + f2163b8 commit 4e10596

3 files changed

Lines changed: 212 additions & 13 deletions

File tree

‎libs/binder/Parcel.cpp‎

Lines changed: 55 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2656,14 +2656,14 @@ const flat_binder_object* Parcel::readObject(bool nullMetaData) const
26562656
}
26572657
#endif // BINDER_WITH_KERNEL_IPC
26582658

2659-
void Parcel::closeFileDescriptors() {
2659+
void Parcel::closeFileDescriptors(size_t newObjectsSize) {
26602660
if (auto* kernelFields = maybeKernelFields()) {
26612661
#ifdef BINDER_WITH_KERNEL_IPC
26622662
size_t i = kernelFields->mObjectsSize;
26632663
if (i > 0) {
26642664
// ALOGI("Closing file descriptors for %zu objects...", i);
26652665
}
2666-
while (i > 0) {
2666+
while (i > newObjectsSize) {
26672667
i--;
26682668
const flat_binder_object* flat =
26692669
reinterpret_cast<flat_binder_object*>(mData + kernelFields->mObjects[i]);
@@ -2674,6 +2674,7 @@ void Parcel::closeFileDescriptors() {
26742674
}
26752675
}
26762676
#else // BINDER_WITH_KERNEL_IPC
2677+
(void)newObjectsSize;
26772678
LOG_ALWAYS_FATAL("Binder kernel driver disabled at build time");
26782679
#endif // BINDER_WITH_KERNEL_IPC
26792680
} else if (auto* rpcFields = maybeRpcFields()) {
@@ -2898,7 +2899,7 @@ void Parcel::freeDataNoInit()
28982899
//ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
28992900
auto* kernelFields = maybeKernelFields();
29002901
// Close FDs before freeing, otherwise they will leak for kernel binder.
2901-
closeFileDescriptors();
2902+
closeFileDescriptors(/*newObjectsSize=*/0);
29022903
mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,
29032904
kernelFields ? kernelFields->mObjectsSize : 0);
29042905
} else {
@@ -3035,13 +3036,38 @@ status_t Parcel::continueWrite(size_t desired)
30353036
objectsSize = 0;
30363037
} else {
30373038
if (kernelFields) {
3039+
#ifdef BINDER_WITH_KERNEL_IPC
3040+
validateReadData(mDataSize); // hack to sort the objects
30383041
while (objectsSize > 0) {
3039-
if (kernelFields->mObjects[objectsSize - 1] < desired) break;
3042+
if (kernelFields->mObjects[objectsSize - 1] + sizeof(flat_binder_object) <=
3043+
desired)
3044+
break;
30403045
objectsSize--;
30413046
}
3047+
#endif // BINDER_WITH_KERNEL_IPC
30423048
} else {
30433049
while (objectsSize > 0) {
3044-
if (rpcFields->mObjectPositions[objectsSize - 1] < desired) break;
3050+
// Object size varies by type.
3051+
uint32_t pos = rpcFields->mObjectPositions[objectsSize - 1];
3052+
size_t size = sizeof(RpcFields::ObjectType);
3053+
uint32_t minObjectEnd;
3054+
if (__builtin_add_overflow(pos, sizeof(RpcFields::ObjectType), &minObjectEnd) ||
3055+
minObjectEnd > mDataSize) {
3056+
return BAD_VALUE;
3057+
}
3058+
const auto type = *reinterpret_cast<const RpcFields::ObjectType*>(mData + pos);
3059+
switch (type) {
3060+
case RpcFields::TYPE_BINDER_NULL:
3061+
break;
3062+
case RpcFields::TYPE_BINDER:
3063+
size += sizeof(uint64_t); // address
3064+
break;
3065+
case RpcFields::TYPE_NATIVE_FILE_DESCRIPTOR:
3066+
size += sizeof(int32_t); // fd index
3067+
break;
3068+
}
3069+
3070+
if (pos + size <= desired) break;
30453071
objectsSize--;
30463072
}
30473073
}
@@ -3090,15 +3116,24 @@ status_t Parcel::continueWrite(size_t desired)
30903116
if (mData) {
30913117
memcpy(data, mData, mDataSize < desired ? mDataSize : desired);
30923118
}
3119+
#ifdef BINDER_WITH_KERNEL_IPC
30933120
if (objects && kernelFields && kernelFields->mObjects) {
30943121
memcpy(objects, kernelFields->mObjects, objectsSize * sizeof(binder_size_t));
3122+
// All FDs are owned when `mOwner`, even when `cookie == 0`. When
3123+
// we switch to `!mOwner`, we need to explicitly mark the FDs as
3124+
// owned.
3125+
for (size_t i = 0; i < objectsSize; i++) {
3126+
flat_binder_object* flat = reinterpret_cast<flat_binder_object*>(data + objects[i]);
3127+
if (flat->hdr.type == BINDER_TYPE_FD) {
3128+
flat->cookie = 1;
3129+
}
3130+
}
30953131
}
30963132
// ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
30973133
if (kernelFields) {
3098-
// TODO(b/239222407): This seems wrong. We should only free FDs when
3099-
// they are in a truncated section of the parcel.
3100-
closeFileDescriptors();
3134+
closeFileDescriptors(objectsSize);
31013135
}
3136+
#endif // BINDER_WITH_KERNEL_IPC
31023137
mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,
31033138
kernelFields ? kernelFields->mObjectsSize : 0);
31043139
mOwner = nullptr;
@@ -3225,11 +3260,19 @@ status_t Parcel::truncateRpcObjects(size_t newObjectsSize) {
32253260
}
32263261
while (rpcFields->mObjectPositions.size() > newObjectsSize) {
32273262
uint32_t pos = rpcFields->mObjectPositions.back();
3228-
rpcFields->mObjectPositions.pop_back();
3263+
uint32_t minObjectEnd;
3264+
if (__builtin_add_overflow(pos, sizeof(RpcFields::ObjectType), &minObjectEnd) ||
3265+
minObjectEnd > mDataSize) {
3266+
return BAD_VALUE;
3267+
}
32293268
const auto type = *reinterpret_cast<const RpcFields::ObjectType*>(mData + pos);
32303269
if (type == RpcFields::TYPE_NATIVE_FILE_DESCRIPTOR) {
3231-
const auto fdIndex =
3232-
*reinterpret_cast<const int32_t*>(mData + pos + sizeof(RpcFields::ObjectType));
3270+
uint32_t objectEnd;
3271+
if (__builtin_add_overflow(minObjectEnd, sizeof(int32_t), &objectEnd) ||
3272+
objectEnd > mDataSize) {
3273+
return BAD_VALUE;
3274+
}
3275+
const auto fdIndex = *reinterpret_cast<const int32_t*>(mData + minObjectEnd);
32333276
if (rpcFields->mFds == nullptr || fdIndex < 0 ||
32343277
static_cast<size_t>(fdIndex) >= rpcFields->mFds->size()) {
32353278
ALOGE("RPC Parcel contains invalid file descriptor index. index=%d fd_count=%zu",
@@ -3239,6 +3282,7 @@ status_t Parcel::truncateRpcObjects(size_t newObjectsSize) {
32393282
// In practice, this always removes the last element.
32403283
rpcFields->mFds->erase(rpcFields->mFds->begin() + fdIndex);
32413284
}
3285+
rpcFields->mObjectPositions.pop_back();
32423286
}
32433287
return OK;
32443288
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -648,8 +648,8 @@ class Parcel {
648648
LIBBINDER_EXPORTED void print(std::ostream& to, uint32_t flags = 0) const;
649649

650650
private:
651-
// Explicitly close all file descriptors in the parcel.
652-
void closeFileDescriptors();
651+
// Close all file descriptors in the parcel at object positions >= newObjectsSize.
652+
void closeFileDescriptors(size_t newObjectsSize);
653653

654654
// `objects` and `objectsSize` always 0 for RPC Parcels.
655655
typedef void (*release_func)(const uint8_t* data, size_t dataSize, const binder_size_t* objects,

‎libs/binder/tests/binderLibTest.cpp‎

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646

4747
#include <linux/sched.h>
4848
#include <sys/epoll.h>
49+
#include <sys/mman.h>
4950
#include <sys/prctl.h>
5051
#include <sys/socket.h>
5152
#include <sys/un.h>
@@ -110,6 +111,8 @@ enum BinderLibTestTranscationCode {
110111
BINDER_LIB_TEST_LINK_DEATH_TRANSACTION,
111112
BINDER_LIB_TEST_WRITE_FILE_TRANSACTION,
112113
BINDER_LIB_TEST_WRITE_PARCEL_FILE_DESCRIPTOR_TRANSACTION,
114+
BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION,
115+
BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION,
113116
BINDER_LIB_TEST_EXIT_TRANSACTION,
114117
BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION,
115118
BINDER_LIB_TEST_GET_PTR_SIZE_TRANSACTION,
@@ -536,6 +539,30 @@ class TestDeathRecipient : public IBinder::DeathRecipient, public BinderLibTestE
536539
};
537540
};
538541

542+
ssize_t countFds() {
543+
return std::distance(std::filesystem::directory_iterator("/proc/self/fd"),
544+
std::filesystem::directory_iterator{});
545+
}
546+
547+
struct FdLeakDetector {
548+
int startCount;
549+
550+
FdLeakDetector() {
551+
// This log statement is load bearing. We have to log something before
552+
// counting FDs to make sure the logging system is initialized, otherwise
553+
// the sockets it opens will look like a leak.
554+
ALOGW("FdLeakDetector counting FDs.");
555+
startCount = countFds();
556+
}
557+
~FdLeakDetector() {
558+
int endCount = countFds();
559+
if (startCount != endCount) {
560+
ADD_FAILURE() << "fd count changed (" << startCount << " -> " << endCount
561+
<< ") fd leak?";
562+
}
563+
}
564+
};
565+
539566
TEST_F(BinderLibTest, CannotUseBinderAfterFork) {
540567
// EXPECT_DEATH works by forking the process
541568
EXPECT_DEATH({ ProcessState::self(); }, "libbinder ProcessState can not be used after fork");
@@ -1175,6 +1202,100 @@ TEST_F(BinderLibTest, PassParcelFileDescriptor) {
11751202
EXPECT_EQ(0, read(read_end.get(), readbuf.data(), datasize));
11761203
}
11771204

1205+
TEST_F(BinderLibTest, RecvOwnedFileDescriptors) {
1206+
FdLeakDetector fd_leak_detector;
1207+
1208+
Parcel data;
1209+
Parcel reply;
1210+
EXPECT_EQ(NO_ERROR,
1211+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
1212+
&reply));
1213+
unique_fd a, b;
1214+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1215+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
1216+
}
1217+
1218+
// Used to trigger fdsan error (b/239222407).
1219+
TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndWriteInt) {
1220+
GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
1221+
1222+
FdLeakDetector fd_leak_detector;
1223+
1224+
Parcel data;
1225+
Parcel reply;
1226+
EXPECT_EQ(NO_ERROR,
1227+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
1228+
&reply));
1229+
reply.setDataPosition(reply.dataSize());
1230+
reply.writeInt32(0);
1231+
reply.setDataPosition(0);
1232+
unique_fd a, b;
1233+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1234+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
1235+
}
1236+
1237+
// Used to trigger fdsan error (b/239222407).
1238+
TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndTruncate) {
1239+
GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
1240+
1241+
FdLeakDetector fd_leak_detector;
1242+
1243+
Parcel data;
1244+
Parcel reply;
1245+
EXPECT_EQ(NO_ERROR,
1246+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
1247+
&reply));
1248+
reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
1249+
unique_fd a, b;
1250+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1251+
EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
1252+
}
1253+
1254+
TEST_F(BinderLibTest, RecvUnownedFileDescriptors) {
1255+
FdLeakDetector fd_leak_detector;
1256+
1257+
Parcel data;
1258+
Parcel reply;
1259+
EXPECT_EQ(NO_ERROR,
1260+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
1261+
&reply));
1262+
unique_fd a, b;
1263+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1264+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
1265+
}
1266+
1267+
// Used to trigger fdsan error (b/239222407).
1268+
TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndWriteInt) {
1269+
FdLeakDetector fd_leak_detector;
1270+
1271+
Parcel data;
1272+
Parcel reply;
1273+
EXPECT_EQ(NO_ERROR,
1274+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
1275+
&reply));
1276+
reply.setDataPosition(reply.dataSize());
1277+
reply.writeInt32(0);
1278+
reply.setDataPosition(0);
1279+
unique_fd a, b;
1280+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1281+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
1282+
}
1283+
1284+
// Used to trigger fdsan error (b/239222407).
1285+
TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndTruncate) {
1286+
FdLeakDetector fd_leak_detector;
1287+
1288+
Parcel data;
1289+
Parcel reply;
1290+
EXPECT_EQ(NO_ERROR,
1291+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
1292+
&reply));
1293+
reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
1294+
unique_fd a, b;
1295+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1296+
EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
1297+
}
1298+
11781299
TEST_F(BinderLibTest, PromoteLocal) {
11791300
sp<IBinder> strong = new BBinder();
11801301
wp<IBinder> weak = strong;
@@ -2224,6 +2345,40 @@ class BinderLibTestService : public BBinder {
22242345
if (ret != size) return UNKNOWN_ERROR;
22252346
return NO_ERROR;
22262347
}
2348+
case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION: {
2349+
unique_fd fd1(memfd_create("memfd1", MFD_CLOEXEC));
2350+
if (!fd1.ok()) {
2351+
PLOGE("memfd_create failed");
2352+
return UNKNOWN_ERROR;
2353+
}
2354+
unique_fd fd2(memfd_create("memfd2", MFD_CLOEXEC));
2355+
if (!fd2.ok()) {
2356+
PLOGE("memfd_create failed");
2357+
return UNKNOWN_ERROR;
2358+
}
2359+
status_t ret;
2360+
ret = reply->writeFileDescriptor(fd1.release(), true);
2361+
if (ret != NO_ERROR) {
2362+
return ret;
2363+
}
2364+
ret = reply->writeFileDescriptor(fd2.release(), true);
2365+
if (ret != NO_ERROR) {
2366+
return ret;
2367+
}
2368+
return NO_ERROR;
2369+
}
2370+
case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION: {
2371+
status_t ret;
2372+
ret = reply->writeFileDescriptor(STDOUT_FILENO, false);
2373+
if (ret != NO_ERROR) {
2374+
return ret;
2375+
}
2376+
ret = reply->writeFileDescriptor(STDERR_FILENO, false);
2377+
if (ret != NO_ERROR) {
2378+
return ret;
2379+
}
2380+
return NO_ERROR;
2381+
}
22272382
case BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION:
22282383
alarm(10);
22292384
return NO_ERROR;

0 commit comments

Comments
 (0)