diff options
37 files changed, 392 insertions, 126 deletions
diff --git a/.clang-format b/.clang-format index 03af56d640..6725a1fde8 100644 --- a/.clang-format +++ b/.clang-format @@ -11,3 +11,4 @@ ContinuationIndentWidth: 8 IndentWidth: 4 PenaltyBreakBeforeFirstCallParameter: 100000 SpacesBeforeTrailingComments: 1 +IncludeBlocks: Preserve diff --git a/cmds/dumpstate/Android.bp b/cmds/dumpstate/Android.bp index f48f1fb6f8..aff32c38c2 100644 --- a/cmds/dumpstate/Android.bp +++ b/cmds/dumpstate/Android.bp @@ -99,6 +99,7 @@ cc_defaults { "libhidlbase", "liblog", "libutils", + "libbinderdebug", ], srcs: [ "DumpstateService.cpp", diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index c2ae341600..b8df99fd55 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2177,13 +2177,13 @@ void Dumpstate::DumpstateBoard(int out_fd) { } /* - * mount debugfs for non-user builds with ro.product.enforce_debugfs_restrictions + * mount debugfs for non-user builds with ro.product.debugfs_restrictions.enabled * set to true and unmount it after invoking dumpstateBoard_* methods. * This is to enable debug builds to not have debugfs mounted during runtime. * It will also ensure that debugfs is only accessed by the dumpstate HAL. */ auto mount_debugfs = - android::base::GetBoolProperty("ro.product.enforce_debugfs_restrictions", false); + android::base::GetBoolProperty("ro.product.debugfs_restrictions.enabled", false); if (mount_debugfs) { RunCommand("mount debugfs", {"mount", "-t", "debugfs", "debugfs", "/sys/kernel/debug"}, AS_ROOT_20); diff --git a/cmds/dumpsys/Android.bp b/cmds/dumpsys/Android.bp index 91aa018451..6ab6b7f951 100644 --- a/cmds/dumpsys/Android.bp +++ b/cmds/dumpsys/Android.bp @@ -32,6 +32,7 @@ cc_defaults { "libutils", "liblog", "libbinder", + "libbinderdebug", ], static_libs: [ diff --git a/cmds/dumpsys/OWNERS b/cmds/dumpsys/OWNERS index 2a9b681318..4f6a89ebe5 100644 --- a/cmds/dumpsys/OWNERS +++ b/cmds/dumpsys/OWNERS @@ -2,3 +2,6 @@ set noparent nandana@google.com jsharkey@android.com + +# for ServiceManager mock +per-file dumpsys_test.cpp=smoreland@google.com diff --git a/cmds/dumpsys/dumpsys.cpp b/cmds/dumpsys/dumpsys.cpp index a017246184..ba1c449dbf 100644 --- a/cmds/dumpsys/dumpsys.cpp +++ b/cmds/dumpsys/dumpsys.cpp @@ -25,6 +25,7 @@ #include <binder/Parcel.h> #include <binder/ProcessState.h> #include <binder/TextOutput.h> +#include <binderdebug/BinderDebug.h> #include <serviceutils/PriorityDumper.h> #include <utils/Log.h> #include <utils/Vector.h> @@ -60,13 +61,15 @@ static void usage() { "usage: dumpsys\n" " To dump all services.\n" "or:\n" - " dumpsys [-t TIMEOUT] [--priority LEVEL] [--pid] [--help | -l | --skip SERVICES " + " dumpsys [-t TIMEOUT] [--priority LEVEL] [--pid] [--thread] [--help | -l | " + "--skip SERVICES " "| SERVICE [ARGS]]\n" " --help: shows this help\n" " -l: only list services, do not dump them\n" " -t TIMEOUT_SEC: TIMEOUT to use in seconds instead of default 10 seconds\n" " -T TIMEOUT_MS: TIMEOUT to use in milliseconds instead of default 10 seconds\n" " --pid: dump PID instead of usual dump\n" + " --thread: dump thread usage instead of usual dump\n" " --proto: filter services that support dumping data in proto format. Dumps\n" " will be in proto format.\n" " --priority LEVEL: filter services based on specified priority\n" @@ -125,7 +128,8 @@ int Dumpsys::main(int argc, char* const argv[]) { Type type = Type::DUMP; int timeoutArgMs = 10000; int priorityFlags = IServiceManager::DUMP_FLAG_PRIORITY_ALL; - static struct option longOptions[] = {{"pid", no_argument, 0, 0}, + static struct option longOptions[] = {{"thread", no_argument, 0, 0}, + {"pid", no_argument, 0, 0}, {"priority", required_argument, 0, 0}, {"proto", no_argument, 0, 0}, {"skip", no_argument, 0, 0}, @@ -163,6 +167,8 @@ int Dumpsys::main(int argc, char* const argv[]) { } } else if (!strcmp(longOptions[optionIndex].name, "pid")) { type = Type::PID; + } else if (!strcmp(longOptions[optionIndex].name, "thread")) { + type = Type::THREAD; } break; @@ -329,6 +335,23 @@ static status_t dumpPidToFd(const sp<IBinder>& service, const unique_fd& fd) { return OK; } +static status_t dumpThreadsToFd(const sp<IBinder>& service, const unique_fd& fd) { + pid_t pid; + status_t status = service->getDebugPid(&pid); + if (status != OK) { + return status; + } + BinderPidInfo pidInfo; + status = getBinderPidInfo(BinderDebugContext::BINDER, pid, &pidInfo); + if (status != OK) { + return status; + } + WriteStringToFd("Threads in use: " + std::to_string(pidInfo.threadUsage) + "/" + + std::to_string(pidInfo.threadCount) + "\n", + fd.get()); + return OK; +} + status_t Dumpsys::startDumpThread(Type type, const String16& serviceName, const Vector<String16>& args) { sp<IBinder> service = sm_->checkService(serviceName); @@ -359,6 +382,9 @@ status_t Dumpsys::startDumpThread(Type type, const String16& serviceName, case Type::PID: err = dumpPidToFd(service, remote_end); break; + case Type::THREAD: + err = dumpThreadsToFd(service, remote_end); + break; default: std::cerr << "Unknown dump type" << static_cast<int>(type) << std::endl; return; diff --git a/cmds/dumpsys/dumpsys.h b/cmds/dumpsys/dumpsys.h index 929c55c364..349947ce12 100644 --- a/cmds/dumpsys/dumpsys.h +++ b/cmds/dumpsys/dumpsys.h @@ -52,13 +52,14 @@ class Dumpsys { static void setServiceArgs(Vector<String16>& args, bool asProto, int priorityFlags); enum class Type { - DUMP, // dump using `dump` function - PID, // dump pid of server only + DUMP, // dump using `dump` function + PID, // dump pid of server only + THREAD, // dump thread usage of server only }; /** * Starts a thread to connect to a service and get its dump output. The thread redirects - * the output to a pipe. Thread must be stopped by a subsequent callto {@code + * the output to a pipe. Thread must be stopped by a subsequent call to {@code * stopDumpThread}. * @param serviceName * @param args list of arguments to pass to service dump method. diff --git a/cmds/dumpsys/tests/Android.bp b/cmds/dumpsys/tests/Android.bp index 6854c7550e..58fec30c9b 100644 --- a/cmds/dumpsys/tests/Android.bp +++ b/cmds/dumpsys/tests/Android.bp @@ -19,6 +19,7 @@ cc_test { "libbase", "libbinder", "libutils", + "libbinderdebug", ], static_libs: [ @@ -26,6 +27,4 @@ cc_test { "libgmock", "libserviceutils", ], - - clang: true, } diff --git a/cmds/dumpsys/tests/AndroidTest.xml b/cmds/dumpsys/tests/AndroidTest.xml index 1a8c67f7aa..c2351d9aff 100644 --- a/cmds/dumpsys/tests/AndroidTest.xml +++ b/cmds/dumpsys/tests/AndroidTest.xml @@ -23,4 +23,4 @@ <option name="native-test-device-path" value="/data/local/tmp" /> <option name="module-name" value="dumpsys_test" /> </test> -</configuration>
\ No newline at end of file +</configuration> diff --git a/cmds/dumpsys/tests/dumpsys_test.cpp b/cmds/dumpsys/tests/dumpsys_test.cpp index 0ad256471f..c9d2dbb883 100644 --- a/cmds/dumpsys/tests/dumpsys_test.cpp +++ b/cmds/dumpsys/tests/dumpsys_test.cpp @@ -16,12 +16,15 @@ #include "../dumpsys.h" +#include <regex> #include <vector> #include <gmock/gmock.h> #include <gtest/gtest.h> #include <android-base/file.h> +#include <binder/Binder.h> +#include <binder/ProcessState.h> #include <serviceutils/PriorityDumper.h> #include <utils/String16.h> #include <utils/String8.h> @@ -223,6 +226,10 @@ class DumpsysTest : public Test { EXPECT_THAT(stdout_, HasSubstr(expected)); } + void AssertOutputFormat(const std::string format) { + EXPECT_THAT(stdout_, testing::MatchesRegex(format)); + } + void AssertDumped(const std::string& service, const std::string& dump) { EXPECT_THAT(stdout_, HasSubstr("DUMP OF SERVICE " + service + ":\n" + dump)); EXPECT_THAT(stdout_, HasSubstr("was the duration of dumpsys " + service + ", ending at: ")); @@ -575,6 +582,30 @@ TEST_F(DumpsysTest, ListServiceWithPid) { AssertOutput(std::to_string(getpid()) + "\n"); } +// Tests 'dumpsys --thread' +TEST_F(DumpsysTest, ListAllServicesWithThread) { + ExpectListServices({"Locksmith", "Valet"}); + ExpectCheckService("Locksmith"); + ExpectCheckService("Valet"); + + CallMain({"--thread"}); + + AssertRunningServices({"Locksmith", "Valet"}); + + const std::string format("(.|\n)*((Threads in use: [0-9]+/[0-9]+)?\n-(.|\n)*){2}"); + AssertOutputFormat(format); +} + +// Tests 'dumpsys --thread service_name' +TEST_F(DumpsysTest, ListServiceWithThread) { + ExpectCheckService("Locksmith"); + + CallMain({"--thread", "Locksmith"}); + // returns an empty string without root enabled + const std::string format("(^$|Threads in use: [0-9]/[0-9]+\n)"); + AssertOutputFormat(format); +} + TEST_F(DumpsysTest, GetBytesWritten) { const char* serviceName = "service2"; const char* dumpContents = "dump1"; @@ -600,3 +631,13 @@ TEST_F(DumpsysTest, WriteDumpWithoutThreadStart) { /* as_proto = */ false, elapsedDuration, bytesWritten); EXPECT_THAT(status, Eq(INVALID_OPERATION)); } + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + + // start a binder thread pool for testing --thread option + android::ProcessState::self()->setThreadPoolMaxThreadCount(8); + ProcessState::self()->startThreadPool(); + + return RUN_ALL_TESTS(); +} diff --git a/cmds/installd/otapreopt_chroot.cpp b/cmds/installd/otapreopt_chroot.cpp index 3a87776162..83f01de01e 100644 --- a/cmds/installd/otapreopt_chroot.cpp +++ b/cmds/installd/otapreopt_chroot.cpp @@ -179,6 +179,11 @@ static int otapreopt_chroot(const int argc, char **arg) { // want it for product APKs. Same notes as vendor above. TryExtraMount("product", arg[2], "/postinstall/product"); + // Try to mount the system_ext partition. update_engine doesn't do this for + // us, but we want it for system_ext APKs. Same notes as vendor and product + // above. + TryExtraMount("system_ext", arg[2], "/postinstall/system_ext"); + constexpr const char* kPostInstallLinkerconfig = "/postinstall/linkerconfig"; // Try to mount /postinstall/linkerconfig. we will set it up after performing the chroot if (mount("tmpfs", kPostInstallLinkerconfig, "tmpfs", 0, nullptr) != 0) { diff --git a/cmds/lshal/ListCommand.cpp b/cmds/lshal/ListCommand.cpp index d5110f6203..2722e214e8 100644 --- a/cmds/lshal/ListCommand.cpp +++ b/cmds/lshal/ListCommand.cpp @@ -417,7 +417,7 @@ void ListCommand::dumpVintf(const NullableOStream<std::ostream>& out) const { } } out << "-->" << std::endl; - out << vintf::gHalManifestConverter(manifest, vintf::SerializeFlags::HALS_ONLY); + out << vintf::toXml(manifest, vintf::SerializeFlags::HALS_ONLY); } std::string ListCommand::INIT_VINTF_NOTES{ diff --git a/cmds/lshal/test.cpp b/cmds/lshal/test.cpp index 7c1ca91528..6f08f74690 100644 --- a/cmds/lshal/test.cpp +++ b/cmds/lshal/test.cpp @@ -47,8 +47,6 @@ using ::android::hardware::hidl_vec; using ::android::hardware::Void; using android::vintf::Arch; using android::vintf::CompatibilityMatrix; -using android::vintf::gCompatibilityMatrixConverter; -using android::vintf::gHalManifestConverter; using android::vintf::HalManifest; using android::vintf::Transport; using android::vintf::VintfObject; @@ -510,7 +508,7 @@ TEST_F(ListTest, DumpVintf) { std::string error; vintf::HalManifest m; - EXPECT_EQ(true, vintf::gHalManifestConverter(&m, out.str(), &error)) + EXPECT_EQ(true, vintf::fromXml(&m, out.str(), &error)) << "--init-vintf does not emit valid HAL manifest: " << error; } @@ -775,10 +773,10 @@ TEST_F(ListTest, Vintf) { auto deviceMatrix = std::make_shared<CompatibilityMatrix>(); auto frameworkMatrix = std::make_shared<CompatibilityMatrix>(); - ASSERT_TRUE(gHalManifestConverter(deviceManifest.get(), deviceManifestXml)); - ASSERT_TRUE(gHalManifestConverter(frameworkManifest.get(), frameworkManifestXml)); - ASSERT_TRUE(gCompatibilityMatrixConverter(deviceMatrix.get(), deviceMatrixXml)); - ASSERT_TRUE(gCompatibilityMatrixConverter(frameworkMatrix.get(), frameworkMatrixXml)); + ASSERT_TRUE(fromXml(deviceManifest.get(), deviceManifestXml)); + ASSERT_TRUE(fromXml(frameworkManifest.get(), frameworkManifestXml)); + ASSERT_TRUE(fromXml(deviceMatrix.get(), deviceMatrixXml)); + ASSERT_TRUE(fromXml(frameworkMatrix.get(), frameworkMatrixXml)); ON_CALL(*mockList, getDeviceManifest()).WillByDefault(Return(deviceManifest)); ON_CALL(*mockList, getDeviceMatrix()).WillByDefault(Return(deviceMatrix)); @@ -964,7 +962,7 @@ public: " </hal>\n" "</manifest>"; auto manifest = std::make_shared<HalManifest>(); - EXPECT_TRUE(gHalManifestConverter(manifest.get(), mockManifestXml)); + EXPECT_TRUE(fromXml(manifest.get(), mockManifestXml)); EXPECT_CALL(*mockList, getDeviceManifest()) .Times(AnyNumber()) .WillRepeatedly(Return(manifest)); diff --git a/libs/binder/Android.bp b/libs/binder/Android.bp index 79fa6bae30..cba82071d4 100644 --- a/libs/binder/Android.bp +++ b/libs/binder/Android.bp @@ -60,10 +60,6 @@ cc_library_headers { // Currently, these are only on system android (not vendor, not host) // TODO(b/183654927) - move these into separate libraries libbinder_device_interface_sources = [ - "AppOpsManager.cpp", - "IAppOpsCallback.cpp", - "IAppOpsService.cpp", - "IPermissionController.cpp", "PermissionCache.cpp", "PermissionController.cpp", diff --git a/libs/binder/IPCThreadState.cpp b/libs/binder/IPCThreadState.cpp index 406bd54e6f..6fb1227f63 100644 --- a/libs/binder/IPCThreadState.cpp +++ b/libs/binder/IPCThreadState.cpp @@ -90,6 +90,8 @@ static const char *kReturnStrings[] = { "BR_DEAD_BINDER", "BR_CLEAR_DEATH_NOTIFICATION_DONE", "BR_FAILED_REPLY", + "BR_FROZEN_REPLY", + "BR_ONEWAY_SPAM_SUSPECT", "BR_TRANSACTION_SEC_CTX", }; @@ -894,6 +896,11 @@ status_t IPCThreadState::waitForResponse(Parcel *reply, status_t *acquireResult) } switch (cmd) { + case BR_ONEWAY_SPAM_SUSPECT: + ALOGE("Process seems to be sending too many oneway calls."); + CallStack::logStack("oneway spamming", CallStack::getCurrent().get(), + ANDROID_LOG_ERROR); + [[fallthrough]]; case BR_TRANSACTION_COMPLETE: if (!reply && !acquireResult) goto finish; break; diff --git a/libs/binder/ProcessState.cpp b/libs/binder/ProcessState.cpp index a8b2fb2ff8..ca99042342 100644 --- a/libs/binder/ProcessState.cpp +++ b/libs/binder/ProcessState.cpp @@ -43,6 +43,7 @@ #define BINDER_VM_SIZE ((1 * 1024 * 1024) - sysconf(_SC_PAGE_SIZE) * 2) #define DEFAULT_MAX_BINDER_THREADS 15 +#define DEFAULT_ENABLE_ONEWAY_SPAM_DETECTION 1 #ifdef __ANDROID_VNDK__ const char* kDefaultDriver = "/dev/vndbinder"; @@ -358,6 +359,15 @@ status_t ProcessState::setThreadPoolMaxThreadCount(size_t maxThreads) { return result; } +status_t ProcessState::enableOnewaySpamDetection(bool enable) { + uint32_t enableDetection = enable ? 1 : 0; + if (ioctl(mDriverFD, BINDER_ENABLE_ONEWAY_SPAM_DETECTION, &enableDetection) == -1) { + ALOGE("Binder ioctl to enable oneway spam detection failed: %s", strerror(errno)); + return -errno; + } + return NO_ERROR; +} + void ProcessState::giveThreadPoolName() { androidSetThreadName( makeBinderThreadName().string() ); } @@ -388,6 +398,11 @@ static int open_driver(const char *driver) if (result == -1) { ALOGE("Binder ioctl to set max threads failed: %s", strerror(errno)); } + uint32_t enable = DEFAULT_ENABLE_ONEWAY_SPAM_DETECTION; + result = ioctl(fd, BINDER_ENABLE_ONEWAY_SPAM_DETECTION, &enable); + if (result == -1) { + ALOGE("Binder ioctl to enable oneway spam detection failed: %s", strerror(errno)); + } } else { ALOGW("Opening '%s' failed: %s\n", driver, strerror(errno)); } diff --git a/libs/binder/RpcConnection.cpp b/libs/binder/RpcConnection.cpp index 4aff92b58a..1388a801e4 100644 --- a/libs/binder/RpcConnection.cpp +++ b/libs/binder/RpcConnection.cpp @@ -18,6 +18,16 @@ #include <binder/RpcConnection.h> +#include <arpa/inet.h> +#include <netdb.h> +#include <netinet/in.h> +#include <sys/socket.h> +#include <sys/types.h> +#include <sys/un.h> +#include <unistd.h> + +#include <string_view> + #include <binder/Parcel.h> #include <binder/Stability.h> #include <utils/String8.h> @@ -25,11 +35,6 @@ #include "RpcState.h" #include "RpcWireFormat.h" -#include <sys/socket.h> -#include <sys/types.h> -#include <sys/un.h> -#include <unistd.h> - #ifdef __GLIBC__ extern "C" pid_t gettid(); #endif @@ -41,6 +46,7 @@ extern "C" pid_t gettid(); namespace android { using base::unique_fd; +using AddrInfo = std::unique_ptr<addrinfo, decltype(&freeaddrinfo)>; RpcConnection::SocketAddress::~SocketAddress() {} @@ -51,6 +57,10 @@ RpcConnection::RpcConnection() { } RpcConnection::~RpcConnection() { LOG_RPC_DETAIL("RpcConnection destroyed %p", this); + + std::lock_guard<std::mutex> _l(mSocketMutex); + LOG_ALWAYS_FATAL_IF(mServers.size() != 0, + "Should not be able to destroy a connection with servers in use."); } sp<RpcConnection> RpcConnection::make() { @@ -120,6 +130,70 @@ bool RpcConnection::addVsockClient(unsigned int cid, unsigned int port) { #endif // __BIONIC__ +class SocketAddressImpl : public RpcConnection::SocketAddress { +public: + SocketAddressImpl(const sockaddr* addr, size_t size, const String8& desc) + : mAddr(addr), mSize(size), mDesc(desc) {} + [[nodiscard]] std::string toString() const override { + return std::string(mDesc.c_str(), mDesc.size()); + } + [[nodiscard]] const sockaddr* addr() const override { return mAddr; } + [[nodiscard]] size_t addrSize() const override { return mSize; } + void set(const sockaddr* addr, size_t size) { + mAddr = addr; + mSize = size; + } + +private: + const sockaddr* mAddr = nullptr; + size_t mSize = 0; + String8 mDesc; +}; + +AddrInfo GetAddrInfo(const char* addr, unsigned int port) { + addrinfo hint{ + .ai_flags = 0, + .ai_family = AF_UNSPEC, + .ai_socktype = SOCK_STREAM, + .ai_protocol = 0, + }; + addrinfo* aiStart = nullptr; + if (int rc = getaddrinfo(addr, std::to_string(port).data(), &hint, &aiStart); 0 != rc) { + ALOGE("Unable to resolve %s:%u: %s", addr, port, gai_strerror(rc)); + return AddrInfo(nullptr, nullptr); + } + if (aiStart == nullptr) { + ALOGE("Unable to resolve %s:%u: getaddrinfo returns null", addr, port); + return AddrInfo(nullptr, nullptr); + } + return AddrInfo(aiStart, &freeaddrinfo); +} + +bool RpcConnection::setupInetServer(unsigned int port) { + auto aiStart = GetAddrInfo("127.0.0.1", port); + if (aiStart == nullptr) return false; + SocketAddressImpl socketAddress(nullptr, 0, String8::format("127.0.0.1:%u", port)); + for (auto ai = aiStart.get(); ai != nullptr; ai = ai->ai_next) { + socketAddress.set(ai->ai_addr, ai->ai_addrlen); + if (setupSocketServer(socketAddress)) return true; + } + ALOGE("None of the socket address resolved for 127.0.0.1:%u can be set up as inet server.", + port); + return false; +} + +bool RpcConnection::addInetClient(const char* addr, unsigned int port) { + auto aiStart = GetAddrInfo(addr, port); + if (aiStart == nullptr) return false; + SocketAddressImpl socketAddress(nullptr, 0, String8::format("%s:%u", addr, port)); + for (auto ai = aiStart.get(); ai != nullptr; ai = ai->ai_next) { + socketAddress.set(ai->ai_addr, ai->ai_addrlen); + if (addSocketClient(socketAddress)) return true; + } + ALOGE("None of the socket address resolved for %s:%u can be added as inet client.", addr, port); + return false; +} + bool RpcConnection::addNullDebuggingClient() { unique_fd serverFd(TEMP_FAILURE_RETRY(open("/dev/null", O_WRONLY | O_CLOEXEC))); @@ -152,36 +226,35 @@ status_t RpcConnection::sendDecStrong(const RpcAddress& address) { } void RpcConnection::join() { - // establish a connection - { - unique_fd clientFd( - TEMP_FAILURE_RETRY(accept4(mServer.get(), nullptr, 0 /*length*/, SOCK_CLOEXEC))); - if (clientFd < 0) { - // If this log becomes confusing, should save more state from setupUnixDomainServer - // in order to output here. - ALOGE("Could not accept4 socket: %s", strerror(errno)); - return; - } - - LOG_RPC_DETAIL("accept4 on fd %d yields fd %d", mServer.get(), clientFd.get()); - - assignServerToThisThread(std::move(clientFd)); + // TODO(b/185167543): do this dynamically, instead of from a static number + // of threads + unique_fd clientFd( + TEMP_FAILURE_RETRY(accept4(mServer.get(), nullptr, 0 /*length*/, SOCK_CLOEXEC))); + if (clientFd < 0) { + // If this log becomes confusing, should save more state from setupUnixDomainServer + // in order to output here. + ALOGE("Could not accept4 socket: %s", strerror(errno)); + return; } - // We may not use the connection we just established (two threads might - // establish connections for each other), but for now, just use one - // server/socket connection. - ExclusiveSocket socket(sp<RpcConnection>::fromExisting(this), SocketUse::SERVER); + LOG_RPC_DETAIL("accept4 on fd %d yields fd %d", mServer.get(), clientFd.get()); + + // must be registered to allow arbitrary client code executing commands to + // be able to do nested calls (we can't only read from it) + sp<ConnectionSocket> socket = assignServerToThisThread(std::move(clientFd)); while (true) { status_t error = - state()->getAndExecuteCommand(socket.fd(), sp<RpcConnection>::fromExisting(this)); + state()->getAndExecuteCommand(socket->fd, sp<RpcConnection>::fromExisting(this)); if (error != OK) { ALOGI("Binder socket thread closing w/ status %s", statusToString(error).c_str()); - return; + break; } } + + LOG_ALWAYS_FATAL_IF(!removeServerSocket(socket), + "bad state: socket object guaranteed to be in list"); } void RpcConnection::setForServer(const wp<RpcServer>& server) { @@ -246,11 +319,23 @@ void RpcConnection::addClient(unique_fd&& fd) { mClients.push_back(connection); } -void RpcConnection::assignServerToThisThread(unique_fd&& fd) { +sp<RpcConnection::ConnectionSocket> RpcConnection::assignServerToThisThread(unique_fd&& fd) { std::lock_guard<std::mutex> _l(mSocketMutex); sp<ConnectionSocket> connection = sp<ConnectionSocket>::make(); connection->fd = std::move(fd); + connection->exclusiveTid = gettid(); mServers.push_back(connection); + + return connection; +} + +bool RpcConnection::removeServerSocket(const sp<ConnectionSocket>& socket) { + std::lock_guard<std::mutex> _l(mSocketMutex); + if (auto it = std::find(mServers.begin(), mServers.end(), socket); it != mServers.end()) { + mServers.erase(it); + return true; + } + return false; } RpcConnection::ExclusiveSocket::ExclusiveSocket(const sp<RpcConnection>& connection, SocketUse use) @@ -265,37 +350,31 @@ RpcConnection::ExclusiveSocket::ExclusiveSocket(const sp<RpcConnection>& connect // CHECK FOR DEDICATED CLIENT SOCKET // - // A server/looper should always use a dedicated connection. - if (use != SocketUse::SERVER) { - findSocket(tid, &exclusive, &available, mConnection->mClients, - mConnection->mClientsOffset); - - // WARNING: this assumes a server cannot request its client to send - // a transaction, as mServers is excluded below. - // - // Imagine we have more than one thread in play, and a single thread - // sends a synchronous, then an asynchronous command. Imagine the - // asynchronous command is sent on the first client socket. Then, if - // we naively send a synchronous command to that same socket, the - // thread on the far side might be busy processing the asynchronous - // command. So, we move to considering the second available thread - // for subsequent calls. - if (use == SocketUse::CLIENT_ASYNC && (exclusive != nullptr || available != nullptr)) { - mConnection->mClientsOffset = - (mConnection->mClientsOffset + 1) % mConnection->mClients.size(); - } + // A server/looper should always use a dedicated connection if available + findSocket(tid, &exclusive, &available, mConnection->mClients, mConnection->mClientsOffset); + + // WARNING: this assumes a server cannot request its client to send + // a transaction, as mServers is excluded below. + // + // Imagine we have more than one thread in play, and a single thread + // sends a synchronous, then an asynchronous command. Imagine the + // asynchronous command is sent on the first client socket. Then, if + // we naively send a synchronous command to that same socket, the + // thread on the far side might be busy processing the asynchronous + // command. So, we move to considering the second available thread + // for subsequent calls. + if (use == SocketUse::CLIENT_ASYNC && (exclusive != nullptr || available != nullptr)) { + mConnection->mClientsOffset = + (mConnection->mClientsOffset + 1) % mConnection->mClients.size(); } - // USE SERVING SOCKET (to start serving or for nested transaction) + // USE SERVING SOCKET (for nested transaction) // // asynchronous calls cannot be nested if (use != SocketUse::CLIENT_ASYNC) { - // servers should start serving on an available thread only - // otherwise, this should only be a nested call - bool useAvailable = use == SocketUse::SERVER; - - findSocket(tid, &exclusive, (useAvailable ? &available : nullptr), - mConnection->mServers, 0 /* index hint */); + // server sockets are always assigned to a thread + findSocket(tid, &exclusive, nullptr /*available*/, mConnection->mServers, + 0 /* index hint */); } // if our thread is already using a connection, prioritize using that @@ -309,8 +388,6 @@ RpcConnection::ExclusiveSocket::ExclusiveSocket(const sp<RpcConnection>& connect break; } - LOG_ALWAYS_FATAL_IF(use == SocketUse::SERVER, "Must create connection to join one."); - // in regular binder, this would usually be a deadlock :) LOG_ALWAYS_FATAL_IF(mConnection->mClients.size() == 0, "Not a client of any connection. You must create a connection to an " diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp index 755ff35781..d9341369fa 100644 --- a/libs/binder/RpcState.cpp +++ b/libs/binder/RpcState.cpp @@ -40,7 +40,7 @@ status_t RpcState::onBinderLeaving(const sp<RpcConnection>& connection, const sp // We need to be able to send instructions over the socket for how to // connect to a different server, and we also need to let the host // process know that this is happening. - ALOGE("Canot send binder from unrelated binder RPC connection."); + ALOGE("Cannot send binder from unrelated binder RPC connection."); return INVALID_OPERATION; } @@ -498,19 +498,20 @@ status_t RpcState::processTransactInternal(const base::unique_fd& fd, } } - Parcel data; - // transaction->data is owned by this function. Parcel borrows this data and - // only holds onto it for the duration of this function call. Parcel will be - // deleted before the 'transactionData' object. - data.ipcSetDataReference(transaction->data, - transactionData.size() - offsetof(RpcWireTransaction, data), - nullptr /*object*/, 0 /*objectCount*/, do_nothing_to_transact_data); - data.markForRpc(connection); - Parcel reply; reply.markForRpc(connection); if (replyStatus == OK) { + Parcel data; + // transaction->data is owned by this function. Parcel borrows this data and + // only holds onto it for the duration of this function call. Parcel will be + // deleted before the 'transactionData' object. + data.ipcSetDataReference(transaction->data, + transactionData.size() - offsetof(RpcWireTransaction, data), + nullptr /*object*/, 0 /*objectCount*/, + do_nothing_to_transact_data); + data.markForRpc(connection); + if (target) { replyStatus = target->transact(transaction->code, data, &reply, transaction->flags); } else { diff --git a/libs/binder/include/binder/ProcessState.h b/libs/binder/include/binder/ProcessState.h index 0919648541..b9db5d726c 100644 --- a/libs/binder/include/binder/ProcessState.h +++ b/libs/binder/include/binder/ProcessState.h @@ -58,6 +58,7 @@ public: void spawnPooledThread(bool isMain); status_t setThreadPoolMaxThreadCount(size_t maxThreads); + status_t enableOnewaySpamDetection(bool enable); void giveThreadPoolName(); String8 getDriverName(); diff --git a/libs/binder/include/binder/RpcConnection.h b/libs/binder/include/binder/RpcConnection.h index dba47b4d15..2395e78de2 100644 --- a/libs/binder/include/binder/RpcConnection.h +++ b/libs/binder/include/binder/RpcConnection.h @@ -74,6 +74,16 @@ public: #endif // __BIONIC__ /** + * Creates an RPC server at the current port. + */ + [[nodiscard]] bool setupInetServer(unsigned int port); + + /** + * Connects to an RPC server at the given address and port. + */ + [[nodiscard]] bool addInetClient(const char* addr, unsigned int port); + + /** * For debugging! * * Sets up an empty socket. All queries to this socket which require a @@ -118,11 +128,6 @@ private: friend sp<RpcConnection>; RpcConnection(); - bool setupSocketServer(const SocketAddress& address); - bool addSocketClient(const SocketAddress& address); - void addClient(base::unique_fd&& fd); - void assignServerToThisThread(base::unique_fd&& fd); - struct ConnectionSocket : public RefBase { base::unique_fd fd; @@ -131,11 +136,16 @@ private: std::optional<pid_t> exclusiveTid; }; + bool setupSocketServer(const SocketAddress& address); + bool addSocketClient(const SocketAddress& address); + void addClient(base::unique_fd&& fd); + sp<ConnectionSocket> assignServerToThisThread(base::unique_fd&& fd); + bool removeServerSocket(const sp<ConnectionSocket>& socket); + enum class SocketUse { CLIENT, CLIENT_ASYNC, CLIENT_REFCOUNT, - SERVER, }; // RAII object for connection socket diff --git a/libs/binder/include/binder/Status.h b/libs/binder/include/binder/Status.h index c30ae01d60..aaafa36d40 100644 --- a/libs/binder/include/binder/Status.h +++ b/libs/binder/include/binder/Status.h @@ -91,6 +91,9 @@ public: static Status fromExceptionCode(int32_t exceptionCode, const char* message); + // warning: this is still considered an error if it is constructed with a + // zero value error code. Please use Status::ok() instead and avoid zero + // error codes static Status fromServiceSpecificError(int32_t serviceSpecificErrorCode); static Status fromServiceSpecificError(int32_t serviceSpecificErrorCode, const String8& message); diff --git a/libs/binder/include/private/binder/binder_module.h b/libs/binder/include/private/binder/binder_module.h index 157919903f..151235c16b 100644 --- a/libs/binder/include/private/binder/binder_module.h +++ b/libs/binder/include/private/binder/binder_module.h @@ -32,10 +32,6 @@ #include <sys/ioctl.h> #include <linux/android/binder.h> -#ifdef __cplusplus -namespace android { -#endif - #ifndef BR_FROZEN_REPLY // Temporary definition of BR_FROZEN_REPLY. For production // this will come from UAPI binder.h @@ -88,8 +84,18 @@ struct binder_frozen_status_info { }; #endif //BINDER_GET_FROZEN_INFO -#ifdef __cplusplus -} // namespace android -#endif +#ifndef BR_ONEWAY_SPAM_SUSPECT +// Temporary definition of BR_ONEWAY_SPAM_SUSPECT. For production +// this will come from UAPI binder.h +#define BR_ONEWAY_SPAM_SUSPECT _IO('r', 19) +#endif //BR_ONEWAY_SPAM_SUSPECT + +#ifndef BINDER_ENABLE_ONEWAY_SPAM_DETECTION +/* + * Temporary definitions for oneway spam detection support. For the final version + * these will be defined in the UAPI binder.h file from upstream kernel. + */ +#define BINDER_ENABLE_ONEWAY_SPAM_DETECTION _IOW('b', 16, __u32) +#endif //BINDER_ENABLE_ONEWAY_SPAM_DETECTION #endif // _BINDER_MODULE_H_ diff --git a/libs/binder/ndk/include_ndk/android/binder_ibinder.h b/libs/binder/ndk/include_ndk/android/binder_ibinder.h index b9adc9a025..9e2050b411 100644 --- a/libs/binder/ndk/include_ndk/android/binder_ibinder.h +++ b/libs/binder/ndk/include_ndk/android/binder_ibinder.h @@ -173,7 +173,7 @@ typedef binder_status_t (*AIBinder_Class_onTransact)(AIBinder* binder, transacti * Available since API level 29. * * \param interfaceDescriptor this is a unique identifier for the class. This is used internally for - * sanity checks on transactions. + * validity checks on transactions. This should be utf-8. * \param onCreate see AIBinder_Class_onCreate. * \param onDestroy see AIBinder_Class_onDestroy. * \param onTransact see AIBinder_Class_onTransact. @@ -645,7 +645,9 @@ binder_status_t AIBinder_setExtension(AIBinder* binder, AIBinder* ext) __INTRODU * * \return the class descriptor string. This pointer will never be null; a * descriptor is required to define a class. The pointer is owned by the class - * and will remain valid as long as the class does. + * and will remain valid as long as the class does. For a local class, this will + * be the same value (not necessarily pointer equal) as is passed into + * AIBinder_Class_define. Format is utf-8. */ const char* AIBinder_Class_getDescriptor(const AIBinder_Class* clazz) __INTRODUCED_IN(31); @@ -669,7 +671,7 @@ const char* AIBinder_Class_getDescriptor(const AIBinder_Class* clazz) __INTRODUC * * \return whether "lhs < rhs" is true */ -bool AIBinder_lt(const AIBinder* lhs, const AIBinder* rhs); +bool AIBinder_lt(const AIBinder* lhs, const AIBinder* rhs) __INTRODUCED_IN(31); /** * Clone an AIBinder_Weak. Useful because even if a weak binder promotes to a @@ -683,7 +685,7 @@ bool AIBinder_lt(const AIBinder* lhs, const AIBinder* rhs); * \return clone of the input parameter. This must be deleted with * AIBinder_Weak_delete. Null if weak input parameter is also null. */ -AIBinder_Weak* AIBinder_Weak_clone(const AIBinder_Weak* weak); +AIBinder_Weak* AIBinder_Weak_clone(const AIBinder_Weak* weak) __INTRODUCED_IN(31); /** * Whether AIBinder_Weak is less than another. @@ -718,7 +720,7 @@ AIBinder_Weak* AIBinder_Weak_clone(const AIBinder_Weak* weak); * * \return whether "lhs < rhs" is true */ -bool AIBinder_Weak_lt(const AIBinder_Weak* lhs, const AIBinder_Weak* rhs); +bool AIBinder_Weak_lt(const AIBinder_Weak* lhs, const AIBinder_Weak* rhs) __INTRODUCED_IN(31); __END_DECLS diff --git a/libs/binder/rust/Android.bp b/libs/binder/rust/Android.bp index e12a429cf9..57c9013f60 100644 --- a/libs/binder/rust/Android.bp +++ b/libs/binder/rust/Android.bp @@ -65,15 +65,15 @@ rust_bindgen { // rustified "--constified-enum", "android::c_interface::consts::.*", - "--whitelist-type", "android::c_interface::.*", - "--whitelist-type", "AStatus", - "--whitelist-type", "AIBinder_Class", - "--whitelist-type", "AIBinder", - "--whitelist-type", "AIBinder_Weak", - "--whitelist-type", "AIBinder_DeathRecipient", - "--whitelist-type", "AParcel", - "--whitelist-type", "binder_status_t", - "--whitelist-function", ".*", + "--allowlist-type", "android::c_interface::.*", + "--allowlist-type", "AStatus", + "--allowlist-type", "AIBinder_Class", + "--allowlist-type", "AIBinder", + "--allowlist-type", "AIBinder_Weak", + "--allowlist-type", "AIBinder_DeathRecipient", + "--allowlist-type", "AParcel", + "--allowlist-type", "binder_status_t", + "--allowlist-function", ".*", ], shared_libs: [ "libbinder_ndk", diff --git a/libs/binder/rust/tests/Android.bp b/libs/binder/rust/tests/Android.bp index 0bf76c696a..607860f462 100644 --- a/libs/binder/rust/tests/Android.bp +++ b/libs/binder/rust/tests/Android.bp @@ -114,8 +114,8 @@ rust_bindgen { source_stem: "bindings", cpp_std: "gnu++17", bindgen_flags: [ - "--whitelist-type", "Transaction", - "--whitelist-var", "TESTDATA_.*", + "--allowlist-type", "Transaction", + "--allowlist-var", "TESTDATA_.*", ], shared_libs: [ diff --git a/libs/binder/tests/Android.bp b/libs/binder/tests/Android.bp index e39408c0bf..f303b7c1e3 100644 --- a/libs/binder/tests/Android.bp +++ b/libs/binder/tests/Android.bp @@ -155,6 +155,11 @@ cc_benchmark { name: "binderRpcBenchmark", defaults: ["binder_test_defaults"], host_supported: true, + target: { + darwin: { + enabled: false, + }, + }, srcs: [ "binderRpcBenchmark.cpp", "IBinderRpcBenchmark.aidl", diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp index e2193fadab..dc8c0f157e 100644 --- a/libs/binder/tests/binderLibTest.cpp +++ b/libs/binder/tests/binderLibTest.cpp @@ -88,6 +88,7 @@ enum BinderLibTestTranscationCode { BINDER_LIB_TEST_GETPID, BINDER_LIB_TEST_ECHO_VECTOR, BINDER_LIB_TEST_REJECT_BUF, + BINDER_LIB_TEST_CAN_GET_SID, }; pid_t start_server_process(int arg2, bool usePoll = false) @@ -1192,6 +1193,14 @@ TEST_F(BinderLibTest, BufRejected) { EXPECT_NE(NO_ERROR, ret); } +TEST_F(BinderLibTest, GotSid) { + sp<IBinder> server = addServer(); + + Parcel data; + status_t ret = server->transact(BINDER_LIB_TEST_CAN_GET_SID, data, nullptr); + EXPECT_EQ(OK, ret); +} + class BinderLibTestService : public BBinder { public: @@ -1494,6 +1503,9 @@ class BinderLibTestService : public BBinder case BINDER_LIB_TEST_REJECT_BUF: { return data.objectsCount() == 0 ? BAD_VALUE : NO_ERROR; } + case BINDER_LIB_TEST_CAN_GET_SID: { + return IPCThreadState::self()->getCallingSid() == nullptr ? BAD_VALUE : NO_ERROR; + } default: return UNKNOWN_TRANSACTION; }; diff --git a/libs/binder/tests/binderRpcTest.cpp b/libs/binder/tests/binderRpcTest.cpp index 33402937ab..dd68fdbc6d 100644 --- a/libs/binder/tests/binderRpcTest.cpp +++ b/libs/binder/tests/binderRpcTest.cpp @@ -80,11 +80,10 @@ public: sp<RpcConnection> connection; Status sendString(const std::string& str) override { - std::cout << "Child received string: " << str << std::endl; + (void)str; return Status::ok(); } Status doubleString(const std::string& str, std::string* strstr) override { - std::cout << "Child received string to double: " << str << std::endl; *strstr = str + str; return Status::ok(); } @@ -257,6 +256,7 @@ enum class SocketType { #ifdef __BIONIC__ VSOCK, #endif // __BIONIC__ + INET, }; static inline std::string PrintSocketType(const testing::TestParamInfo<SocketType>& info) { switch (info.param) { @@ -266,6 +266,8 @@ static inline std::string PrintSocketType(const testing::TestParamInfo<SocketTyp case SocketType::VSOCK: return "vm_socket"; #endif // __BIONIC__ + case SocketType::INET: + return "inet_socket"; default: LOG_ALWAYS_FATAL("Unknown socket type"); return ""; @@ -305,6 +307,9 @@ public: CHECK(connection->setupVsockServer(port)); break; #endif // __BIONIC__ + case SocketType::INET: + CHECK(connection->setupInetServer(port)); + break; default: LOG_ALWAYS_FATAL("Unknown socket type"); } @@ -335,6 +340,9 @@ public: if (ret.connection->addVsockClient(VMADDR_CID_LOCAL, port)) goto success; break; #endif // __BIONIC__ + case SocketType::INET: + if (ret.connection->addInetClient("127.0.0.1", port)) goto success; + break; default: LOG_ALWAYS_FATAL("Unknown socket type"); } @@ -740,7 +748,7 @@ TEST_P(BinderRpc, ThreadingStressTest) { threads.push_back(std::thread([&] { for (size_t j = 0; j < kNumCalls; j++) { sp<IBinder> out; - proc.rootIface->repeatBinder(proc.rootBinder, &out); + EXPECT_OK(proc.rootIface->repeatBinder(proc.rootBinder, &out)); EXPECT_EQ(proc.rootBinder, out); } })); @@ -749,6 +757,28 @@ TEST_P(BinderRpc, ThreadingStressTest) { for (auto& t : threads) t.join(); } +TEST_P(BinderRpc, OnewayStressTest) { + constexpr size_t kNumClientThreads = 10; + constexpr size_t kNumServerThreads = 10; + constexpr size_t kNumCalls = 100; + + auto proc = createRpcTestSocketServerProcess(kNumServerThreads); + + std::vector<std::thread> threads; + for (size_t i = 0; i < kNumClientThreads; i++) { + threads.push_back(std::thread([&] { + for (size_t j = 0; j < kNumCalls; j++) { + EXPECT_OK(proc.rootIface->sendString("a")); + } + + // check threads are not stuck + EXPECT_OK(proc.rootIface->sleepMs(250)); + })); + } + + for (auto& t : threads) t.join(); +} + TEST_P(BinderRpc, OnewayCallDoesNotWait) { constexpr size_t kReallyLongTimeMs = 100; constexpr size_t kSleepMs = kReallyLongTimeMs * 5; @@ -852,12 +882,13 @@ TEST_P(BinderRpc, Fds) { } INSTANTIATE_TEST_CASE_P(PerSocket, BinderRpc, - ::testing::Values(SocketType::UNIX + ::testing::ValuesIn({ + SocketType::UNIX, #ifdef __BIONIC__ - , - SocketType::VSOCK + SocketType::VSOCK, #endif // __BIONIC__ - ), + SocketType::INET, + }), PrintSocketType); } // namespace android diff --git a/libs/permission/Android.bp b/libs/permission/Android.bp new file mode 100644 index 0000000000..a5712b319f --- /dev/null +++ b/libs/permission/Android.bp @@ -0,0 +1,23 @@ +package { + // See: http://go/android-license-faq + // A large-scale-change added 'default_applicable_licenses' to import + // all of the 'license_kinds' from "frameworks_native_license" + // to get the below license kinds: + // SPDX-license-identifier-Apache-2.0 + default_applicable_licenses: ["frameworks_native_license"], +} + +cc_library_shared { + name: "libpermission", + srcs: [ + "AppOpsManager.cpp", + "IAppOpsCallback.cpp", + "IAppOpsService.cpp", + ], + export_include_dirs: ["include"], + shared_libs: [ + "libbinder", + "liblog", + "libutils", + ], +} diff --git a/libs/binder/AppOpsManager.cpp b/libs/permission/AppOpsManager.cpp index f3ea1a71d0..f3ea1a71d0 100644 --- a/libs/binder/AppOpsManager.cpp +++ b/libs/permission/AppOpsManager.cpp diff --git a/libs/binder/IAppOpsCallback.cpp b/libs/permission/IAppOpsCallback.cpp index 2b3f462ab8..2b3f462ab8 100644 --- a/libs/binder/IAppOpsCallback.cpp +++ b/libs/permission/IAppOpsCallback.cpp diff --git a/libs/binder/IAppOpsService.cpp b/libs/permission/IAppOpsService.cpp index 1af5ab8719..1af5ab8719 100644 --- a/libs/binder/IAppOpsService.cpp +++ b/libs/permission/IAppOpsService.cpp diff --git a/libs/binder/include/binder/AppOpsManager.h b/libs/permission/include/binder/AppOpsManager.h index 35c697e3d2..35c697e3d2 100644 --- a/libs/binder/include/binder/AppOpsManager.h +++ b/libs/permission/include/binder/AppOpsManager.h diff --git a/libs/binder/include/binder/IAppOpsCallback.h b/libs/permission/include/binder/IAppOpsCallback.h index eb76f57bf8..eb76f57bf8 100644 --- a/libs/binder/include/binder/IAppOpsCallback.h +++ b/libs/permission/include/binder/IAppOpsCallback.h diff --git a/libs/binder/include/binder/IAppOpsService.h b/libs/permission/include/binder/IAppOpsService.h index b0719d4ebc..b0719d4ebc 100644 --- a/libs/binder/include/binder/IAppOpsService.h +++ b/libs/permission/include/binder/IAppOpsService.h diff --git a/libs/sensor/Android.bp b/libs/sensor/Android.bp index 497c33c386..edd453a936 100644 --- a/libs/sensor/Android.bp +++ b/libs/sensor/Android.bp @@ -48,11 +48,10 @@ cc_library_shared { "libutils", "liblog", "libhardware", + "libpermission", ], export_include_dirs: ["include"], - export_shared_lib_headers: ["libbinder", "libhardware"], + export_shared_lib_headers: ["libbinder", "libpermission", "libhardware"], } - -subdirs = ["tests"] diff --git a/services/sensorservice/Android.bp b/services/sensorservice/Android.bp index 9aecaff409..4151b4512f 100644 --- a/services/sensorservice/Android.bp +++ b/services/sensorservice/Android.bp @@ -54,6 +54,7 @@ cc_library_shared { "libbinder", "libsensor", "libsensorprivacy", + "libpermission", "libprotoutil", "libcrypto", "libbase", @@ -74,6 +75,7 @@ cc_library_shared { "libactivitymanager_aidl", "libsensor", "libsensorprivacy", + "libpermission", ], } |