From 72963bc3191455232650e027cf0d90d868fdf36d Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 25 Sep 2024 23:36:04 +0000 Subject: binder_parcel_fuzzer: support write functions This generalizes the append parcel fuzzing functionality, for additional hardening. Fixes: 328161314 Test: manual, run binder_parcel_fuzzer for 3min Ignore-AOSP-First: internal only for security hardening Change-Id: I1f470e0d56241327bff7422d061912b592cc0ddc --- libs/binder/tests/parcel_fuzzer/binder.cpp | 120 ++++++++++++++++++++++++ libs/binder/tests/parcel_fuzzer/binder.h | 1 + libs/binder/tests/parcel_fuzzer/binder_ndk.cpp | 54 +++++++++-- libs/binder/tests/parcel_fuzzer/binder_ndk.h | 1 + libs/binder/tests/parcel_fuzzer/main.cpp | 46 +++++---- libs/binder/tests/parcel_fuzzer/parcel_fuzzer.h | 4 + 6 files changed, 200 insertions(+), 26 deletions(-) (limited to 'libs/binder') diff --git a/libs/binder/tests/parcel_fuzzer/binder.cpp b/libs/binder/tests/parcel_fuzzer/binder.cpp index e378b864f7..a9c1fed756 100644 --- a/libs/binder/tests/parcel_fuzzer/binder.cpp +++ b/libs/binder/tests/parcel_fuzzer/binder.cpp @@ -25,6 +25,8 @@ #include #include #include +#include +#include #include #include "../../Utils.h" @@ -404,5 +406,123 @@ std::vector> BINDER_PARCEL_READ_FUNCTIONS { FUZZ_LOG() << " toString() result: " << toString; }, }; + +std::vector> BINDER_PARCEL_WRITE_FUNCTIONS { + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call setDataSize"; + size_t len = provider.ConsumeIntegralInRange(0, 1024); + p.setDataSize(len); + }, + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call setDataCapacity"; + size_t len = provider.ConsumeIntegralInRange(0, 1024); + p.setDataCapacity(len); + }, + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call setData"; + size_t len = provider.ConsumeIntegralInRange(0, 1024); + std::vector bytes = provider.ConsumeBytes(len); + p.setData(bytes.data(), bytes.size()); + }, + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* options) { + FUZZ_LOG() << "about to call appendFrom"; + + std::vector bytes = provider.ConsumeBytes(provider.ConsumeIntegralInRange(0, 4096)); + ::android::Parcel p2; + fillRandomParcel(&p2, FuzzedDataProvider(bytes.data(), bytes.size()), options); + + int32_t start = provider.ConsumeIntegral(); + int32_t len = provider.ConsumeIntegral(); + p.appendFrom(&p2, start, len); + }, + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call setData"; + size_t len = provider.ConsumeIntegralInRange(0, 1024); + std::vector bytes = provider.ConsumeBytes(len); + p.setData(bytes.data(), bytes.size()); + }, + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call pushAllowFds"; + bool val = provider.ConsumeBool(); + p.pushAllowFds(val); + }, + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call restoreAllowFds"; + bool val = provider.ConsumeBool(); + p.restoreAllowFds(val); + }, + // markForBinder - covered by fillRandomParcel, aborts if called multiple times + // markForRpc - covered by fillRandomParcel, aborts if called multiple times + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call writeInterfaceToken"; + std::string interface = provider.ConsumeRandomLengthString(); + p.writeInterfaceToken(android::String16(interface.c_str())); + }, + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call setEnforceNoDataAvail"; + p.setEnforceNoDataAvail(provider.ConsumeBool()); + }, + [] (::android::Parcel& p, FuzzedDataProvider& /* provider */, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call setServiceFuzzing"; + p.setServiceFuzzing(); + }, + [] (::android::Parcel& p, FuzzedDataProvider& /* provider */, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call freeData"; + p.freeData(); + }, + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call write"; + size_t len = provider.ConsumeIntegralInRange(0, 256); + std::vector bytes = provider.ConsumeBytes(len); + p.write(bytes.data(), bytes.size()); + }, + // write* - write functions all implemented by calling 'write' itself. + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* options) { + FUZZ_LOG() << "about to call writeStrongBinder"; + + // TODO: this logic is somewhat duplicated with random parcel + android::sp binder; + if (provider.ConsumeBool() && options->extraBinders.size() > 0) { + binder = options->extraBinders.at( + provider.ConsumeIntegralInRange(0, options->extraBinders.size() - 1)); + } else { + binder = android::getRandomBinder(&provider); + options->extraBinders.push_back(binder); + } + + p.writeStrongBinder(binder); + }, + [] (::android::Parcel& p, FuzzedDataProvider& /* provider */, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call writeFileDescriptor (no ownership)"; + p.writeFileDescriptor(STDERR_FILENO, false /* takeOwnership */); + }, + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* options) { + FUZZ_LOG() << "about to call writeFileDescriptor (take ownership)"; + std::vector fds = android::getRandomFds(&provider); + if (fds.size() == 0) return; + + p.writeDupFileDescriptor(fds.at(0).get()); + options->extraFds.insert(options->extraFds.end(), + std::make_move_iterator(fds.begin() + 1), + std::make_move_iterator(fds.end())); + }, + // TODO: writeBlob + // TODO: writeDupImmutableBlobFileDescriptor + // TODO: writeObject (or make the API private more likely) + [] (::android::Parcel& p, FuzzedDataProvider& /* provider */, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call writeNoException"; + p.writeNoException(); + }, + [] (::android::Parcel& p, FuzzedDataProvider& /* provider */, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call closeFileDescriptors"; + p.closeFileDescriptors(); + }, + [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call replaceCallingWorkSourceUid"; + uid_t uid = provider.ConsumeIntegral(); + p.replaceCallingWorkSourceUid(uid); + }, +}; + // clang-format on #pragma clang diagnostic pop diff --git a/libs/binder/tests/parcel_fuzzer/binder.h b/libs/binder/tests/parcel_fuzzer/binder.h index 0c51d68a37..b0ac140d3f 100644 --- a/libs/binder/tests/parcel_fuzzer/binder.h +++ b/libs/binder/tests/parcel_fuzzer/binder.h @@ -21,3 +21,4 @@ #include "parcel_fuzzer.h" extern std::vector> BINDER_PARCEL_READ_FUNCTIONS; +extern std::vector> BINDER_PARCEL_WRITE_FUNCTIONS; diff --git a/libs/binder/tests/parcel_fuzzer/binder_ndk.cpp b/libs/binder/tests/parcel_fuzzer/binder_ndk.cpp index 3a1471eabe..d17fc9601c 100644 --- a/libs/binder/tests/parcel_fuzzer/binder_ndk.cpp +++ b/libs/binder/tests/parcel_fuzzer/binder_ndk.cpp @@ -20,8 +20,11 @@ #include "aidl/parcelables/GenericDataParcelable.h" #include "aidl/parcelables/SingleDataParcelable.h" +#include #include #include +#include +#include #include "util.h" @@ -210,16 +213,51 @@ std::vector> BINDER_NDK_PARCEL_READ_FUNCTIONS{ binder_status_t status = AParcel_marshal(p.aParcel(), buffer, start, len); FUZZ_LOG() << "status: " << status; }, - [](const NdkParcelAdapter& /*p*/, FuzzedDataProvider& provider) { - FUZZ_LOG() << "about to unmarshal AParcel"; +}; +std::vector> BINDER_NDK_PARCEL_WRITE_FUNCTIONS{ + [] (NdkParcelAdapter& p, FuzzedDataProvider& provider, android::RandomParcelOptions* options) { + FUZZ_LOG() << "about to call AParcel_writeStrongBinder"; + + // TODO: this logic is somewhat duplicated with random parcel + android::sp binder; + if (provider.ConsumeBool() && options->extraBinders.size() > 0) { + binder = options->extraBinders.at( + provider.ConsumeIntegralInRange(0, options->extraBinders.size() - 1)); + } else { + binder = android::getRandomBinder(&provider); + options->extraBinders.push_back(binder); + } + + ndk::SpAIBinder abinder = ndk::SpAIBinder(AIBinder_fromPlatformBinder(binder)); + AParcel_writeStrongBinder(p.aParcel(), abinder.get()); + }, + [] (NdkParcelAdapter& p, FuzzedDataProvider& provider, android::RandomParcelOptions* options) { + FUZZ_LOG() << "about to call AParcel_writeParcelFileDescriptor"; + + auto fds = android::getRandomFds(&provider); + if (fds.size() == 0) return; + + AParcel_writeParcelFileDescriptor(p.aParcel(), fds.at(0).get()); + options->extraFds.insert(options->extraFds.end(), + std::make_move_iterator(fds.begin() + 1), + std::make_move_iterator(fds.end())); + }, + // all possible data writes can be done as a series of 4-byte reads + [] (NdkParcelAdapter& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call AParcel_writeInt32"; + int32_t val = provider.ConsumeIntegral(); + AParcel_writeInt32(p.aParcel(), val); + }, + [] (NdkParcelAdapter& p, FuzzedDataProvider& /* provider */, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call AParcel_reset"; + AParcel_reset(p.aParcel()); + }, + [](NdkParcelAdapter& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { + FUZZ_LOG() << "about to call AParcel_unmarshal"; size_t len = provider.ConsumeIntegralInRange(0, provider.remaining_bytes()); - std::vector parcelData = provider.ConsumeBytes(len); - const uint8_t* buffer = parcelData.data(); - const size_t bufferLen = parcelData.size(); - NdkParcelAdapter adapter; - binder_status_t status = AParcel_unmarshal(adapter.aParcel(), buffer, bufferLen); + std::vector data = provider.ConsumeBytes(len); + binder_status_t status = AParcel_unmarshal(p.aParcel(), data.data(), data.size()); FUZZ_LOG() << "status: " << status; }, - }; // clang-format on diff --git a/libs/binder/tests/parcel_fuzzer/binder_ndk.h b/libs/binder/tests/parcel_fuzzer/binder_ndk.h index d19f25bc88..0c8b725400 100644 --- a/libs/binder/tests/parcel_fuzzer/binder_ndk.h +++ b/libs/binder/tests/parcel_fuzzer/binder_ndk.h @@ -50,3 +50,4 @@ private: }; extern std::vector> BINDER_NDK_PARCEL_READ_FUNCTIONS; +extern std::vector> BINDER_NDK_PARCEL_WRITE_FUNCTIONS; diff --git a/libs/binder/tests/parcel_fuzzer/main.cpp b/libs/binder/tests/parcel_fuzzer/main.cpp index a57d07fb24..ede0e925ee 100644 --- a/libs/binder/tests/parcel_fuzzer/main.cpp +++ b/libs/binder/tests/parcel_fuzzer/main.cpp @@ -80,6 +80,7 @@ void doTransactFuzz(const char* backend, const sp& binder, FuzzedDataProvider (void)binder->transact(code, data, &reply, flag); } +// start with a Parcel full of data (e.g. like you get from another process) template void doReadFuzz(const char* backend, const std::vector>& reads, FuzzedDataProvider&& provider) { @@ -98,7 +99,7 @@ void doReadFuzz(const char* backend, const std::vector>& reads, fillRandomParcel(&p, std::move(provider), &options); // since we are only using a byte to index - CHECK(reads.size() <= 255) << reads.size(); + CHECK_LE(reads.size(), 255u) << reads.size(); FUZZ_LOG() << "backend: " << backend; FUZZ_LOG() << "input: " << HexString(p.data(), p.dataSize()); @@ -115,26 +116,31 @@ void doReadFuzz(const char* backend, const std::vector>& reads, } } -// Append two random parcels. template -void doAppendFuzz(const char* backend, FuzzedDataProvider&& provider) { - int32_t start = provider.ConsumeIntegral(); - int32_t len = provider.ConsumeIntegral(); - - std::vector bytes = provider.ConsumeBytes( - provider.ConsumeIntegralInRange(0, provider.remaining_bytes())); - - // same options so that FDs and binders could be shared in both Parcels +void doReadWriteFuzz(const char* backend, const std::vector>& reads, + const std::vector>& writes, FuzzedDataProvider&& provider) { RandomParcelOptions options; - P p0, p1; - fillRandomParcel(&p0, FuzzedDataProvider(bytes.data(), bytes.size()), &options); - fillRandomParcel(&p1, std::move(provider), &options); + P p; + fillRandomParcel(&p, std::move(provider), &options); + + // since we are only using a byte to index + CHECK_LE(reads.size() + writes.size(), 255u) << reads.size(); FUZZ_LOG() << "backend: " << backend; - FUZZ_LOG() << "start: " << start << " len: " << len; - p0.appendFrom(&p1, start, len); + while (provider.remaining_bytes() > 0) { + uint8_t idx = provider.ConsumeIntegralInRange(0, reads.size() + writes.size() - 1); + + FUZZ_LOG() << "Instruction " << idx << " avail: " << p.dataAvail() + << " pos: " << p.dataPosition() << " cap: " << p.dataCapacity(); + + if (idx < reads.size()) { + reads.at(idx)(p, provider); + } else { + writes.at(idx - reads.size())(p, provider, &options); + } + } } void* NothingClass_onCreate(void* args) { @@ -156,7 +162,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { FuzzedDataProvider provider = FuzzedDataProvider(data, size); - const std::function fuzzBackend[] = { + const std::function fuzzBackend[] = { [](FuzzedDataProvider&& provider) { doTransactFuzz< ::android::hardware::Parcel>("hwbinder", @@ -187,10 +193,14 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { std::move(provider)); }, [](FuzzedDataProvider&& provider) { - doAppendFuzz<::android::Parcel>("binder", std::move(provider)); + doReadWriteFuzz<::android::Parcel>("binder", BINDER_PARCEL_READ_FUNCTIONS, + BINDER_PARCEL_WRITE_FUNCTIONS, + std::move(provider)); }, [](FuzzedDataProvider&& provider) { - doAppendFuzz("binder_ndk", std::move(provider)); + doReadWriteFuzz("binder_ndk", BINDER_NDK_PARCEL_READ_FUNCTIONS, + BINDER_NDK_PARCEL_WRITE_FUNCTIONS, + std::move(provider)); }, }; diff --git a/libs/binder/tests/parcel_fuzzer/parcel_fuzzer.h b/libs/binder/tests/parcel_fuzzer/parcel_fuzzer.h index 765a93e8c9..dbd0caea01 100644 --- a/libs/binder/tests/parcel_fuzzer/parcel_fuzzer.h +++ b/libs/binder/tests/parcel_fuzzer/parcel_fuzzer.h @@ -15,9 +15,13 @@ */ #pragma once +#include #include #include template using ParcelRead = std::function; +template +using ParcelWrite = std::function; -- cgit v1.2.3-59-g8ed1b From 19ff63eea17a7dbdc1e79e64308fadf7d03ca464 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Tue, 1 Oct 2024 18:27:14 +0000 Subject: binder_parcel_fuzzer: cleanup dups/to remove - setData was represented multiple times - closeFileDescriptors should be private Ignore-AOSP-First: fuzzer work Bug: 328161314 Test: N/A Change-Id: I3c5d4c96c69788bc71a01ad3af971c99b162a970 --- libs/binder/tests/parcel_fuzzer/binder.cpp | 18 ------------------ 1 file changed, 18 deletions(-) (limited to 'libs/binder') diff --git a/libs/binder/tests/parcel_fuzzer/binder.cpp b/libs/binder/tests/parcel_fuzzer/binder.cpp index a9c1fed756..07f0143767 100644 --- a/libs/binder/tests/parcel_fuzzer/binder.cpp +++ b/libs/binder/tests/parcel_fuzzer/binder.cpp @@ -117,14 +117,6 @@ std::vector> BINDER_PARCEL_READ_FUNCTIONS { p.setDataPosition(pos); FUZZ_LOG() << "setDataPosition done"; }, - [] (const ::android::Parcel& p, FuzzedDataProvider& provider) { - size_t len = provider.ConsumeIntegralInRange(0, 1024); - std::vector bytes = provider.ConsumeBytes(len); - FUZZ_LOG() << "about to setData: " <<(bytes.data() ? HexString(bytes.data(), bytes.size()) : "null"); - // TODO: allow all read and write operations - (*const_cast<::android::Parcel*>(&p)).setData(bytes.data(), bytes.size()); - FUZZ_LOG() << "setData done"; - }, PARCEL_READ_NO_STATUS(size_t, allowFds), PARCEL_READ_NO_STATUS(size_t, hasFileDescriptors), PARCEL_READ_NO_STATUS(std::vector>, debugReadAllStrongBinders), @@ -435,12 +427,6 @@ std::vector> BINDER_PARCEL_WRITE_FUNCTIONS { int32_t len = provider.ConsumeIntegral(); p.appendFrom(&p2, start, len); }, - [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { - FUZZ_LOG() << "about to call setData"; - size_t len = provider.ConsumeIntegralInRange(0, 1024); - std::vector bytes = provider.ConsumeBytes(len); - p.setData(bytes.data(), bytes.size()); - }, [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { FUZZ_LOG() << "about to call pushAllowFds"; bool val = provider.ConsumeBool(); @@ -513,10 +499,6 @@ std::vector> BINDER_PARCEL_WRITE_FUNCTIONS { FUZZ_LOG() << "about to call writeNoException"; p.writeNoException(); }, - [] (::android::Parcel& p, FuzzedDataProvider& /* provider */, android::RandomParcelOptions* /*options*/) { - FUZZ_LOG() << "about to call closeFileDescriptors"; - p.closeFileDescriptors(); - }, [] (::android::Parcel& p, FuzzedDataProvider& provider, android::RandomParcelOptions* /*options*/) { FUZZ_LOG() << "about to call replaceCallingWorkSourceUid"; uid_t uid = provider.ConsumeIntegral(); -- cgit v1.2.3-59-g8ed1b From 1021015c7dce890a078e0e9f21db73f000fdc878 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Tue, 1 Oct 2024 19:15:16 +0000 Subject: binder_parcel_fuzzer: avoid consuming all provider In doReadWriteFuzz, it should not call fillRandomParcel, as this consumes the entire provider. It's meant to always start with an empty Parcel, but I forgot to remove this. Ignore-AOSP-First: fuzzer work Bug: 328161314 Test: run fuzzer, starts finding crashes Change-Id: I1ce474a4e39464fd53f6cd9c440b40bd128fada1 --- libs/binder/tests/parcel_fuzzer/main.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'libs/binder') diff --git a/libs/binder/tests/parcel_fuzzer/main.cpp b/libs/binder/tests/parcel_fuzzer/main.cpp index ede0e925ee..192f9d5dce 100644 --- a/libs/binder/tests/parcel_fuzzer/main.cpp +++ b/libs/binder/tests/parcel_fuzzer/main.cpp @@ -96,7 +96,7 @@ void doReadFuzz(const char* backend, const std::vector>& reads, RandomParcelOptions options; P p; - fillRandomParcel(&p, std::move(provider), &options); + fillRandomParcel(&p, std::move(provider), &options); // consumes provider // since we are only using a byte to index CHECK_LE(reads.size(), 255u) << reads.size(); @@ -120,9 +120,7 @@ template void doReadWriteFuzz(const char* backend, const std::vector>& reads, const std::vector>& writes, FuzzedDataProvider&& provider) { RandomParcelOptions options; - P p; - fillRandomParcel(&p, std::move(provider), &options); // since we are only using a byte to index CHECK_LE(reads.size() + writes.size(), 255u) << reads.size(); -- cgit v1.2.3-59-g8ed1b From 608524d462278c2c9f6716cd94f126c85e9f2e91 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 2 Oct 2024 01:00:23 +0000 Subject: libbinder: Parcel: grow rejects large data pos This is unexpected behavior so throw an error. Allocating this much memory may cause OOM or other issues. Bug: 370831157 Test: fuzzer Change-Id: Iea0884ca61b08e52e6a6e9c66693e427cb5536f4 --- libs/binder/Parcel.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'libs/binder') diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 37113629a8..3d36f2eed7 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -2948,6 +2948,14 @@ status_t Parcel::growData(size_t len) return BAD_VALUE; } + if (mDataPos > mDataSize) { + // b/370831157 - this case used to abort. We also don't expect mDataPos < mDataSize, but + // this would only waste a bit of memory, so it's okay. + ALOGE("growData only expected at the end of a Parcel. pos: %zu, size: %zu, capacity: %zu", + mDataPos, len, mDataCapacity); + return BAD_VALUE; + } + if (len > SIZE_MAX - mDataSize) return NO_MEMORY; // overflow if (mDataSize + len > SIZE_MAX / 3) return NO_MEMORY; // overflow size_t newSize = ((mDataSize+len)*3)/2; -- cgit v1.2.3-59-g8ed1b From c54dad65317f851ce9d016bd90ec6a7a04da09fc Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 2 Oct 2024 00:37:59 +0000 Subject: libbinder: Parcel: validate read data before write This is slow, but it's required to prevent memory corruption. Ignore-AOSP-First: security Bug: 370840874 Test: fuzzer Change-Id: Ibc5566ade0389221690dc90324f93394cf7fc9a5 --- libs/binder/Parcel.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'libs/binder') diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 3d36f2eed7..d346ad15d2 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -1211,6 +1211,10 @@ restart_write: //printf("Writing %ld bytes, padded to %ld\n", len, padded); uint8_t* const data = mData+mDataPos; + if (status_t status = validateReadData(mDataPos + padded); status != OK) { + return nullptr; // drops status + } + // Need to pad at end? if (padded != len) { #if BYTE_ORDER == BIG_ENDIAN @@ -1799,6 +1803,10 @@ status_t Parcel::writeObject(const flat_binder_object& val, bool nullMetaData) const bool enoughObjects = kernelFields->mObjectsSize < kernelFields->mObjectsCapacity; if (enoughData && enoughObjects) { restart_write: + if (status_t status = validateReadData(mDataPos + sizeof(val)); status != OK) { + return status; + } + *reinterpret_cast(mData+mDataPos) = val; // remember if it's a file descriptor @@ -2042,6 +2050,10 @@ status_t Parcel::writeAligned(T val) { if ((mDataPos+sizeof(val)) <= mDataCapacity) { restart_write: + if (status_t status = validateReadData(mDataPos + sizeof(val)); status != OK) { + return status; + } + memcpy(mData + mDataPos, &val, sizeof(val)); return finishWrite(sizeof(val)); } -- cgit v1.2.3-59-g8ed1b From f99572ae6cb4cc717a7f716f116daef148ee901d Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 2 Oct 2024 01:19:56 +0000 Subject: libbinder: remove writeUnpadded Unused. Ignore-AOSP-First: security related Bug: 328161314 Test: build Change-Id: I751b8e23c02967dc422f5cd8f696e297bcc5c051 --- libs/binder/Parcel.cpp | 25 ------------------------- libs/binder/include/binder/Parcel.h | 1 - 2 files changed, 26 deletions(-) (limited to 'libs/binder') diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 37113629a8..37c22de0a1 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -1150,31 +1150,6 @@ status_t Parcel::finishWrite(size_t len) return NO_ERROR; } -status_t Parcel::writeUnpadded(const void* data, size_t len) -{ - if (len > INT32_MAX) { - // don't accept size_t values which may have come from an - // inadvertent conversion from a negative int. - return BAD_VALUE; - } - - size_t end = mDataPos + len; - if (end < mDataPos) { - // integer overflow - return BAD_VALUE; - } - - if (end <= mDataCapacity) { -restart_write: - memcpy(mData+mDataPos, data, len); - return finishWrite(len); - } - - status_t err = growData(len); - if (err == NO_ERROR) goto restart_write; - return err; -} - status_t Parcel::write(const void* data, size_t len) { if (len > INT32_MAX) { diff --git a/libs/binder/include/binder/Parcel.h b/libs/binder/include/binder/Parcel.h index e2b23be58f..c48fe3db0f 100644 --- a/libs/binder/include/binder/Parcel.h +++ b/libs/binder/include/binder/Parcel.h @@ -172,7 +172,6 @@ public: LIBBINDER_EXPORTED status_t write(const void* data, size_t len); LIBBINDER_EXPORTED void* writeInplace(size_t len); - LIBBINDER_EXPORTED status_t writeUnpadded(const void* data, size_t len); LIBBINDER_EXPORTED status_t writeInt32(int32_t val); LIBBINDER_EXPORTED status_t writeUint32(uint32_t val); LIBBINDER_EXPORTED status_t writeInt64(int64_t val); -- cgit v1.2.3-59-g8ed1b From 3b685aa34bc16feec26a5f0cdfcd64f01c8cf4cc Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 2 Oct 2024 18:37:18 +0000 Subject: libbinder: better object logs Separate patch, b/c won't be backported Bug: 370840874 Test: N/A Ignore-AOSP-First: security related Change-Id: Iefc49398bab70e7255346dd4a0375b11edc1c159 --- libs/binder/Parcel.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'libs/binder') diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 37113629a8..734b4b6129 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -180,7 +180,7 @@ static void acquire_object(const sp& proc, const flat_binder_objec } } - ALOGD("Invalid object type 0x%08x", obj.hdr.type); + ALOGE("Invalid object type 0x%08x to acquire", obj.hdr.type); } static void release_object(const sp& proc, const flat_binder_object& obj, @@ -210,7 +210,7 @@ static void release_object(const sp& proc, const flat_binder_objec } } - ALOGE("Invalid object type 0x%08x", obj.hdr.type); + ALOGE("Invalid object type 0x%08x to release", obj.hdr.type); } #endif // BINDER_WITH_KERNEL_IPC @@ -1874,7 +1874,10 @@ status_t Parcel::validateReadData(size_t upperBound) const if (mDataPos < kernelFields->mObjects[nextObject] + sizeof(flat_binder_object)) { // Requested info overlaps with an object if (!mServiceFuzzing) { - ALOGE("Attempt to read from protected data in Parcel %p", this); + ALOGE("Attempt to read or write from protected data in Parcel %p. pos: " + "%zu, nextObject: %zu, object offset: %llu, object size: %zu", + this, mDataPos, nextObject, kernelFields->mObjects[nextObject], + sizeof(flat_binder_object)); } return PERMISSION_DENIED; } -- cgit v1.2.3-59-g8ed1b From f2163b846228ded7187358048efb20681614779e Mon Sep 17 00:00:00 2001 From: Frederick Mayle Date: Mon, 30 Sep 2024 17:42:45 -0700 Subject: binder: fix FD handling in continueWrite Only close FDs within the truncated part of the parcel. This change also fixes a bug where a parcel truncated into the middle of an object would not properly free that object. That could have resulted in an OOB access in `Parcel::truncateRpcObjects`, so more bounds checking is added. The new tests show how to reproduce the bug by appending to or partially truncating Parcels owned by the kernel. Two cases are disabled because of a bug in the Parcel fdsan code (b/370824489). Flag: EXEMPT bugfix Ignore-AOSP-First: security fix Bug: 239222407, 359179312 Test: atest binderLibTest Change-Id: Iadf7e2e98e3eb97c56ec2fed2b49d1e6492af9a3 --- libs/binder/Parcel.cpp | 66 ++++++++++++--- libs/binder/include/binder/Parcel.h | 4 +- libs/binder/tests/binderLibTest.cpp | 155 ++++++++++++++++++++++++++++++++++++ 3 files changed, 212 insertions(+), 13 deletions(-) (limited to 'libs/binder') diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 842ea77459..8e989952d0 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -2656,14 +2656,14 @@ const flat_binder_object* Parcel::readObject(bool nullMetaData) const } #endif // BINDER_WITH_KERNEL_IPC -void Parcel::closeFileDescriptors() { +void Parcel::closeFileDescriptors(size_t newObjectsSize) { if (auto* kernelFields = maybeKernelFields()) { #ifdef BINDER_WITH_KERNEL_IPC size_t i = kernelFields->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(mData + kernelFields->mObjects[i]); @@ -2674,6 +2674,7 @@ void Parcel::closeFileDescriptors() { } } #else // BINDER_WITH_KERNEL_IPC + (void)newObjectsSize; LOG_ALWAYS_FATAL("Binder kernel driver disabled at build time"); #endif // BINDER_WITH_KERNEL_IPC } else if (auto* rpcFields = maybeRpcFields()) { @@ -2898,7 +2899,7 @@ void Parcel::freeDataNoInit() //ALOGI("Freeing data ref of %p (pid=%d)", this, getpid()); auto* kernelFields = maybeKernelFields(); // Close FDs before freeing, otherwise they will leak for kernel binder. - closeFileDescriptors(); + closeFileDescriptors(/*newObjectsSize=*/0); mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr, kernelFields ? kernelFields->mObjectsSize : 0); } else { @@ -3035,13 +3036,38 @@ status_t Parcel::continueWrite(size_t desired) objectsSize = 0; } else { if (kernelFields) { +#ifdef BINDER_WITH_KERNEL_IPC + validateReadData(mDataSize); // hack to sort the objects while (objectsSize > 0) { - if (kernelFields->mObjects[objectsSize - 1] < desired) break; + if (kernelFields->mObjects[objectsSize - 1] + sizeof(flat_binder_object) <= + desired) + break; objectsSize--; } +#endif // BINDER_WITH_KERNEL_IPC } else { while (objectsSize > 0) { - if (rpcFields->mObjectPositions[objectsSize - 1] < desired) break; + // Object size varies by type. + uint32_t pos = rpcFields->mObjectPositions[objectsSize - 1]; + size_t size = sizeof(RpcFields::ObjectType); + uint32_t minObjectEnd; + if (__builtin_add_overflow(pos, sizeof(RpcFields::ObjectType), &minObjectEnd) || + minObjectEnd > mDataSize) { + return BAD_VALUE; + } + const auto type = *reinterpret_cast(mData + pos); + switch (type) { + case RpcFields::TYPE_BINDER_NULL: + break; + case RpcFields::TYPE_BINDER: + size += sizeof(uint64_t); // address + break; + case RpcFields::TYPE_NATIVE_FILE_DESCRIPTOR: + size += sizeof(int32_t); // fd index + break; + } + + if (pos + size <= desired) break; objectsSize--; } } @@ -3090,15 +3116,24 @@ status_t Parcel::continueWrite(size_t desired) if (mData) { memcpy(data, mData, mDataSize < desired ? mDataSize : desired); } +#ifdef BINDER_WITH_KERNEL_IPC if (objects && kernelFields && kernelFields->mObjects) { memcpy(objects, kernelFields->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(data + objects[i]); + if (flat->hdr.type == BINDER_TYPE_FD) { + flat->cookie = 1; + } + } } // ALOGI("Freeing data ref of %p (pid=%d)", this, getpid()); if (kernelFields) { - // TODO(b/239222407): This seems wrong. We should only free FDs when - // they are in a truncated section of the parcel. - closeFileDescriptors(); + closeFileDescriptors(objectsSize); } +#endif // BINDER_WITH_KERNEL_IPC mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr, kernelFields ? kernelFields->mObjectsSize : 0); mOwner = nullptr; @@ -3225,11 +3260,19 @@ status_t Parcel::truncateRpcObjects(size_t newObjectsSize) { } while (rpcFields->mObjectPositions.size() > newObjectsSize) { uint32_t pos = rpcFields->mObjectPositions.back(); - rpcFields->mObjectPositions.pop_back(); + uint32_t minObjectEnd; + if (__builtin_add_overflow(pos, sizeof(RpcFields::ObjectType), &minObjectEnd) || + minObjectEnd > mDataSize) { + return BAD_VALUE; + } const auto type = *reinterpret_cast(mData + pos); if (type == RpcFields::TYPE_NATIVE_FILE_DESCRIPTOR) { - const auto fdIndex = - *reinterpret_cast(mData + pos + sizeof(RpcFields::ObjectType)); + uint32_t objectEnd; + if (__builtin_add_overflow(minObjectEnd, sizeof(int32_t), &objectEnd) || + objectEnd > mDataSize) { + return BAD_VALUE; + } + const auto fdIndex = *reinterpret_cast(mData + minObjectEnd); if (rpcFields->mFds == nullptr || fdIndex < 0 || static_cast(fdIndex) >= rpcFields->mFds->size()) { ALOGE("RPC Parcel contains invalid file descriptor index. index=%d fd_count=%zu", @@ -3239,6 +3282,7 @@ status_t Parcel::truncateRpcObjects(size_t newObjectsSize) { // In practice, this always removes the last element. rpcFields->mFds->erase(rpcFields->mFds->begin() + fdIndex); } + rpcFields->mObjectPositions.pop_back(); } return OK; } diff --git a/libs/binder/include/binder/Parcel.h b/libs/binder/include/binder/Parcel.h index ed4e211481..50a58e9b45 100644 --- a/libs/binder/include/binder/Parcel.h +++ b/libs/binder/include/binder/Parcel.h @@ -648,8 +648,8 @@ public: LIBBINDER_EXPORTED void print(std::ostream& to, uint32_t flags = 0) const; private: - // Explicitly close all file descriptors in the parcel. - void closeFileDescriptors(); + // Close all file descriptors in the parcel at object positions >= newObjectsSize. + void closeFileDescriptors(size_t newObjectsSize); // `objects` and `objectsSize` always 0 for RPC Parcels. typedef void (*release_func)(const uint8_t* data, size_t dataSize, const binder_size_t* objects, diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp index bcab6decca..e152763be1 100644 --- a/libs/binder/tests/binderLibTest.cpp +++ b/libs/binder/tests/binderLibTest.cpp @@ -46,6 +46,7 @@ #include #include +#include #include #include #include @@ -110,6 +111,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, @@ -536,6 +539,30 @@ class TestDeathRecipient : public IBinder::DeathRecipient, public BinderLibTestE }; }; +ssize_t countFds() { + return std::distance(std::filesystem::directory_iterator("/proc/self/fd"), + std::filesystem::directory_iterator{}); +} + +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, CannotUseBinderAfterFork) { // EXPECT_DEATH works by forking the process EXPECT_DEATH({ ProcessState::self(); }, "libbinder ProcessState can not be used after fork"); @@ -1175,6 +1202,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 strong = new BBinder(); wp weak = strong; @@ -2224,6 +2345,40 @@ public: 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()) { + PLOGE("memfd_create failed"); + return UNKNOWN_ERROR; + } + unique_fd fd2(memfd_create("memfd2", MFD_CLOEXEC)); + if (!fd2.ok()) { + PLOGE("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; -- cgit v1.2.3-59-g8ed1b From 74befc9eaef3521ea575776e1ed2c75d49fb8b8e Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 20 Nov 2024 22:58:37 +0000 Subject: libbinder: deprecated notice for *CString These APIs are super custom and not used by AIDL. Don't manually write parceling code, and don't use CString with binder for sure either. Bug: 376674798 Test: N/A Change-Id: I8520c24f3a23f0e65e17c08e4e719dd410037c06 --- libs/binder/include/binder/Parcel.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'libs/binder') diff --git a/libs/binder/include/binder/Parcel.h b/libs/binder/include/binder/Parcel.h index ceab20a275..0c7366e683 100644 --- a/libs/binder/include/binder/Parcel.h +++ b/libs/binder/include/binder/Parcel.h @@ -178,7 +178,8 @@ public: LIBBINDER_EXPORTED status_t writeUint64(uint64_t val); LIBBINDER_EXPORTED status_t writeFloat(float val); LIBBINDER_EXPORTED status_t writeDouble(double val); - LIBBINDER_EXPORTED status_t writeCString(const char* str); + LIBBINDER_EXPORTED status_t writeCString(const char* str) + __attribute__((deprecated("use AIDL, writeString* instead"))); LIBBINDER_EXPORTED status_t writeString8(const String8& str); LIBBINDER_EXPORTED status_t writeString8(const char* str, size_t len); LIBBINDER_EXPORTED status_t writeString16(const String16& str); @@ -434,7 +435,8 @@ public: LIBBINDER_EXPORTED status_t readUtf8FromUtf16(std::unique_ptr* str) const __attribute__((deprecated("use std::optional version instead"))); - LIBBINDER_EXPORTED const char* readCString() const; + LIBBINDER_EXPORTED const char* readCString() const + __attribute__((deprecated("use AIDL, use readString*"))); LIBBINDER_EXPORTED String8 readString8() const; LIBBINDER_EXPORTED status_t readString8(String8* pArg) const; LIBBINDER_EXPORTED const char* readString8Inplace(size_t* outLen) const; -- cgit v1.2.3-59-g8ed1b From 75885d9f92d19a2d7778a33f19a0c15fd5e13d14 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 20 Nov 2024 22:56:20 +0000 Subject: readCString: implemented with readInplace All read logic must go through and be validated in the same few places. Bug: 376674798 Test: binderClearBufTest covers this, and in presubmit Change-Id: Icc0ade84b671ecd3026069d8f672ff254d58e995 --- libs/binder/Parcel.cpp | 4 +--- libs/binder/tests/binderParcelUnitTest.cpp | 32 ++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) (limited to 'libs/binder') diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 96d821e196..a5f416f1ba 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -2223,9 +2223,7 @@ const char* Parcel::readCString() const const char* eos = reinterpret_cast(memchr(str, 0, avail)); if (eos) { const size_t len = eos - str; - mDataPos += pad_size(len+1); - ALOGV("readCString Setting data pos of %p to %zu", this, mDataPos); - return str; + return static_cast(readInplace(len + 1)); } } return nullptr; diff --git a/libs/binder/tests/binderParcelUnitTest.cpp b/libs/binder/tests/binderParcelUnitTest.cpp index 32a70e5b11..6259d9d2d2 100644 --- a/libs/binder/tests/binderParcelUnitTest.cpp +++ b/libs/binder/tests/binderParcelUnitTest.cpp @@ -33,6 +33,38 @@ using android::String8; using android::binder::Status; using android::binder::unique_fd; +static void checkCString(const char* str) { + for (size_t i = 0; i < 3; i++) { + Parcel p; + + for (size_t j = 0; j < i; j++) p.writeInt32(3); + + p.writeCString(str); + int32_t pos = p.dataPosition(); + + p.setDataPosition(0); + + for (size_t j = 0; j < i; j++) p.readInt32(); + const char* str2 = p.readCString(); + + ASSERT_EQ(std::string(str), str2); + ASSERT_EQ(pos, p.dataPosition()); + } +} + +TEST(Parcel, TestReadCString) { + // we should remove the *CString APIs, but testing them until + // they are deleted. + checkCString(""); + checkCString("a"); + checkCString("\n"); + checkCString("32"); + checkCString("321"); + checkCString("3210"); + checkCString("3210b"); + checkCString("123434"); +} + TEST(Parcel, NonNullTerminatedString8) { String8 kTestString = String8("test-is-good"); -- cgit v1.2.3-59-g8ed1b