diff options
115 files changed, 1863 insertions, 1055 deletions
diff --git a/cmds/atrace/atrace.rc b/cmds/atrace/atrace.rc index c3cf2c2a60..fc0801cce8 100644 --- a/cmds/atrace/atrace.rc +++ b/cmds/atrace/atrace.rc @@ -228,10 +228,6 @@ on late-init chmod 0666 /sys/kernel/debug/tracing/events/thermal/cdev_update/enable chmod 0666 /sys/kernel/tracing/events/thermal/cdev_update/enable -# Tracing disabled by default - write /sys/kernel/debug/tracing/tracing_on 0 - write /sys/kernel/tracing/tracing_on 0 - # Read and truncate the kernel trace. chmod 0666 /sys/kernel/debug/tracing/trace chmod 0666 /sys/kernel/tracing/trace @@ -310,11 +306,9 @@ on late-init chmod 0666 /sys/kernel/tracing/events/synthetic/suspend_resume_minimal/enable chmod 0666 /sys/kernel/debug/tracing/events/synthetic/suspend_resume_minimal/enable -on late-init && property:ro.boot.fastboot.boottrace=enabled - setprop debug.atrace.tags.enableflags 802922 - setprop persist.traced.enable 0 - write /sys/kernel/debug/tracing/tracing_on 1 - write /sys/kernel/tracing/tracing_on 1 +on late-init && property:ro.boot.fastboot.boottrace= + write /sys/kernel/debug/tracing/tracing_on 0 + write /sys/kernel/tracing/tracing_on 0 # Only create the tracing instance if persist.mm_events.enabled # Attempting to remove the tracing instance after it has been created @@ -527,7 +521,6 @@ on late-init && property:ro.boot.hypervisor.vm.supported=1 chmod 0440 /sys/kernel/debug/tracing/hyp/events/hyp/host_mem_abort/id chmod 0440 /sys/kernel/tracing/hyp/events/hyp/host_mem_abort/id - on property:persist.debug.atrace.boottrace=1 start boottrace @@ -536,10 +529,3 @@ service boottrace /system/bin/atrace --async_start -f /data/misc/boottrace/categ user root disabled oneshot - -on property:sys.boot_completed=1 && property:ro.boot.fastboot.boottrace=enabled - setprop debug.atrace.tags.enableflags 0 - setprop persist.traced.enable 1 - write /sys/kernel/debug/tracing/tracing_on 0 - write /sys/kernel/tracing/tracing_on 0 - diff --git a/cmds/dumpstate/DumpstateUtil.cpp b/cmds/dumpstate/DumpstateUtil.cpp index aa42541a66..615701ccc1 100644 --- a/cmds/dumpstate/DumpstateUtil.cpp +++ b/cmds/dumpstate/DumpstateUtil.cpp @@ -207,6 +207,7 @@ std::string PropertiesHelper::build_type_ = ""; int PropertiesHelper::dry_run_ = -1; int PropertiesHelper::unroot_ = -1; int PropertiesHelper::parallel_run_ = -1; +int PropertiesHelper::strict_run_ = -1; bool PropertiesHelper::IsUserBuild() { if (build_type_.empty()) { @@ -237,6 +238,14 @@ bool PropertiesHelper::IsParallelRun() { return parallel_run_ == 1; } +bool PropertiesHelper::IsStrictRun() { + if (strict_run_ == -1) { + // Defaults to using stricter timeouts. + strict_run_ = android::base::GetBoolProperty("dumpstate.strict_run", true) ? 1 : 0; + } + return strict_run_ == 1; +} + int DumpFileToFd(int out_fd, const std::string& title, const std::string& path) { android::base::unique_fd fd(TEMP_FAILURE_RETRY(open(path.c_str(), O_RDONLY | O_NONBLOCK | O_CLOEXEC))); if (fd.get() < 0) { diff --git a/cmds/dumpstate/DumpstateUtil.h b/cmds/dumpstate/DumpstateUtil.h index b00c46e0db..9e955e3c04 100644 --- a/cmds/dumpstate/DumpstateUtil.h +++ b/cmds/dumpstate/DumpstateUtil.h @@ -193,11 +193,19 @@ class PropertiesHelper { */ static bool IsParallelRun(); + /* + * Strict-run mode is determined by the `dumpstate.strict_run` sysprop which + * will default to true. This results in shortened timeouts for flaky + * sections. + */ + static bool IsStrictRun(); + private: static std::string build_type_; static int dry_run_; static int unroot_; static int parallel_run_; + static int strict_run_; }; /* diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index dc0e26b9a3..9444729a6c 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -820,9 +820,12 @@ void Dumpstate::PrintHeader() const { RunCommandToFd(STDOUT_FILENO, "", {"uptime", "-p"}, CommandOptions::WithTimeout(1).Always().Build()); printf("Bugreport format version: %s\n", version_.c_str()); - printf("Dumpstate info: id=%d pid=%d dry_run=%d parallel_run=%d args=%s bugreport_mode=%s\n", - id_, pid_, PropertiesHelper::IsDryRun(), PropertiesHelper::IsParallelRun(), - options_->args.c_str(), options_->bugreport_mode_string.c_str()); + printf( + "Dumpstate info: id=%d pid=%d dry_run=%d parallel_run=%d strict_run=%d args=%s " + "bugreport_mode=%s\n", + id_, pid_, PropertiesHelper::IsDryRun(), PropertiesHelper::IsParallelRun(), + PropertiesHelper::IsStrictRun(), options_->args.c_str(), + options_->bugreport_mode_string.c_str()); printf("\n"); } @@ -1044,7 +1047,8 @@ static void DumpIncidentReport() { MYLOGE("Could not open %s to dump incident report.\n", path.c_str()); return; } - RunCommandToFd(fd, "", {"incident", "-u"}, CommandOptions::WithTimeout(20).Build()); + RunCommandToFd(fd, "", {"incident", "-u"}, + CommandOptions::WithTimeout(PropertiesHelper::IsStrictRun() ? 20 : 120).Build()); bool empty = 0 == lseek(fd, 0, SEEK_END); if (!empty) { // Use a different name from "incident.proto" @@ -1414,12 +1418,12 @@ static void DumpHals(int out_fd = STDOUT_FILENO) { auto ret = sm->list([&](const auto& interfaces) { for (const std::string& interface : interfaces) { std::string cleanName = interface; - std::replace_if(cleanName.begin(), - cleanName.end(), - [](char c) { - return !isalnum(c) && - std::string("@-_:.").find(c) == std::string::npos; - }, '_'); + std::replace_if( + cleanName.begin(), cleanName.end(), + [](char c) { + return !isalnum(c) && std::string("@-_.").find(c) == std::string::npos; + }, + '_'); const std::string path = ds.bugreport_internal_dir_ + "/lshal_debug_" + cleanName; bool empty = false; @@ -1750,6 +1754,20 @@ static Dumpstate::RunStatus dumpstate() { RunCommand("SYSTEM PROPERTIES", {"getprop"}); + DumpFile("SYSTEM BUILD-TIME RELEASE FLAGS", "/system/etc/build_flags.json"); + DumpFile("SYSTEM_EXT BUILD-TIME RELEASE FLAGS", "/system_ext/etc/build_flags.json"); + DumpFile("PRODUCT BUILD-TIME RELEASE FLAGS", "/product/etc/build_flags.json"); + DumpFile("VENDOR BUILD-TIME RELEASE FLAGS", "/vendor/etc/build_flags.json"); + + DumpFile("SYSTEM BUILD-TIME ACONFIG FLAGS (check dumpstate build_config for runtime values)", + "/system/etc/aconfig_flags.textproto"); + DumpFile("SYSTEM_EXT BUILD-TIME ACONFIG FLAGS (check dumpstate build_config for runtime" + " values)", "/system_ext/etc/aconfig_flags.textproto"); + DumpFile("PRODUCT BUILD-TIME ACONFIG FLAGS (check dumpstate build_config for runtime values)", + "/product/etc/aconfig_flags.textproto"); + DumpFile("VENDOR BUILD-TIME ACONFIG FLAGS (check dumpstate build_config for runtime values)", + "/vendor/etc/aconfig_flags.textproto"); + RunCommand("STORAGED IO INFO", {"storaged", "-u", "-p"}); RunCommand("FILESYSTEMS & FREE SPACE", {"df"}); @@ -3053,6 +3071,12 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, MYLOGI("Running on dry-run mode (to disable it, call 'setprop dumpstate.dry_run false')\n"); } + if (PropertiesHelper::IsStrictRun()) { + MYLOGI( + "Running on strict-run mode, which has shorter timeouts " + "(to disable, call 'setprop dumpstate.strict_run false')\n"); + } + MYLOGI("dumpstate info: id=%d, args='%s', bugreport_mode= %s bugreport format version: %s\n", id_, options_->args.c_str(), options_->bugreport_mode_string.c_str(), version_.c_str()); diff --git a/cmds/installd/CrateManager.cpp b/cmds/installd/CrateManager.cpp index b17cba15d5..fd1df35aee 100644 --- a/cmds/installd/CrateManager.cpp +++ b/cmds/installd/CrateManager.cpp @@ -29,9 +29,10 @@ #include <sys/xattr.h> #include <unistd.h> +#include <utils.h> #include <fstream> +#include <functional> #include <string> -#include <utils.h> #include "utils.h" diff --git a/cmds/installd/CrateManager.h b/cmds/installd/CrateManager.h index 1f30b5dc79..d9b590f784 100644 --- a/cmds/installd/CrateManager.h +++ b/cmds/installd/CrateManager.h @@ -25,6 +25,7 @@ #include <sys/stat.h> #include <sys/types.h> +#include <functional> #include <optional> #include <string> #include <vector> diff --git a/cmds/installd/InstalldNativeService.cpp b/cmds/installd/InstalldNativeService.cpp index c6132e8ceb..b302f52f3e 100644 --- a/cmds/installd/InstalldNativeService.cpp +++ b/cmds/installd/InstalldNativeService.cpp @@ -236,6 +236,16 @@ binder::Status checkArgumentFileName(const std::string& path) { } \ } +// we could have tighter checks, but this is only to avoid hard errors. Negative values are defined +// in UserHandle.java and carry specific meanings that may not be handled by certain APIs here. +#define ENFORCE_VALID_USER(userId) \ + { \ + if (static_cast<uid_t>(std::abs(userId)) >= \ + std::numeric_limits<uid_t>::max() / AID_USER_OFFSET) { \ + return error("userId invalid: " + std::to_string(userId)); \ + } \ + } + #define CHECK_ARGUMENT_UUID(uuid) { \ binder::Status status = checkArgumentUuid((uuid)); \ if (!status.isOk()) { \ @@ -696,6 +706,7 @@ binder::Status InstalldNativeService::createAppDataLocked( int32_t flags, int32_t appId, int32_t previousAppId, const std::string& seInfo, int32_t targetSdkVersion, int64_t* _aidl_return) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); CHECK_ARGUMENT_PACKAGE_NAME(packageName); @@ -790,6 +801,8 @@ binder::Status InstalldNativeService::createAppDataLocked( binder::Status InstalldNativeService::createSdkSandboxDataPackageDirectory( const std::optional<std::string>& uuid, const std::string& packageName, int32_t userId, int32_t appId, int32_t flags) { + ENFORCE_VALID_USER(userId); + int32_t sdkSandboxUid = multiuser_get_sdk_sandbox_uid(userId, appId); if (sdkSandboxUid == -1) { // There no valid sdk sandbox process for this app. Skip creation of data directory @@ -828,6 +841,7 @@ binder::Status InstalldNativeService::createAppData( int32_t flags, int32_t appId, int32_t previousAppId, const std::string& seInfo, int32_t targetSdkVersion, int64_t* _aidl_return) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); CHECK_ARGUMENT_PACKAGE_NAME(packageName); LOCK_PACKAGE_USER(); @@ -839,6 +853,7 @@ binder::Status InstalldNativeService::createAppData( const android::os::CreateAppDataArgs& args, android::os::CreateAppDataResult* _aidl_return) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(args.userId); // Locking is performed depeer in the callstack. int64_t ceDataInode = -1; @@ -854,6 +869,10 @@ binder::Status InstalldNativeService::createAppDataBatched( const std::vector<android::os::CreateAppDataArgs>& args, std::vector<android::os::CreateAppDataResult>* _aidl_return) { ENFORCE_UID(AID_SYSTEM); + for (const auto& arg : args) { + ENFORCE_VALID_USER(arg.userId); + } + // Locking is performed depeer in the callstack. std::vector<android::os::CreateAppDataResult> results; @@ -868,6 +887,7 @@ binder::Status InstalldNativeService::createAppDataBatched( binder::Status InstalldNativeService::reconcileSdkData( const android::os::ReconcileSdkDataArgs& args) { + ENFORCE_VALID_USER(args.userId); // Locking is performed depeer in the callstack. return reconcileSdkData(args.uuid, args.packageName, args.subDirNames, args.userId, args.appId, @@ -891,6 +911,7 @@ binder::Status InstalldNativeService::reconcileSdkData(const std::optional<std:: int userId, int appId, int previousAppId, const std::string& seInfo, int flags) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); CHECK_ARGUMENT_PACKAGE_NAME(packageName); LOCK_PACKAGE_USER(); @@ -974,6 +995,7 @@ binder::Status InstalldNativeService::reconcileSdkData(const std::optional<std:: binder::Status InstalldNativeService::migrateAppData(const std::optional<std::string>& uuid, const std::string& packageName, int32_t userId, int32_t flags) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); CHECK_ARGUMENT_PACKAGE_NAME(packageName); LOCK_PACKAGE_USER(); @@ -1041,6 +1063,7 @@ binder::Status InstalldNativeService::clearAppProfiles(const std::string& packag binder::Status InstalldNativeService::clearAppData(const std::optional<std::string>& uuid, const std::string& packageName, int32_t userId, int32_t flags, int64_t ceDataInode) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); CHECK_ARGUMENT_PACKAGE_NAME(packageName); LOCK_PACKAGE_USER(); @@ -1132,6 +1155,7 @@ binder::Status InstalldNativeService::clearAppData(const std::optional<std::stri binder::Status InstalldNativeService::clearSdkSandboxDataPackageDirectory( const std::optional<std::string>& uuid, const std::string& packageName, int32_t userId, int32_t flags) { + ENFORCE_VALID_USER(userId); const char* uuid_ = uuid ? uuid->c_str() : nullptr; const char* pkgname = packageName.c_str(); @@ -1218,6 +1242,7 @@ binder::Status InstalldNativeService::deleteReferenceProfile(const std::string& binder::Status InstalldNativeService::destroyAppData(const std::optional<std::string>& uuid, const std::string& packageName, int32_t userId, int32_t flags, int64_t ceDataInode) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); CHECK_ARGUMENT_PACKAGE_NAME(packageName); LOCK_PACKAGE_USER(); @@ -1288,6 +1313,8 @@ binder::Status InstalldNativeService::destroyAppData(const std::optional<std::st binder::Status InstalldNativeService::destroySdkSandboxDataPackageDirectory( const std::optional<std::string>& uuid, const std::string& packageName, int32_t userId, int32_t flags) { + ENFORCE_VALID_USER(userId); + const char* uuid_ = uuid ? uuid->c_str() : nullptr; const char* pkgname = packageName.c_str(); @@ -1435,6 +1462,7 @@ binder::Status InstalldNativeService::snapshotAppData(const std::optional<std::s int32_t userId, int32_t snapshotId, int32_t storageFlags, int64_t* _aidl_return) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID_IS_TEST_OR_NULL(volumeUuid); CHECK_ARGUMENT_PACKAGE_NAME(packageName); LOCK_PACKAGE_USER(); @@ -1569,6 +1597,7 @@ binder::Status InstalldNativeService::restoreAppDataSnapshot( const int32_t appId, const std::string& seInfo, const int32_t userId, const int32_t snapshotId, int32_t storageFlags) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID_IS_TEST_OR_NULL(volumeUuid); CHECK_ARGUMENT_PACKAGE_NAME(packageName); LOCK_PACKAGE_USER(); @@ -1641,6 +1670,7 @@ binder::Status InstalldNativeService::destroyAppDataSnapshot( const int32_t userId, const int64_t ceSnapshotInode, const int32_t snapshotId, int32_t storageFlags) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID_IS_TEST_OR_NULL(volumeUuid); CHECK_ARGUMENT_PACKAGE_NAME(packageName); LOCK_PACKAGE_USER(); @@ -1674,6 +1704,7 @@ binder::Status InstalldNativeService::destroyCeSnapshotsNotSpecified( const std::optional<std::string>& volumeUuid, const int32_t userId, const std::vector<int32_t>& retainSnapshotIds) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID_IS_TEST_OR_NULL(volumeUuid); LOCK_USER(); @@ -1864,6 +1895,7 @@ fail: binder::Status InstalldNativeService::createUserData(const std::optional<std::string>& uuid, int32_t userId, int32_t userSerial ATTRIBUTE_UNUSED, int32_t flags) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); LOCK_USER(); @@ -1884,6 +1916,7 @@ binder::Status InstalldNativeService::createUserData(const std::optional<std::st binder::Status InstalldNativeService::destroyUserData(const std::optional<std::string>& uuid, int32_t userId, int32_t flags) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); LOCK_USER(); @@ -2671,6 +2704,7 @@ binder::Status InstalldNativeService::getUserSize(const std::optional<std::strin int32_t userId, int32_t flags, const std::vector<int32_t>& appIds, std::vector<int64_t>* _aidl_return) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); // NOTE: Locking is relaxed on this method, since it's limited to // read-only measurements without mutation. @@ -2806,6 +2840,7 @@ binder::Status InstalldNativeService::getExternalSize(const std::optional<std::s int32_t userId, int32_t flags, const std::vector<int32_t>& appIds, std::vector<int64_t>* _aidl_return) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); // NOTE: Locking is relaxed on this method, since it's limited to // read-only measurements without mutation. @@ -2926,6 +2961,7 @@ binder::Status InstalldNativeService::getAppCrates( const std::vector<std::string>& packageNames, int32_t userId, std::optional<std::vector<std::optional<CrateMetadata>>>* _aidl_return) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); for (const auto& packageName : packageNames) { CHECK_ARGUMENT_PACKAGE_NAME(packageName); @@ -2975,6 +3011,7 @@ binder::Status InstalldNativeService::getUserCrates( const std::optional<std::string>& uuid, int32_t userId, std::optional<std::vector<std::optional<CrateMetadata>>>* _aidl_return) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); #ifdef ENABLE_STORAGE_CRATES LOCK_USER(); @@ -3018,6 +3055,7 @@ binder::Status InstalldNativeService::getUserCrates( binder::Status InstalldNativeService::setAppQuota(const std::optional<std::string>& uuid, int32_t userId, int32_t appId, int64_t cacheQuota) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); std::lock_guard<std::recursive_mutex> lock(mQuotasLock); @@ -3261,6 +3299,7 @@ binder::Status InstalldNativeService::restoreconAppData(const std::optional<std: const std::string& packageName, int32_t userId, int32_t flags, int32_t appId, const std::string& seInfo) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); CHECK_ARGUMENT_PACKAGE_NAME(packageName); LOCK_PACKAGE_USER(); @@ -3271,6 +3310,7 @@ binder::Status InstalldNativeService::restoreconAppDataLocked( const std::optional<std::string>& uuid, const std::string& packageName, int32_t userId, int32_t flags, int32_t appId, const std::string& seInfo) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); CHECK_ARGUMENT_PACKAGE_NAME(packageName); @@ -3302,6 +3342,7 @@ binder::Status InstalldNativeService::restoreconSdkDataLocked( const std::optional<std::string>& uuid, const std::string& packageName, int32_t userId, int32_t flags, int32_t appId, const std::string& seInfo) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_UUID(uuid); CHECK_ARGUMENT_PACKAGE_NAME(packageName); @@ -3753,6 +3794,7 @@ binder::Status InstalldNativeService::prepareAppProfile(const std::string& packa int32_t userId, int32_t appId, const std::string& profileName, const std::string& codePath, const std::optional<std::string>& dexMetadata, bool* _aidl_return) { ENFORCE_UID(AID_SYSTEM); + ENFORCE_VALID_USER(userId); CHECK_ARGUMENT_PACKAGE_NAME(packageName); CHECK_ARGUMENT_PATH(codePath); LOCK_PACKAGE_USER(); @@ -3775,6 +3817,7 @@ binder::Status InstalldNativeService::migrateLegacyObbData() { binder::Status InstalldNativeService::cleanupInvalidPackageDirs( const std::optional<std::string>& uuid, int32_t userId, int32_t flags) { + ENFORCE_VALID_USER(userId); const char* uuid_cstr = uuid ? uuid->c_str() : nullptr; if (flags & FLAG_STORAGE_CE) { diff --git a/cmds/installd/dexopt.h b/cmds/installd/dexopt.h index 5cf402c54c..df02588a22 100644 --- a/cmds/installd/dexopt.h +++ b/cmds/installd/dexopt.h @@ -18,6 +18,7 @@ #define DEXOPT_H_ #include "installd_constants.h" +#include "unique_file.h" #include <sys/types.h> @@ -156,6 +157,10 @@ const char* select_execution_binary( // artifacts. int get_odex_visibility(const char* apk_path, const char* instruction_set, const char* oat_dir); +UniqueFile maybe_open_reference_profile(const std::string& pkgname, const std::string& dex_path, + const char* profile_name, bool profile_guided, + bool is_public, int uid, bool is_secondary_dex); + } // namespace installd } // namespace android diff --git a/cmds/installd/otapreopt.cpp b/cmds/installd/otapreopt.cpp index 7cabdb09e1..818fd8066e 100644 --- a/cmds/installd/otapreopt.cpp +++ b/cmds/installd/otapreopt.cpp @@ -14,20 +14,21 @@ ** limitations under the License. */ -#include <algorithm> #include <inttypes.h> -#include <limits> -#include <random> -#include <regex> #include <selinux/android.h> #include <selinux/avc.h> #include <stdlib.h> #include <string.h> #include <sys/capability.h> +#include <sys/mman.h> #include <sys/prctl.h> #include <sys/stat.h> -#include <sys/mman.h> #include <sys/wait.h> +#include <algorithm> +#include <iterator> +#include <limits> +#include <random> +#include <regex> #include <android-base/logging.h> #include <android-base/macros.h> @@ -47,6 +48,7 @@ #include "otapreopt_parameters.h" #include "otapreopt_utils.h" #include "system_properties.h" +#include "unique_file.h" #include "utils.h" #ifndef LOG_TAG @@ -87,6 +89,9 @@ static_assert(DEXOPT_GENERATE_APP_IMAGE == 1 << 12, "DEXOPT_GENERATE_APP_IMAGE u static_assert(DEXOPT_MASK == (0x3dfe | DEXOPT_IDLE_BACKGROUND_JOB), "DEXOPT_MASK unexpected."); +constexpr const char* kAotCompilerFilters[]{ + "space-profile", "space", "speed-profile", "speed", "everything-profile", "everything", +}; template<typename T> static constexpr bool IsPowerOfTwo(T x) { @@ -415,6 +420,32 @@ private: return (strcmp(arg, "!") == 0) ? nullptr : arg; } + bool IsAotCompilation() const { + if (std::find(std::begin(kAotCompilerFilters), std::end(kAotCompilerFilters), + parameters_.compiler_filter) == std::end(kAotCompilerFilters)) { + return false; + } + + int dexopt_flags = parameters_.dexopt_flags; + bool profile_guided = (dexopt_flags & DEXOPT_PROFILE_GUIDED) != 0; + bool is_secondary_dex = (dexopt_flags & DEXOPT_SECONDARY_DEX) != 0; + bool is_public = (dexopt_flags & DEXOPT_PUBLIC) != 0; + + if (profile_guided) { + UniqueFile reference_profile = + maybe_open_reference_profile(parameters_.pkgName, parameters_.apk_path, + parameters_.profile_name, profile_guided, + is_public, parameters_.uid, is_secondary_dex); + struct stat sbuf; + if (reference_profile.fd() == -1 || + (fstat(reference_profile.fd(), &sbuf) != -1 && sbuf.st_size == 0)) { + return false; + } + } + + return true; + } + bool ShouldSkipPreopt() const { // There's one thing we have to be careful about: we may/will be asked to compile an app // living in the system image. This may be a valid request - if the app wasn't compiled, @@ -439,9 +470,12 @@ private: // (This is ugly as it's the only thing where we need to understand the contents // of parameters_, but it beats postponing the decision or using the call- // backs to do weird things.) + + // In addition, no need to preopt for "verify". The existing vdex files in the OTA package + // and the /data partition will still be usable after the OTA update is applied. const char* apk_path = parameters_.apk_path; CHECK(apk_path != nullptr); - if (StartsWith(apk_path, android_root_)) { + if (StartsWith(apk_path, android_root_) || !IsAotCompilation()) { const char* last_slash = strrchr(apk_path, '/'); if (last_slash != nullptr) { std::string path(apk_path, last_slash - apk_path + 1); @@ -471,13 +505,18 @@ private: // TODO(calin): embed the profile name in the parameters. int Dexopt() { std::string error; + + int dexopt_flags = parameters_.dexopt_flags; + // Make sure dex2oat is run with background priority. + dexopt_flags |= DEXOPT_BOOTCOMPLETE | DEXOPT_IDLE_BACKGROUND_JOB; + int res = dexopt(parameters_.apk_path, parameters_.uid, parameters_.pkgName, parameters_.instruction_set, parameters_.dexopt_needed, parameters_.oat_dir, - parameters_.dexopt_flags, + dexopt_flags, parameters_.compiler_filter, parameters_.volume_uuid, parameters_.shared_libraries, diff --git a/cmds/installd/otapreopt_chroot.cpp b/cmds/installd/otapreopt_chroot.cpp index c86993cb06..c40caf56d8 100644 --- a/cmds/installd/otapreopt_chroot.cpp +++ b/cmds/installd/otapreopt_chroot.cpp @@ -19,9 +19,12 @@ #include <sys/mount.h> #include <sys/stat.h> #include <sys/wait.h> +#include <unistd.h> +#include <algorithm> #include <array> #include <fstream> +#include <iostream> #include <sstream> #include <android-base/file.h> @@ -29,6 +32,7 @@ #include <android-base/macros.h> #include <android-base/scopeguard.h> #include <android-base/stringprintf.h> +#include <android-base/strings.h> #include <android-base/unique_fd.h> #include <libdm/dm.h> #include <selinux/android.h> @@ -37,7 +41,7 @@ #include "otapreopt_utils.h" #ifndef LOG_TAG -#define LOG_TAG "otapreopt" +#define LOG_TAG "otapreopt_chroot" #endif using android::base::StringPrintf; @@ -49,20 +53,22 @@ namespace installd { // so just try the possibilities one by one. static constexpr std::array kTryMountFsTypes = {"ext4", "erofs"}; -static void CloseDescriptor(int fd) { - if (fd >= 0) { - int result = close(fd); - UNUSED(result); // Ignore result. Printing to logcat will open a new descriptor - // that we do *not* want. - } -} - static void CloseDescriptor(const char* descriptor_string) { int fd = -1; std::istringstream stream(descriptor_string); stream >> fd; if (!stream.fail()) { - CloseDescriptor(fd); + if (fd >= 0) { + if (close(fd) < 0) { + PLOG(ERROR) << "Failed to close " << fd; + } + } + } +} + +static void SetCloseOnExec(int fd) { + if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) { + PLOG(ERROR) << "Failed to set FD_CLOEXEC on " << fd; } } @@ -129,24 +135,39 @@ static void TryExtraMount(const char* name, const char* slot, const char* target } // Entry for otapreopt_chroot. Expected parameters are: -// [cmd] [status-fd] [target-slot] "dexopt" [dexopt-params] -// The file descriptor denoted by status-fd will be closed. The rest of the parameters will -// be passed on to otapreopt in the chroot. +// +// [cmd] [status-fd] [target-slot-suffix] +// +// The file descriptor denoted by status-fd will be closed. Dexopt commands on +// the form +// +// "dexopt" [dexopt-params] +// +// are then read from stdin until EOF and passed on to /system/bin/otapreopt one +// by one. After each call a line with the current command count is written to +// stdout and flushed. static int otapreopt_chroot(const int argc, char **arg) { // Validate arguments - // We need the command, status channel and target slot, at a minimum. - if(argc < 3) { - PLOG(ERROR) << "Not enough arguments."; + if (argc == 2 && std::string_view(arg[1]) == "--version") { + // Accept a single --version flag, to allow the script to tell this binary + // from the earlier one. + std::cout << "2" << std::endl; + return 0; + } + if (argc != 3) { + LOG(ERROR) << "Wrong number of arguments: " << argc; exit(208); } - // Close all file descriptors. They are coming from the caller, we do not want to pass them - // on across our fork/exec into a different domain. - // 1) Default descriptors. - CloseDescriptor(STDIN_FILENO); - CloseDescriptor(STDOUT_FILENO); - CloseDescriptor(STDERR_FILENO); - // 2) The status channel. - CloseDescriptor(arg[1]); + const char* status_fd = arg[1]; + const char* slot_suffix = arg[2]; + + // Set O_CLOEXEC on standard fds. They are coming from the caller, we do not + // want to pass them on across our fork/exec into a different domain. + SetCloseOnExec(STDIN_FILENO); + SetCloseOnExec(STDOUT_FILENO); + SetCloseOnExec(STDERR_FILENO); + // Close the status channel. + CloseDescriptor(status_fd); // We need to run the otapreopt tool from the postinstall partition. As such, set up a // mount namespace and change root. @@ -185,20 +206,20 @@ static int otapreopt_chroot(const int argc, char **arg) { // 2) We're in a mount namespace here, so when we die, this will be cleaned up. // 3) Ignore errors. Printing anything at this stage will open a file descriptor // for logging. - if (!ValidateTargetSlotSuffix(arg[2])) { - LOG(ERROR) << "Target slot suffix not legal: " << arg[2]; + if (!ValidateTargetSlotSuffix(slot_suffix)) { + LOG(ERROR) << "Target slot suffix not legal: " << slot_suffix; exit(207); } - TryExtraMount("vendor", arg[2], "/postinstall/vendor"); + TryExtraMount("vendor", slot_suffix, "/postinstall/vendor"); // Try to mount the product partition. update_engine doesn't do this for us, but we // want it for product APKs. Same notes as vendor above. - TryExtraMount("product", arg[2], "/postinstall/product"); + TryExtraMount("product", slot_suffix, "/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"); + TryExtraMount("system_ext", slot_suffix, "/postinstall/system_ext"); constexpr const char* kPostInstallLinkerconfig = "/postinstall/linkerconfig"; // Try to mount /postinstall/linkerconfig. we will set it up after performing the chroot @@ -329,30 +350,37 @@ static int otapreopt_chroot(const int argc, char **arg) { exit(218); } - // Now go on and run otapreopt. + // Now go on and read dexopt lines from stdin and pass them on to otapreopt. - // Incoming: cmd + status-fd + target-slot + cmd... | Incoming | = argc - // Outgoing: cmd + target-slot + cmd... | Outgoing | = argc - 1 - std::vector<std::string> cmd; - cmd.reserve(argc); - cmd.push_back("/system/bin/otapreopt"); + int count = 1; + for (std::array<char, 1000> linebuf; + std::cin.clear(), std::cin.getline(&linebuf[0], linebuf.size()); ++count) { + // Subtract one from gcount() since getline() counts the newline. + std::string line(&linebuf[0], std::cin.gcount() - 1); - // The first parameter is the status file descriptor, skip. - for (size_t i = 2; i < static_cast<size_t>(argc); ++i) { - cmd.push_back(arg[i]); - } + if (std::cin.fail()) { + LOG(ERROR) << "Command exceeds max length " << linebuf.size() << " - skipped: " << line; + continue; + } - // Fork and execute otapreopt in its own process. - std::string error_msg; - bool exec_result = Exec(cmd, &error_msg); - if (!exec_result) { - LOG(ERROR) << "Running otapreopt failed: " << error_msg; - } + std::vector<std::string> tokenized_line = android::base::Tokenize(line, " "); + std::vector<std::string> cmd{"/system/bin/otapreopt", slot_suffix}; + std::move(tokenized_line.begin(), tokenized_line.end(), std::back_inserter(cmd)); - if (!exec_result) { - exit(213); + LOG(INFO) << "Command " << count << ": " << android::base::Join(cmd, " "); + + // Fork and execute otapreopt in its own process. + std::string error_msg; + bool exec_result = Exec(cmd, &error_msg); + if (!exec_result) { + LOG(ERROR) << "Running otapreopt failed: " << error_msg; + } + + // Print the count to stdout and flush to indicate progress. + std::cout << count << std::endl; } + LOG(INFO) << "No more dexopt commands"; return 0; } diff --git a/cmds/installd/otapreopt_script.sh b/cmds/installd/otapreopt_script.sh index db5c34edc2..28bd7932a2 100644 --- a/cmds/installd/otapreopt_script.sh +++ b/cmds/installd/otapreopt_script.sh @@ -16,7 +16,9 @@ # limitations under the License. # -# This script will run as a postinstall step to drive otapreopt. +# This script runs as a postinstall step to drive otapreopt. It comes with the +# OTA package, but runs /system/bin/otapreopt_chroot in the (old) active system +# image. See system/extras/postinst/postinst.sh for some docs. TARGET_SLOT="$1" STATUS_FD="$2" @@ -31,12 +33,11 @@ BOOT_PROPERTY_NAME="dev.bootcomplete" BOOT_COMPLETE=$(getprop $BOOT_PROPERTY_NAME) if [ "$BOOT_COMPLETE" != "1" ] ; then - echo "Error: boot-complete not detected." + echo "$0: Error: boot-complete not detected." # We must return 0 to not block sideload. exit 0 fi - # Compute target slot suffix. # TODO: Once bootctl is not restricted, we should query from there. Or get this from # update_engine as a parameter. @@ -45,45 +46,63 @@ if [ "$TARGET_SLOT" = "0" ] ; then elif [ "$TARGET_SLOT" = "1" ] ; then TARGET_SLOT_SUFFIX="_b" else - echo "Unknown target slot $TARGET_SLOT" + echo "$0: Unknown target slot $TARGET_SLOT" exit 1 fi +if [ "$(/system/bin/otapreopt_chroot --version)" != 2 ]; then + # We require an updated chroot wrapper that reads dexopt commands from stdin. + # Even if we kept compat with the old binary, the OTA preopt wouldn't work due + # to missing sepolicy rules, so there's no use spending time trying to dexopt + # (b/291974157). + echo "$0: Current system image is too old to work with OTA preopt - skipping." + exit 0 +fi PREPARE=$(cmd otadexopt prepare) # Note: Ignore preparation failures. Step and done will fail and exit this. # This is necessary to support suspends - the OTA service will keep # the state around for us. -PROGRESS=$(cmd otadexopt progress) -print -u${STATUS_FD} "global_progress $PROGRESS" - -i=0 -while ((i<MAXIMUM_PACKAGES)) ; do +# Create an array with all dexopt commands in advance, to know how many there are. +otadexopt_cmds=() +while (( ${#otadexopt_cmds[@]} < MAXIMUM_PACKAGES )) ; do DONE=$(cmd otadexopt done) if [ "$DONE" = "OTA complete." ] ; then break fi + otadexopt_cmds+=("$(cmd otadexopt next)") +done - DEXOPT_PARAMS=$(cmd otadexopt next) +DONE=$(cmd otadexopt done) +cmd otadexopt cleanup - /system/bin/otapreopt_chroot $STATUS_FD $TARGET_SLOT_SUFFIX $DEXOPT_PARAMS >&- 2>&- +echo "$0: Using streaming otapreopt_chroot on ${#otadexopt_cmds[@]} packages" - PROGRESS=$(cmd otadexopt progress) - print -u${STATUS_FD} "global_progress $PROGRESS" +function print_otadexopt_cmds { + for cmd in "${otadexopt_cmds[@]}" ; do + print "$cmd" + done +} - sleep 1 - i=$((i+1)) -done +function report_progress { + while read count ; do + # mksh can't do floating point arithmetic, so emulate a fixed point calculation. + (( permilles = 1000 * count / ${#otadexopt_cmds[@]} )) + printf 'global_progress %d.%03d\n' $((permilles / 1000)) $((permilles % 1000)) >&${STATUS_FD} + done +} + +print_otadexopt_cmds | \ + /system/bin/otapreopt_chroot $STATUS_FD $TARGET_SLOT_SUFFIX | \ + report_progress -DONE=$(cmd otadexopt done) if [ "$DONE" = "OTA incomplete." ] ; then - echo "Incomplete." + echo "$0: Incomplete." else - echo "Complete or error." + echo "$0: Complete or error." fi print -u${STATUS_FD} "global_progress 1.0" -cmd otadexopt cleanup exit 0 diff --git a/cmds/installd/utils.h b/cmds/installd/utils.h index ecea1d2b1c..c43fdbd547 100644 --- a/cmds/installd/utils.h +++ b/cmds/installd/utils.h @@ -18,6 +18,7 @@ #ifndef UTILS_H_ #define UTILS_H_ +#include <functional> #include <string> #include <vector> diff --git a/cmds/servicemanager/Android.bp b/cmds/servicemanager/Android.bp index fb69513d24..d73a30bf9b 100644 --- a/cmds/servicemanager/Android.bp +++ b/cmds/servicemanager/Android.bp @@ -93,22 +93,9 @@ cc_fuzz { libfuzzer_options: [ "max_len=50000", ], - }, -} - -// Adding this new fuzzer to test the corpus generated by record_binder -cc_fuzz { - name: "servicemanager_test_fuzzer", - defaults: [ - "servicemanager_defaults", - "service_fuzzer_defaults", - ], - host_supported: true, - srcs: ["fuzzers/ServiceManagerTestFuzzer.cpp"], - fuzz_config: { - libfuzzer_options: [ - "max_len=50000", + cc: [ + "smoreland@google.com", + "waghpawan@google.com", ], }, - corpus: ["fuzzers/servicemamanager_fuzzer_corpus/*"], } diff --git a/cmds/servicemanager/fuzzers/ServiceManagerTestFuzzer.cpp b/cmds/servicemanager/fuzzers/ServiceManagerTestFuzzer.cpp deleted file mode 100644 index e19b6eb279..0000000000 --- a/cmds/servicemanager/fuzzers/ServiceManagerTestFuzzer.cpp +++ /dev/null @@ -1,48 +0,0 @@ -/* - * Copyright (C) 2023 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include <fuzzbinder/libbinder_driver.h> -#include <utils/StrongPointer.h> - -#include "Access.h" -#include "ServiceManager.h" - -using ::android::Access; -using ::android::Parcel; -using ::android::ServiceManager; -using ::android::sp; - -extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { - FuzzedDataProvider provider(data, size); - auto accessPtr = std::make_unique<Access>(); - auto serviceManager = sp<ServiceManager>::make(std::move(accessPtr)); - - // Reserved bytes - provider.ConsumeBytes<uint8_t>(8); - uint32_t code = provider.ConsumeIntegral<uint32_t>(); - uint32_t flag = provider.ConsumeIntegral<uint32_t>(); - std::vector<uint8_t> parcelData = provider.ConsumeRemainingBytes<uint8_t>(); - - Parcel inputParcel; - inputParcel.setData(parcelData.data(), parcelData.size()); - - Parcel reply; - serviceManager->transact(code, inputParcel, &reply, flag); - - serviceManager->clear(); - - return 0; -} diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_1 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_1 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_1 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_10 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_10 Binary files differdeleted file mode 100644 index 07319f864e..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_10 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_11 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_11 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_11 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_12 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_12 Binary files differdeleted file mode 100644 index 07319f864e..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_12 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_13 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_13 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_13 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_14 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_14 Binary files differdeleted file mode 100644 index 07319f864e..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_14 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_15 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_15 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_15 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_16 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_16 Binary files differdeleted file mode 100644 index 07319f864e..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_16 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_17 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_17 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_17 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_18 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_18 Binary files differdeleted file mode 100644 index 88ad474f09..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_18 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_19 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_19 Binary files differdeleted file mode 100644 index fae15a2fea..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_19 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_2 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_2 Binary files differdeleted file mode 100644 index e69ab49d5d..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_2 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_20 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_20 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_20 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_21 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_21 Binary files differdeleted file mode 100644 index 88ad474f09..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_21 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_22 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_22 Binary files differdeleted file mode 100644 index fae15a2fea..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_22 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_23 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_23 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_23 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_24 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_24 Binary files differdeleted file mode 100644 index 88ad474f09..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_24 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_25 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_25 Binary files differdeleted file mode 100644 index fae15a2fea..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_25 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_26 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_26 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_26 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_27 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_27 Binary files differdeleted file mode 100644 index 88ad474f09..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_27 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_28 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_28 Binary files differdeleted file mode 100644 index fae15a2fea..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_28 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_29 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_29 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_29 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_3 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_3 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_3 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_30 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_30 Binary files differdeleted file mode 100644 index 88ad474f09..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_30 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_31 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_31 Binary files differdeleted file mode 100644 index fae15a2fea..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_31 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_32 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_32 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_32 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_33 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_33 Binary files differdeleted file mode 100644 index 88ad474f09..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_33 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_34 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_34 Binary files differdeleted file mode 100644 index fae15a2fea..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_34 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_35 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_35 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_35 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_36 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_36 Binary files differdeleted file mode 100644 index 88ad474f09..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_36 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_37 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_37 Binary files differdeleted file mode 100644 index fae15a2fea..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_37 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_38 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_38 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_38 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_39 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_39 Binary files differdeleted file mode 100644 index b326907a58..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_39 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_4 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_4 Binary files differdeleted file mode 100644 index 05b27bf413..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_4 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_40 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_40 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_40 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_41 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_41 Binary files differdeleted file mode 100644 index b326907a58..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_41 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_42 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_42 Binary files differdeleted file mode 100644 index cdaa1f01b1..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_42 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_43 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_43 Binary files differdeleted file mode 100644 index ff0941b7a6..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_43 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_44 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_44 Binary files differdeleted file mode 100644 index cdaa1f01b1..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_44 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_45 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_45 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_45 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_46 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_46 Binary files differdeleted file mode 100644 index 7e5f948682..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_46 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_5 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_5 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_5 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_6 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_6 Binary files differdeleted file mode 100644 index 07319f864e..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_6 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_7 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_7 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_7 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_8 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_8 Binary files differdeleted file mode 100644 index 07319f864e..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_8 +++ /dev/null diff --git a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_9 b/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_9 Binary files differdeleted file mode 100644 index 39e5104927..0000000000 --- a/cmds/servicemanager/fuzzers/servicemanager_fuzzer_corpus/Transaction_9 +++ /dev/null diff --git a/data/etc/Android.bp b/data/etc/Android.bp index a737bd3fb7..92dc46ed8d 100644 --- a/data/etc/Android.bp +++ b/data/etc/Android.bp @@ -173,6 +173,12 @@ prebuilt_etc { } prebuilt_etc { + name: "android.hardware.threadnetwork.prebuilt.xml", + src: "android.hardware.threadnetwork.xml", + defaults: ["frameworks_native_data_etc_defaults"], +} + +prebuilt_etc { name: "android.hardware.usb.accessory.prebuilt.xml", src: "android.hardware.usb.accessory.xml", defaults: ["frameworks_native_data_etc_defaults"], diff --git a/data/etc/android.hardware.threadnetwork.xml b/data/etc/android.hardware.threadnetwork.xml new file mode 100644 index 0000000000..9cbdc905fb --- /dev/null +++ b/data/etc/android.hardware.threadnetwork.xml @@ -0,0 +1,19 @@ +<?xml version="1.0" encoding="utf-8"?> +<!-- Copyright (C) 2023 The Android Open Source Project + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> +<!-- Adds the feature indicating support for the ThreadNetwork API --> +<permissions> + <feature name="android.hardware.threadnetwork" /> +</permissions> diff --git a/libs/binder/Android.bp b/libs/binder/Android.bp index deff76b3a5..f634c1dcde 100644 --- a/libs/binder/Android.bp +++ b/libs/binder/Android.bp @@ -190,6 +190,9 @@ cc_defaults { "-performance-move-const-arg", // b/273486801 "portability*", ], + lto: { + thin: true, + }, } cc_library_headers { diff --git a/libs/binder/ndk/.clang-format b/libs/binder/ndk/.clang-format index 9a9d936f15..60774143ee 100644 --- a/libs/binder/ndk/.clang-format +++ b/libs/binder/ndk/.clang-format @@ -2,9 +2,7 @@ BasedOnStyle: Google ColumnLimit: 100 IndentWidth: 4 ContinuationIndentWidth: 8 -PointerAlignment: Left TabWidth: 4 AllowShortFunctionsOnASingleLine: Inline PointerAlignment: Left -TabWidth: 4 UseTab: Never diff --git a/libs/binder/ndk/include_cpp/android/binder_auto_utils.h b/libs/binder/ndk/include_cpp/android/binder_auto_utils.h index d6937c2c52..ed53891e3d 100644 --- a/libs/binder/ndk/include_cpp/android/binder_auto_utils.h +++ b/libs/binder/ndk/include_cpp/android/binder_auto_utils.h @@ -115,17 +115,29 @@ class SpAIBinder { */ AIBinder** getR() { return &mBinder; } - bool operator!=(const SpAIBinder& rhs) const { return get() != rhs.get(); } - bool operator<(const SpAIBinder& rhs) const { return get() < rhs.get(); } - bool operator<=(const SpAIBinder& rhs) const { return get() <= rhs.get(); } - bool operator==(const SpAIBinder& rhs) const { return get() == rhs.get(); } - bool operator>(const SpAIBinder& rhs) const { return get() > rhs.get(); } - bool operator>=(const SpAIBinder& rhs) const { return get() >= rhs.get(); } - private: AIBinder* mBinder = nullptr; }; +#define SP_AIBINDER_COMPARE(_op_) \ + static inline bool operator _op_(const SpAIBinder& lhs, const SpAIBinder& rhs) { \ + return lhs.get() _op_ rhs.get(); \ + } \ + static inline bool operator _op_(const SpAIBinder& lhs, const AIBinder* rhs) { \ + return lhs.get() _op_ rhs; \ + } \ + static inline bool operator _op_(const AIBinder* lhs, const SpAIBinder& rhs) { \ + return lhs _op_ rhs.get(); \ + } + +SP_AIBINDER_COMPARE(!=) +SP_AIBINDER_COMPARE(<) +SP_AIBINDER_COMPARE(<=) +SP_AIBINDER_COMPARE(==) +SP_AIBINDER_COMPARE(>) +SP_AIBINDER_COMPARE(>=) +#undef SP_AIBINDER_COMPARE + namespace impl { /** diff --git a/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp b/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp index 27ce615565..25b8e975b3 100644 --- a/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp +++ b/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp @@ -377,18 +377,24 @@ TEST(NdkBinder, CantHaveTwoLocalBinderClassesWithSameDescriptor) { } TEST(NdkBinder, GetTestServiceStressTest) { - // libbinder has some complicated logic to make sure only one instance of - // ABpBinder is associated with each binder. - constexpr size_t kNumThreads = 10; constexpr size_t kNumCalls = 1000; std::vector<std::thread> threads; + // this is not a lazy service, but we must make sure that it's started before calling + // checkService on it, since the other process serving it might not be started yet. + { + // getService, not waitForService, to take advantage of timeout + auto binder = ndk::SpAIBinder(AServiceManager_getService(IFoo::kSomeInstanceName)); + ASSERT_NE(nullptr, binder.get()); + } + for (size_t i = 0; i < kNumThreads; i++) { threads.push_back(std::thread([&]() { for (size_t j = 0; j < kNumCalls; j++) { auto binder = ndk::SpAIBinder(AServiceManager_checkService(IFoo::kSomeInstanceName)); + ASSERT_NE(nullptr, binder.get()); EXPECT_EQ(STATUS_OK, AIBinder_ping(binder.get())); } })); @@ -755,9 +761,9 @@ TEST(NdkBinder, ConvertToPlatformBinder) { // local ndk::SharedRefBase::make<MyBinderNdkUnitTest>()->asBinder()}) { // convert to platform binder - EXPECT_NE(binder.get(), nullptr); + EXPECT_NE(binder, nullptr); sp<IBinder> platformBinder = AIBinder_toPlatformBinder(binder.get()); - EXPECT_NE(platformBinder.get(), nullptr); + EXPECT_NE(platformBinder, nullptr); auto proxy = interface_cast<IBinderNdkUnitTest>(platformBinder); EXPECT_NE(proxy, nullptr); @@ -768,7 +774,7 @@ TEST(NdkBinder, ConvertToPlatformBinder) { // convert back ndk::SpAIBinder backBinder = ndk::SpAIBinder(AIBinder_fromPlatformBinder(platformBinder)); - EXPECT_EQ(backBinder.get(), binder.get()); + EXPECT_EQ(backBinder, binder); } } diff --git a/libs/binder/rust/Android.bp b/libs/binder/rust/Android.bp index d36ebac109..672d6cf5d0 100644 --- a/libs/binder/rust/Android.bp +++ b/libs/binder/rust/Android.bp @@ -97,34 +97,12 @@ rust_bindgen { crate_name: "binder_ndk_bindgen", wrapper_src: "sys/BinderBindings.hpp", source_stem: "bindings", - bindgen_flags: [ + bindgen_flag_files: [ // Unfortunately the only way to specify the rust_non_exhaustive enum // style for a type is to make it the default - "--default-enum-style", - "rust_non_exhaustive", // and then specify constified enums for the enums we don't want // rustified - "--constified-enum", - "android::c_interface::consts::.*", - - "--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", - ".*", + "libbinder_ndk_bindgen_flags.txt", ], shared_libs: [ "libbinder_ndk", diff --git a/libs/binder/rust/binder_tokio/lib.rs b/libs/binder/rust/binder_tokio/lib.rs index 2d2bf7c582..1dc0b2471d 100644 --- a/libs/binder/rust/binder_tokio/lib.rs +++ b/libs/binder/rust/binder_tokio/lib.rs @@ -103,7 +103,12 @@ impl BinderAsyncPool for Tokio { // // This shouldn't cause issues with blocking the thread as only one task will run in a // call to `block_on`, so there aren't other tasks to block. - let result = spawn_me(); + // + // If the `block_in_place` call fails, then you are driving a current-thread runtime on + // the binder threadpool. Instead, it is recommended to use `TokioRuntime<Handle>` when + // the runtime is a current-thread runtime, as the current-thread runtime can be driven + // only by `Runtime::block_on` calls and not by `Handle::block_on`. + let result = tokio::task::block_in_place(spawn_me); Box::pin(after_spawn(result)) } else { let handle = tokio::task::spawn_blocking(spawn_me); diff --git a/libs/binder/rust/libbinder_ndk_bindgen_flags.txt b/libs/binder/rust/libbinder_ndk_bindgen_flags.txt new file mode 100644 index 0000000000..551c59f671 --- /dev/null +++ b/libs/binder/rust/libbinder_ndk_bindgen_flags.txt @@ -0,0 +1,11 @@ +--default-enum-style=rust_non_exhaustive +--constified-enum=android::c_interface::consts::.* +--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=.* diff --git a/libs/binder/rust/rpcbinder/src/server.rs b/libs/binder/rust/rpcbinder/src/server.rs index 81f68f5a29..6fda878d07 100644 --- a/libs/binder/rust/rpcbinder/src/server.rs +++ b/libs/binder/rust/rpcbinder/src/server.rs @@ -33,9 +33,9 @@ foreign_type! { pub struct RpcServerRef; } -/// SAFETY - The opaque handle can be cloned freely. +/// SAFETY: The opaque handle can be cloned freely. unsafe impl Send for RpcServer {} -/// SAFETY - The underlying C++ RpcServer class is thread-safe. +/// SAFETY: The underlying C++ RpcServer class is thread-safe. unsafe impl Sync for RpcServer {} impl RpcServer { @@ -59,7 +59,10 @@ impl RpcServer { /// Creates a binder RPC server, serving the supplied binder service implementation on the given /// socket file descriptor. The socket should be bound to an address before calling this /// function. - pub fn new_bound_socket(mut service: SpIBinder, socket_fd: OwnedFd) -> Result<RpcServer, Error> { + pub fn new_bound_socket( + mut service: SpIBinder, + socket_fd: OwnedFd, + ) -> Result<RpcServer, Error> { let service = service.as_native_mut(); // SAFETY: Service ownership is transferring to the server and won't be valid afterward. @@ -67,7 +70,8 @@ impl RpcServer { // The server takes ownership of the socket FD. unsafe { Self::checked_from_ptr(binder_rpc_unstable_bindgen::ARpcServer_newBoundSocket( - service, socket_fd.into_raw_fd(), + service, + socket_fd.into_raw_fd(), )) } } @@ -120,7 +124,9 @@ impl RpcServer { if ptr.is_null() { return Err(Error::new(ErrorKind::Other, "Failed to start server")); } - Ok(RpcServer::from_ptr(ptr)) + // SAFETY: Our caller must pass us a valid or null pointer, and we've checked that it's not + // null. + Ok(unsafe { RpcServer::from_ptr(ptr) }) } } @@ -130,7 +136,7 @@ impl RpcServerRef { &self, modes: &[FileDescriptorTransportMode], ) { - // SAFETY - Does not keep the pointer after returning does, nor does it + // SAFETY: Does not keep the pointer after returning does, nor does it // read past its boundary. Only passes the 'self' pointer as an opaque handle. unsafe { binder_rpc_unstable_bindgen::ARpcServer_setSupportedFileDescriptorTransportModes( @@ -143,18 +149,21 @@ impl RpcServerRef { /// Starts a new background thread and calls join(). Returns immediately. pub fn start(&self) { + // SAFETY: RpcServerRef wraps a valid pointer to an ARpcServer. unsafe { binder_rpc_unstable_bindgen::ARpcServer_start(self.as_ptr()) }; } /// Joins the RpcServer thread. The call blocks until the server terminates. /// This must be called from exactly one thread. pub fn join(&self) { + // SAFETY: RpcServerRef wraps a valid pointer to an ARpcServer. unsafe { binder_rpc_unstable_bindgen::ARpcServer_join(self.as_ptr()) }; } /// Shuts down the running RpcServer. Can be called multiple times and from /// multiple threads. Called automatically during drop(). pub fn shutdown(&self) -> Result<(), Error> { + // SAFETY: RpcServerRef wraps a valid pointer to an ARpcServer. if unsafe { binder_rpc_unstable_bindgen::ARpcServer_shutdown(self.as_ptr()) } { Ok(()) } else { diff --git a/libs/binder/rust/rpcbinder/src/session.rs b/libs/binder/rust/rpcbinder/src/session.rs index 28c5390665..79a951073e 100644 --- a/libs/binder/rust/rpcbinder/src/session.rs +++ b/libs/binder/rust/rpcbinder/src/session.rs @@ -36,15 +36,15 @@ foreign_type! { pub struct RpcSessionRef; } -/// SAFETY - The opaque handle can be cloned freely. +/// SAFETY: The opaque handle can be cloned freely. unsafe impl Send for RpcSession {} -/// SAFETY - The underlying C++ RpcSession class is thread-safe. +/// SAFETY: The underlying C++ RpcSession class is thread-safe. unsafe impl Sync for RpcSession {} impl RpcSession { /// Allocates a new RpcSession object. pub fn new() -> RpcSession { - // SAFETY - Takes ownership of the returned handle, which has correct refcount. + // SAFETY: Takes ownership of the returned handle, which has correct refcount. unsafe { RpcSession::from_ptr(binder_rpc_unstable_bindgen::ARpcSession_new()) } } } @@ -58,7 +58,7 @@ impl Default for RpcSession { impl RpcSessionRef { /// Sets the file descriptor transport mode for this session. pub fn set_file_descriptor_transport_mode(&self, mode: FileDescriptorTransportMode) { - // SAFETY - Only passes the 'self' pointer as an opaque handle. + // SAFETY: Only passes the 'self' pointer as an opaque handle. unsafe { binder_rpc_unstable_bindgen::ARpcSession_setFileDescriptorTransportMode( self.as_ptr(), @@ -69,7 +69,7 @@ impl RpcSessionRef { /// Sets the maximum number of incoming threads. pub fn set_max_incoming_threads(&self, threads: usize) { - // SAFETY - Only passes the 'self' pointer as an opaque handle. + // SAFETY: Only passes the 'self' pointer as an opaque handle. unsafe { binder_rpc_unstable_bindgen::ARpcSession_setMaxIncomingThreads(self.as_ptr(), threads) }; @@ -77,7 +77,7 @@ impl RpcSessionRef { /// Sets the maximum number of outgoing connections. pub fn set_max_outgoing_connections(&self, connections: usize) { - // SAFETY - Only passes the 'self' pointer as an opaque handle. + // SAFETY: Only passes the 'self' pointer as an opaque handle. unsafe { binder_rpc_unstable_bindgen::ARpcSession_setMaxOutgoingConnections( self.as_ptr(), @@ -210,10 +210,10 @@ impl RpcSessionRef { type RequestFd<'a> = &'a mut dyn FnMut() -> Option<RawFd>; unsafe extern "C" fn request_fd_wrapper(param: *mut c_void) -> c_int { + let request_fd_ptr = param as *mut RequestFd; // SAFETY: This is only ever called by RpcPreconnectedClient, within the lifetime of the // BinderFdFactory reference, with param being a properly aligned non-null pointer to an // initialized instance. - let request_fd_ptr = param as *mut RequestFd; - let request_fd = request_fd_ptr.as_mut().unwrap(); + let request_fd = unsafe { request_fd_ptr.as_mut().unwrap() }; request_fd().unwrap_or(-1) } diff --git a/libs/binder/rust/src/binder.rs b/libs/binder/rust/src/binder.rs index 993bdca4a5..463c210316 100644 --- a/libs/binder/rust/src/binder.rs +++ b/libs/binder/rust/src/binder.rs @@ -97,8 +97,8 @@ where /// Interface stability promise /// -/// An interface can promise to be a stable vendor interface ([`Vintf`]), or -/// makes no stability guarantees ([`Local`]). [`Local`] is +/// An interface can promise to be a stable vendor interface ([`Stability::Vintf`]), +/// or makes no stability guarantees ([`Stability::Local`]). [`Stability::Local`] is /// currently the default stability. #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Default)] pub enum Stability { @@ -139,8 +139,8 @@ impl TryFrom<i32> for Stability { /// via `Binder::new(object)`. /// /// This is a low-level interface that should normally be automatically -/// generated from AIDL via the [`declare_binder_interface!`] macro. When using -/// the AIDL backend, users need only implement the high-level AIDL-defined +/// generated from AIDL via the [`crate::declare_binder_interface!`] macro. +/// When using the AIDL backend, users need only implement the high-level AIDL-defined /// interface. The AIDL compiler then generates a container struct that wraps /// the user-defined service and implements `Remotable`. pub trait Remotable: Send + Sync { @@ -297,18 +297,17 @@ impl InterfaceClass { /// Note: the returned pointer will not be constant. Calling this method /// multiple times for the same type will result in distinct class /// pointers. A static getter for this value is implemented in - /// [`declare_binder_interface!`]. + /// [`crate::declare_binder_interface!`]. pub fn new<I: InterfaceClassMethods>() -> InterfaceClass { let descriptor = CString::new(I::get_descriptor()).unwrap(); + // Safety: `AIBinder_Class_define` expects a valid C string, and three + // valid callback functions, all non-null pointers. The C string is + // copied and need not be valid for longer than the call, so we can drop + // it after the call. We can safely assign null to the onDump and + // handleShellCommand callbacks as long as the class pointer was + // non-null. Rust None for a Option<fn> is guaranteed to be a NULL + // pointer. Rust retains ownership of the pointer after it is defined. let ptr = unsafe { - // Safety: `AIBinder_Class_define` expects a valid C string, and - // three valid callback functions, all non-null pointers. The C - // string is copied and need not be valid for longer than the call, - // so we can drop it after the call. We can safely assign null to - // the onDump and handleShellCommand callbacks as long as the class - // pointer was non-null. Rust None for a Option<fn> is guaranteed to - // be a NULL pointer. Rust retains ownership of the pointer after it - // is defined. let class = sys::AIBinder_Class_define( descriptor.as_ptr(), Some(I::on_create), @@ -338,13 +337,12 @@ impl InterfaceClass { /// Get the interface descriptor string of this class. pub fn get_descriptor(&self) -> String { + // SAFETY: The descriptor returned by AIBinder_Class_getDescriptor is + // always a two-byte null terminated sequence of u16s. Thus, we can + // continue reading from the pointer until we hit a null value, and this + // pointer can be a valid slice if the slice length is <= the number of + // u16 elements before the null terminator. unsafe { - // SAFETY: The descriptor returned by AIBinder_Class_getDescriptor - // is always a two-byte null terminated sequence of u16s. Thus, we - // can continue reading from the pointer until we hit a null value, - // and this pointer can be a valid slice if the slice length is <= - // the number of u16 elements before the null terminator. - let raw_descriptor: *const c_char = sys::AIBinder_Class_getDescriptor(self.0); CStr::from_ptr(raw_descriptor) .to_str() @@ -542,17 +540,15 @@ macro_rules! binder_fn_get_class { static CLASS_INIT: std::sync::Once = std::sync::Once::new(); static mut CLASS: Option<$crate::binder_impl::InterfaceClass> = None; + // Safety: This assignment is guarded by the `CLASS_INIT` `Once` + // variable, and therefore is thread-safe, as it can only occur + // once. CLASS_INIT.call_once(|| unsafe { - // Safety: This assignment is guarded by the `CLASS_INIT` `Once` - // variable, and therefore is thread-safe, as it can only occur - // once. CLASS = Some($constructor); }); - unsafe { - // Safety: The `CLASS` variable can only be mutated once, above, - // and is subsequently safe to read from any thread. - CLASS.unwrap() - } + // Safety: The `CLASS` variable can only be mutated once, above, and + // is subsequently safe to read from any thread. + unsafe { CLASS.unwrap() } } }; } @@ -664,6 +660,8 @@ pub unsafe trait AsNative<T> { fn as_native_mut(&mut self) -> *mut T; } +// Safety: If V is a valid Android C++ type then we can either use that or a +// null pointer. unsafe impl<T, V: AsNative<T>> AsNative<T> for Option<V> { fn as_native(&self) -> *const T { self.as_ref().map_or(ptr::null(), |v| v.as_native()) @@ -924,15 +922,15 @@ macro_rules! declare_binder_interface { static CLASS_INIT: std::sync::Once = std::sync::Once::new(); static mut CLASS: Option<$crate::binder_impl::InterfaceClass> = None; + // Safety: This assignment is guarded by the `CLASS_INIT` `Once` + // variable, and therefore is thread-safe, as it can only occur + // once. CLASS_INIT.call_once(|| unsafe { - // Safety: This assignment is guarded by the `CLASS_INIT` `Once` - // variable, and therefore is thread-safe, as it can only occur - // once. CLASS = Some($crate::binder_impl::InterfaceClass::new::<$crate::binder_impl::Binder<$native>>()); }); + // Safety: The `CLASS` variable can only be mutated once, above, + // and is subsequently safe to read from any thread. unsafe { - // Safety: The `CLASS` variable can only be mutated once, above, - // and is subsequently safe to read from any thread. CLASS.unwrap() } } @@ -1025,17 +1023,7 @@ macro_rules! declare_binder_interface { } if ibinder.associate_class(<$native as $crate::binder_impl::Remotable>::get_class()) { - let service: std::result::Result<$crate::binder_impl::Binder<$native>, $crate::StatusCode> = - std::convert::TryFrom::try_from(ibinder.clone()); - if let Ok(service) = service { - // We were able to associate with our expected class and - // the service is local. - todo!() - //return Ok($crate::Strong::new(Box::new(service))); - } else { - // Service is remote - return Ok($crate::Strong::new(Box::new(<$proxy as $crate::binder_impl::Proxy>::from_binder(ibinder)?))); - } + return Ok($crate::Strong::new(Box::new(<$proxy as $crate::binder_impl::Proxy>::from_binder(ibinder)?))); } Err($crate::StatusCode::BAD_TYPE.into()) diff --git a/libs/binder/rust/src/error.rs b/libs/binder/rust/src/error.rs index ba260624bc..eb04cc3153 100644 --- a/libs/binder/rust/src/error.rs +++ b/libs/binder/rust/src/error.rs @@ -112,41 +112,35 @@ fn to_cstring<T: AsRef<str>>(message: T) -> Option<CString> { impl Status { /// Create a status object representing a successful transaction. pub fn ok() -> Self { - let ptr = unsafe { - // Safety: `AStatus_newOk` always returns a new, heap allocated - // pointer to an `ASTatus` object, so we know this pointer will be - // valid. - // - // Rust takes ownership of the returned pointer. - sys::AStatus_newOk() - }; + // Safety: `AStatus_newOk` always returns a new, heap allocated + // pointer to an `ASTatus` object, so we know this pointer will be + // valid. + // + // Rust takes ownership of the returned pointer. + let ptr = unsafe { sys::AStatus_newOk() }; Self(ptr::NonNull::new(ptr).expect("Unexpected null AStatus pointer")) } /// Create a status object from a service specific error pub fn new_service_specific_error(err: i32, message: Option<&CStr>) -> Status { let ptr = if let Some(message) = message { - unsafe { - // Safety: Any i32 is a valid service specific error for the - // error code parameter. We construct a valid, null-terminated - // `CString` from the message, which must be a valid C-style - // string to pass as the message. This function always returns a - // new, heap allocated pointer to an `AStatus` object, so we - // know the returned pointer will be valid. - // - // Rust takes ownership of the returned pointer. - sys::AStatus_fromServiceSpecificErrorWithMessage(err, message.as_ptr()) - } + // Safety: Any i32 is a valid service specific error for the + // error code parameter. We construct a valid, null-terminated + // `CString` from the message, which must be a valid C-style + // string to pass as the message. This function always returns a + // new, heap allocated pointer to an `AStatus` object, so we + // know the returned pointer will be valid. + // + // Rust takes ownership of the returned pointer. + unsafe { sys::AStatus_fromServiceSpecificErrorWithMessage(err, message.as_ptr()) } } else { - unsafe { - // Safety: Any i32 is a valid service specific error for the - // error code parameter. This function always returns a new, - // heap allocated pointer to an `AStatus` object, so we know the - // returned pointer will be valid. - // - // Rust takes ownership of the returned pointer. - sys::AStatus_fromServiceSpecificError(err) - } + // Safety: Any i32 is a valid service specific error for the + // error code parameter. This function always returns a new, + // heap allocated pointer to an `AStatus` object, so we know the + // returned pointer will be valid. + // + // Rust takes ownership of the returned pointer. + unsafe { sys::AStatus_fromServiceSpecificError(err) } }; Self(ptr::NonNull::new(ptr).expect("Unexpected null AStatus pointer")) } @@ -159,6 +153,8 @@ impl Status { /// Create a status object from an exception code pub fn new_exception(exception: ExceptionCode, message: Option<&CStr>) -> Status { if let Some(message) = message { + // Safety: the C string pointer is valid and not retained by the + // function. let ptr = unsafe { sys::AStatus_fromExceptionCodeWithMessage(exception as i32, message.as_ptr()) }; @@ -187,37 +183,31 @@ impl Status { /// Returns `true` if this status represents a successful transaction. pub fn is_ok(&self) -> bool { - unsafe { - // Safety: `Status` always contains a valid `AStatus` pointer, so we - // are always passing a valid pointer to `AStatus_isOk` here. - sys::AStatus_isOk(self.as_native()) - } + // Safety: `Status` always contains a valid `AStatus` pointer, so we + // are always passing a valid pointer to `AStatus_isOk` here. + unsafe { sys::AStatus_isOk(self.as_native()) } } /// Returns a description of the status. pub fn get_description(&self) -> String { - let description_ptr = unsafe { - // Safety: `Status` always contains a valid `AStatus` pointer, so we - // are always passing a valid pointer to `AStatus_getDescription` - // here. - // - // `AStatus_getDescription` always returns a valid pointer to a null - // terminated C string. Rust is responsible for freeing this pointer - // via `AStatus_deleteDescription`. - sys::AStatus_getDescription(self.as_native()) - }; - let description = unsafe { - // Safety: `AStatus_getDescription` always returns a valid C string, - // which can be safely converted to a `CStr`. - CStr::from_ptr(description_ptr) - }; + // Safety: `Status` always contains a valid `AStatus` pointer, so we + // are always passing a valid pointer to `AStatus_getDescription` + // here. + // + // `AStatus_getDescription` always returns a valid pointer to a null + // terminated C string. Rust is responsible for freeing this pointer + // via `AStatus_deleteDescription`. + let description_ptr = unsafe { sys::AStatus_getDescription(self.as_native()) }; + // Safety: `AStatus_getDescription` always returns a valid C string, + // which can be safely converted to a `CStr`. + let description = unsafe { CStr::from_ptr(description_ptr) }; let description = description.to_string_lossy().to_string(); + // Safety: `description_ptr` was returned from + // `AStatus_getDescription` above, and must be freed via + // `AStatus_deleteDescription`. We must not access the pointer after + // this call, so we copy it into an owned string above and return + // that string. unsafe { - // Safety: `description_ptr` was returned from - // `AStatus_getDescription` above, and must be freed via - // `AStatus_deleteDescription`. We must not access the pointer after - // this call, so we copy it into an owned string above and return - // that string. sys::AStatus_deleteDescription(description_ptr); } description @@ -225,12 +215,10 @@ impl Status { /// Returns the exception code of the status. pub fn exception_code(&self) -> ExceptionCode { - let code = unsafe { - // Safety: `Status` always contains a valid `AStatus` pointer, so we - // are always passing a valid pointer to `AStatus_getExceptionCode` - // here. - sys::AStatus_getExceptionCode(self.as_native()) - }; + // Safety: `Status` always contains a valid `AStatus` pointer, so we + // are always passing a valid pointer to `AStatus_getExceptionCode` + // here. + let code = unsafe { sys::AStatus_getExceptionCode(self.as_native()) }; parse_exception_code(code) } @@ -241,11 +229,9 @@ impl Status { /// exception or a service specific error. To find out if this transaction /// as a whole is okay, use [`is_ok`](Self::is_ok) instead. pub fn transaction_error(&self) -> StatusCode { - let code = unsafe { - // Safety: `Status` always contains a valid `AStatus` pointer, so we - // are always passing a valid pointer to `AStatus_getStatus` here. - sys::AStatus_getStatus(self.as_native()) - }; + // Safety: `Status` always contains a valid `AStatus` pointer, so we + // are always passing a valid pointer to `AStatus_getStatus` here. + let code = unsafe { sys::AStatus_getStatus(self.as_native()) }; parse_status_code(code) } @@ -258,12 +244,10 @@ impl Status { /// find out if this transaction as a whole is okay, use /// [`is_ok`](Self::is_ok) instead. pub fn service_specific_error(&self) -> i32 { - unsafe { - // Safety: `Status` always contains a valid `AStatus` pointer, so we - // are always passing a valid pointer to - // `AStatus_getServiceSpecificError` here. - sys::AStatus_getServiceSpecificError(self.as_native()) - } + // Safety: `Status` always contains a valid `AStatus` pointer, so we + // are always passing a valid pointer to + // `AStatus_getServiceSpecificError` here. + unsafe { sys::AStatus_getServiceSpecificError(self.as_native()) } } /// Calls `op` if the status was ok, otherwise returns an `Err` value of @@ -321,24 +305,20 @@ impl From<StatusCode> for Status { impl From<status_t> for Status { fn from(status: status_t) -> Status { - let ptr = unsafe { - // Safety: `AStatus_fromStatus` expects any `status_t` integer, so - // this is a safe FFI call. Unknown values will be coerced into - // UNKNOWN_ERROR. - sys::AStatus_fromStatus(status) - }; + // Safety: `AStatus_fromStatus` expects any `status_t` integer, so + // this is a safe FFI call. Unknown values will be coerced into + // UNKNOWN_ERROR. + let ptr = unsafe { sys::AStatus_fromStatus(status) }; Self(ptr::NonNull::new(ptr).expect("Unexpected null AStatus pointer")) } } impl From<ExceptionCode> for Status { fn from(code: ExceptionCode) -> Status { - let ptr = unsafe { - // Safety: `AStatus_fromExceptionCode` expects any - // `binder_exception_t` (i32) integer, so this is a safe FFI call. - // Unknown values will be coerced into EX_TRANSACTION_FAILED. - sys::AStatus_fromExceptionCode(code as i32) - }; + // Safety: `AStatus_fromExceptionCode` expects any + // `binder_exception_t` (i32) integer, so this is a safe FFI call. + // Unknown values will be coerced into EX_TRANSACTION_FAILED. + let ptr = unsafe { sys::AStatus_fromExceptionCode(code as i32) }; Self(ptr::NonNull::new(ptr).expect("Unexpected null AStatus pointer")) } } @@ -363,20 +343,18 @@ impl From<Status> for status_t { impl Drop for Status { fn drop(&mut self) { + // Safety: `Status` manages the lifetime of its inner `AStatus` + // pointee, so we need to delete it here. We know that the pointer + // will be valid here since `Status` always contains a valid pointer + // while it is alive. unsafe { - // Safety: `Status` manages the lifetime of its inner `AStatus` - // pointee, so we need to delete it here. We know that the pointer - // will be valid here since `Status` always contains a valid pointer - // while it is alive. sys::AStatus_delete(self.0.as_mut()); } } } -/// # Safety -/// -/// `Status` always contains a valid pointer to an `AStatus` object, so we can -/// trivially convert it to a correctly-typed raw pointer. +/// Safety: `Status` always contains a valid pointer to an `AStatus` object, so +/// we can trivially convert it to a correctly-typed raw pointer. /// /// Care must be taken that the returned pointer is only dereferenced while the /// `Status` object is still alive. @@ -386,11 +364,97 @@ unsafe impl AsNative<sys::AStatus> for Status { } fn as_native_mut(&mut self) -> *mut sys::AStatus { - unsafe { - // Safety: The pointer will be valid here since `Status` always - // contains a valid and initialized pointer while it is alive. - self.0.as_mut() - } + // Safety: The pointer will be valid here since `Status` always contains + // a valid and initialized pointer while it is alive. + unsafe { self.0.as_mut() } + } +} + +/// A conversion from `std::result::Result<T, E>` to `binder::Result<T>`. If this type is `Ok(T)`, +/// it's returned as is. If this type is `Err(E)`, `E` is converted into `Status` which can be +/// either a general binder exception, or a service-specific exception. +/// +/// # Examples +/// +/// ``` +/// // std::io::Error is formatted as the exception's message +/// fn file_exists(name: &str) -> binder::Result<bool> { +/// std::fs::metadata(name) +/// .or_service_specific_exception(NOT_FOUND)? +/// } +/// +/// // A custom function is used to create the exception's message +/// fn file_exists(name: &str) -> binder::Result<bool> { +/// std::fs::metadata(name) +/// .or_service_specific_exception_with(NOT_FOUND, +/// |e| format!("file {} not found: {:?}", name, e))? +/// } +/// +/// // anyhow::Error is formatted as the exception's message +/// use anyhow::{Context, Result}; +/// fn file_exists(name: &str) -> binder::Result<bool> { +/// std::fs::metadata(name) +/// .context("file {} not found") +/// .or_service_specific_exception(NOT_FOUND)? +/// } +/// +/// // General binder exceptions can be created similarly +/// fn file_exists(name: &str) -> binder::Result<bool> { +/// std::fs::metadata(name) +/// .or_binder_exception(ExceptionCode::ILLEGAL_ARGUMENT)? +/// } +/// ``` +pub trait IntoBinderResult<T, E> { + /// Converts the embedded error into a general binder exception of code `exception`. The + /// message of the exception is set by formatting the error for debugging. + fn or_binder_exception(self, exception: ExceptionCode) -> result::Result<T, Status>; + + /// Converts the embedded error into a general binder exception of code `exception`. The + /// message of the exception is set by lazily evaluating the `op` function. + fn or_binder_exception_with<M: AsRef<str>, O: FnOnce(E) -> M>( + self, + exception: ExceptionCode, + op: O, + ) -> result::Result<T, Status>; + + /// Converts the embedded error into a service-specific binder exception. `error_code` is used + /// to distinguish different service-specific binder exceptions. The message of the exception + /// is set by formatting the error for debugging. + fn or_service_specific_exception(self, error_code: i32) -> result::Result<T, Status>; + + /// Converts the embedded error into a service-specific binder exception. `error_code` is used + /// to distinguish different service-specific binder exceptions. The message of the exception + /// is set by lazily evaluating the `op` function. + fn or_service_specific_exception_with<M: AsRef<str>, O: FnOnce(E) -> M>( + self, + error_code: i32, + op: O, + ) -> result::Result<T, Status>; +} + +impl<T, E: std::fmt::Debug> IntoBinderResult<T, E> for result::Result<T, E> { + fn or_binder_exception(self, exception: ExceptionCode) -> result::Result<T, Status> { + self.or_binder_exception_with(exception, |e| format!("{:?}", e)) + } + + fn or_binder_exception_with<M: AsRef<str>, O: FnOnce(E) -> M>( + self, + exception: ExceptionCode, + op: O, + ) -> result::Result<T, Status> { + self.map_err(|e| Status::new_exception_str(exception, Some(op(e)))) + } + + fn or_service_specific_exception(self, error_code: i32) -> result::Result<T, Status> { + self.or_service_specific_exception_with(error_code, |e| format!("{:?}", e)) + } + + fn or_service_specific_exception_with<M: AsRef<str>, O: FnOnce(E) -> M>( + self, + error_code: i32, + op: O, + ) -> result::Result<T, Status> { + self.map_err(|e| Status::new_service_specific_error_str(error_code, Some(op(e)))) } } @@ -430,4 +494,66 @@ mod tests { assert_eq!(status.service_specific_error(), 0); assert_eq!(status.get_description(), "Status(-5, EX_ILLEGAL_STATE): ''".to_string()); } + + #[test] + fn convert_to_service_specific_exception() { + let res: std::result::Result<(), Status> = + Err("message").or_service_specific_exception(-42); + + assert!(res.is_err()); + let status = res.unwrap_err(); + assert_eq!(status.exception_code(), ExceptionCode::SERVICE_SPECIFIC); + assert_eq!(status.service_specific_error(), -42); + assert_eq!( + status.get_description(), + "Status(-8, EX_SERVICE_SPECIFIC): '-42: \"message\"'".to_string() + ); + } + + #[test] + fn convert_to_service_specific_exception_with() { + let res: std::result::Result<(), Status> = Err("message") + .or_service_specific_exception_with(-42, |e| format!("outer message: {:?}", e)); + + assert!(res.is_err()); + let status = res.unwrap_err(); + assert_eq!(status.exception_code(), ExceptionCode::SERVICE_SPECIFIC); + assert_eq!(status.service_specific_error(), -42); + assert_eq!( + status.get_description(), + "Status(-8, EX_SERVICE_SPECIFIC): '-42: outer message: \"message\"'".to_string() + ); + } + + #[test] + fn convert_to_binder_exception() { + let res: std::result::Result<(), Status> = + Err("message").or_binder_exception(ExceptionCode::ILLEGAL_STATE); + + assert!(res.is_err()); + let status = res.unwrap_err(); + assert_eq!(status.exception_code(), ExceptionCode::ILLEGAL_STATE); + assert_eq!(status.service_specific_error(), 0); + assert_eq!( + status.get_description(), + "Status(-5, EX_ILLEGAL_STATE): '\"message\"'".to_string() + ); + } + + #[test] + fn convert_to_binder_exception_with() { + let res: std::result::Result<(), Status> = Err("message") + .or_binder_exception_with(ExceptionCode::ILLEGAL_STATE, |e| { + format!("outer message: {:?}", e) + }); + + assert!(res.is_err()); + let status = res.unwrap_err(); + assert_eq!(status.exception_code(), ExceptionCode::ILLEGAL_STATE); + assert_eq!(status.service_specific_error(), 0); + assert_eq!( + status.get_description(), + "Status(-5, EX_ILLEGAL_STATE): 'outer message: \"message\"'".to_string() + ); + } } diff --git a/libs/binder/rust/src/lib.rs b/libs/binder/rust/src/lib.rs index 0c8b48f1f5..8841fe640b 100644 --- a/libs/binder/rust/src/lib.rs +++ b/libs/binder/rust/src/lib.rs @@ -106,7 +106,7 @@ use binder_ndk_sys as sys; pub use crate::binder_async::{BinderAsyncPool, BoxFuture}; pub use binder::{BinderFeatures, FromIBinder, IBinder, Interface, Strong, Weak}; -pub use error::{ExceptionCode, Status, StatusCode}; +pub use error::{ExceptionCode, IntoBinderResult, Status, StatusCode}; pub use native::{ add_service, force_lazy_services_persist, is_handling_transaction, register_lazy_service, LazyServiceGuard, diff --git a/libs/binder/rust/src/native.rs b/libs/binder/rust/src/native.rs index 5557168055..b248f5eb28 100644 --- a/libs/binder/rust/src/native.rs +++ b/libs/binder/rust/src/native.rs @@ -42,7 +42,7 @@ pub struct Binder<T: Remotable> { rust_object: *mut T, } -/// # Safety +/// Safety: /// /// A `Binder<T>` is a pair of unique owning pointers to two values: /// * a C++ ABBinder which the C++ API guarantees can be passed between threads @@ -54,7 +54,7 @@ pub struct Binder<T: Remotable> { /// to how `Box<T>` is `Send` if `T` is `Send`. unsafe impl<T: Remotable> Send for Binder<T> {} -/// # Safety +/// Safety: /// /// A `Binder<T>` is a pair of unique owning pointers to two values: /// * a C++ ABBinder which is thread-safe, i.e. `Send + Sync` @@ -89,15 +89,13 @@ impl<T: Remotable> Binder<T> { pub fn new_with_stability(rust_object: T, stability: Stability) -> Binder<T> { let class = T::get_class(); let rust_object = Box::into_raw(Box::new(rust_object)); - let ibinder = unsafe { - // Safety: `AIBinder_new` expects a valid class pointer (which we - // initialize via `get_class`), and an arbitrary pointer - // argument. The caller owns the returned `AIBinder` pointer, which - // is a strong reference to a `BBinder`. This reference should be - // decremented via `AIBinder_decStrong` when the reference lifetime - // ends. - sys::AIBinder_new(class.into(), rust_object as *mut c_void) - }; + // Safety: `AIBinder_new` expects a valid class pointer (which we + // initialize via `get_class`), and an arbitrary pointer + // argument. The caller owns the returned `AIBinder` pointer, which + // is a strong reference to a `BBinder`. This reference should be + // decremented via `AIBinder_decStrong` when the reference lifetime + // ends. + let ibinder = unsafe { sys::AIBinder_new(class.into(), rust_object as *mut c_void) }; let mut binder = Binder { ibinder, rust_object }; binder.mark_stability(stability); binder @@ -176,15 +174,14 @@ impl<T: Remotable> Binder<T> { /// } /// # } pub fn set_extension(&mut self, extension: &mut SpIBinder) -> Result<()> { - let status = unsafe { - // Safety: `AIBinder_setExtension` expects two valid, mutable - // `AIBinder` pointers. We are guaranteed that both `self` and - // `extension` contain valid `AIBinder` pointers, because they - // cannot be initialized without a valid - // pointer. `AIBinder_setExtension` does not take ownership of - // either parameter. - sys::AIBinder_setExtension(self.as_native_mut(), extension.as_native_mut()) - }; + let status = + // Safety: `AIBinder_setExtension` expects two valid, mutable + // `AIBinder` pointers. We are guaranteed that both `self` and + // `extension` contain valid `AIBinder` pointers, because they + // cannot be initialized without a valid + // pointer. `AIBinder_setExtension` does not take ownership of + // either parameter. + unsafe { sys::AIBinder_setExtension(self.as_native_mut(), extension.as_native_mut()) }; status_result(status) } @@ -199,9 +196,9 @@ impl<T: Remotable> Binder<T> { match stability { Stability::Local => self.mark_local_stability(), Stability::Vintf => { + // Safety: Self always contains a valid `AIBinder` pointer, so + // we can always call this C API safely. unsafe { - // Safety: Self always contains a valid `AIBinder` pointer, so - // we can always call this C API safely. sys::AIBinder_markVintfStability(self.as_native_mut()); } } @@ -212,9 +209,9 @@ impl<T: Remotable> Binder<T> { /// building for android_vendor and system otherwise. #[cfg(android_vendor)] fn mark_local_stability(&mut self) { + // Safety: Self always contains a valid `AIBinder` pointer, so we can + // always call this C API safely. unsafe { - // Safety: Self always contains a valid `AIBinder` pointer, so - // we can always call this C API safely. sys::AIBinder_markVendorStability(self.as_native_mut()); } } @@ -223,9 +220,9 @@ impl<T: Remotable> Binder<T> { /// building for android_vendor and system otherwise. #[cfg(not(android_vendor))] fn mark_local_stability(&mut self) { + // Safety: Self always contains a valid `AIBinder` pointer, so we can + // always call this C API safely. unsafe { - // Safety: Self always contains a valid `AIBinder` pointer, so - // we can always call this C API safely. sys::AIBinder_markSystemStability(self.as_native_mut()); } } @@ -239,13 +236,13 @@ impl<T: Remotable> Interface for Binder<T> { /// remotable object, which will prevent the object from being dropped while /// the `SpIBinder` is alive. fn as_binder(&self) -> SpIBinder { + // Safety: `self.ibinder` is guaranteed to always be a valid pointer + // to an `AIBinder` by the `Binder` constructor. We are creating a + // copy of the `self.ibinder` strong reference, but + // `SpIBinder::from_raw` assumes it receives an owned pointer with + // its own strong reference. We first increment the reference count, + // so that the new `SpIBinder` will be tracked as a new reference. unsafe { - // Safety: `self.ibinder` is guaranteed to always be a valid pointer - // to an `AIBinder` by the `Binder` constructor. We are creating a - // copy of the `self.ibinder` strong reference, but - // `SpIBinder::from_raw` assumes it receives an owned pointer with - // its own strong reference. We first increment the reference count, - // so that the new `SpIBinder` will be tracked as a new reference. sys::AIBinder_incStrong(self.ibinder); SpIBinder::from_raw(self.ibinder).unwrap() } @@ -275,10 +272,20 @@ impl<T: Remotable> InterfaceClassMethods for Binder<T> { reply: *mut sys::AParcel, ) -> status_t { let res = { - let mut reply = BorrowedParcel::from_raw(reply).unwrap(); - let data = BorrowedParcel::from_raw(data as *mut sys::AParcel).unwrap(); - let object = sys::AIBinder_getUserData(binder); - let binder: &T = &*(object as *const T); + // Safety: The caller must give us a parcel pointer which is either + // null or valid at least for the duration of this function call. We + // don't keep the resulting value beyond the function. + let mut reply = unsafe { BorrowedParcel::from_raw(reply).unwrap() }; + // Safety: The caller must give us a parcel pointer which is either + // null or valid at least for the duration of this function call. We + // don't keep the resulting value beyond the function. + let data = unsafe { BorrowedParcel::from_raw(data as *mut sys::AParcel).unwrap() }; + // Safety: Our caller promised that `binder` is a non-null, valid + // pointer to a local `AIBinder`. + let object = unsafe { sys::AIBinder_getUserData(binder) }; + // Safety: Our caller promised that the binder has a `T` pointer in + // its user data. + let binder: &T = unsafe { &*(object as *const T) }; binder.on_transact(code, &data, &mut reply) }; match res { @@ -295,7 +302,9 @@ impl<T: Remotable> InterfaceClassMethods for Binder<T> { /// Must be called with a valid pointer to a `T` object. After this call, /// the pointer will be invalid and should not be dereferenced. unsafe extern "C" fn on_destroy(object: *mut c_void) { - drop(Box::from_raw(object as *mut T)); + // Safety: Our caller promised that `object` is a valid pointer to a + // `T`. + drop(unsafe { Box::from_raw(object as *mut T) }); } /// Called whenever a new, local `AIBinder` object is needed of a specific @@ -320,7 +329,7 @@ impl<T: Remotable> InterfaceClassMethods for Binder<T> { /// Must be called with a non-null, valid pointer to a local `AIBinder` that /// contains a `T` pointer in its user data. fd should be a non-owned file /// descriptor, and args must be an array of null-terminated string - /// poiinters with length num_args. + /// pointers with length num_args. unsafe extern "C" fn on_dump( binder: *mut sys::AIBinder, fd: i32, @@ -330,8 +339,9 @@ impl<T: Remotable> InterfaceClassMethods for Binder<T> { if fd < 0 { return StatusCode::UNEXPECTED_NULL as status_t; } - // We don't own this file, so we need to be careful not to drop it. - let file = ManuallyDrop::new(File::from_raw_fd(fd)); + // Safety: Our caller promised that fd is a file descriptor. We don't + // own this file descriptor, so we need to be careful not to drop it. + let file = unsafe { ManuallyDrop::new(File::from_raw_fd(fd)) }; if args.is_null() && num_args != 0 { return StatusCode::UNEXPECTED_NULL as status_t; @@ -340,14 +350,22 @@ impl<T: Remotable> InterfaceClassMethods for Binder<T> { let args = if args.is_null() || num_args == 0 { vec![] } else { - slice::from_raw_parts(args, num_args as usize) - .iter() - .map(|s| CStr::from_ptr(*s)) - .collect() + // Safety: Our caller promised that `args` is an array of + // null-terminated string pointers with length `num_args`. + unsafe { + slice::from_raw_parts(args, num_args as usize) + .iter() + .map(|s| CStr::from_ptr(*s)) + .collect() + } }; - let object = sys::AIBinder_getUserData(binder); - let binder: &T = &*(object as *const T); + // Safety: Our caller promised that `binder` is a non-null, valid + // pointer to a local `AIBinder`. + let object = unsafe { sys::AIBinder_getUserData(binder) }; + // Safety: Our caller promised that the binder has a `T` pointer in its + // user data. + let binder: &T = unsafe { &*(object as *const T) }; let res = binder.on_dump(&file, &args); match res { @@ -363,11 +381,11 @@ impl<T: Remotable> Drop for Binder<T> { // actually destroys the object, it calls `on_destroy` and we can drop the // `rust_object` then. fn drop(&mut self) { + // Safety: When `self` is dropped, we can no longer access the + // reference, so can decrement the reference count. `self.ibinder` is + // always a valid `AIBinder` pointer, so is valid to pass to + // `AIBinder_decStrong`. unsafe { - // Safety: When `self` is dropped, we can no longer access the - // reference, so can decrement the reference count. `self.ibinder` - // is always a valid `AIBinder` pointer, so is valid to pass to - // `AIBinder_decStrong`. sys::AIBinder_decStrong(self.ibinder); } } @@ -377,14 +395,11 @@ impl<T: Remotable> Deref for Binder<T> { type Target = T; fn deref(&self) -> &Self::Target { - unsafe { - // Safety: While `self` is alive, the reference count of the - // underlying object is > 0 and therefore `on_destroy` cannot be - // called. Therefore while `self` is alive, we know that - // `rust_object` is still a valid pointer to a heap allocated object - // of type `T`. - &*self.rust_object - } + // Safety: While `self` is alive, the reference count of the underlying + // object is > 0 and therefore `on_destroy` cannot be called. Therefore + // while `self` is alive, we know that `rust_object` is still a valid + // pointer to a heap allocated object of type `T`. + unsafe { &*self.rust_object } } } @@ -405,13 +420,10 @@ impl<B: Remotable> TryFrom<SpIBinder> for Binder<B> { if Some(class) != ibinder.get_class() { return Err(StatusCode::BAD_TYPE); } - let userdata = unsafe { - // Safety: `SpIBinder` always holds a valid pointer pointer to an - // `AIBinder`, which we can safely pass to - // `AIBinder_getUserData`. `ibinder` retains ownership of the - // returned pointer. - sys::AIBinder_getUserData(ibinder.as_native_mut()) - }; + // Safety: `SpIBinder` always holds a valid pointer pointer to an + // `AIBinder`, which we can safely pass to `AIBinder_getUserData`. + // `ibinder` retains ownership of the returned pointer. + let userdata = unsafe { sys::AIBinder_getUserData(ibinder.as_native_mut()) }; if userdata.is_null() { return Err(StatusCode::UNEXPECTED_NULL); } @@ -422,12 +434,10 @@ impl<B: Remotable> TryFrom<SpIBinder> for Binder<B> { } } -/// # Safety -/// -/// The constructor for `Binder` guarantees that `self.ibinder` will contain a -/// valid, non-null pointer to an `AIBinder`, so this implementation is type -/// safe. `self.ibinder` will remain valid for the entire lifetime of `self` -/// because we hold a strong reference to the `AIBinder` until `self` is +/// Safety: The constructor for `Binder` guarantees that `self.ibinder` will +/// contain a valid, non-null pointer to an `AIBinder`, so this implementation +/// is type safe. `self.ibinder` will remain valid for the entire lifetime of +/// `self` because we hold a strong reference to the `AIBinder` until `self` is /// dropped. unsafe impl<B: Remotable> AsNative<sys::AIBinder> for Binder<B> { fn as_native(&self) -> *const sys::AIBinder { @@ -447,14 +457,12 @@ unsafe impl<B: Remotable> AsNative<sys::AIBinder> for Binder<B> { /// This function will panic if the identifier contains a 0 byte (NUL). pub fn add_service(identifier: &str, mut binder: SpIBinder) -> Result<()> { let instance = CString::new(identifier).unwrap(); - let status = unsafe { - // Safety: `AServiceManager_addService` expects valid `AIBinder` and C - // string pointers. Caller retains ownership of both - // pointers. `AServiceManager_addService` creates a new strong reference - // and copies the string, so both pointers need only be valid until the - // call returns. - sys::AServiceManager_addService(binder.as_native_mut(), instance.as_ptr()) - }; + let status = + // Safety: `AServiceManager_addService` expects valid `AIBinder` and C + // string pointers. Caller retains ownership of both pointers. + // `AServiceManager_addService` creates a new strong reference and copies + // the string, so both pointers need only be valid until the call returns. + unsafe { sys::AServiceManager_addService(binder.as_native_mut(), instance.as_ptr()) }; status_result(status) } @@ -470,13 +478,12 @@ pub fn add_service(identifier: &str, mut binder: SpIBinder) -> Result<()> { /// This function will panic if the identifier contains a 0 byte (NUL). pub fn register_lazy_service(identifier: &str, mut binder: SpIBinder) -> Result<()> { let instance = CString::new(identifier).unwrap(); + // Safety: `AServiceManager_registerLazyService` expects valid `AIBinder` and C + // string pointers. Caller retains ownership of both + // pointers. `AServiceManager_registerLazyService` creates a new strong reference + // and copies the string, so both pointers need only be valid until the + // call returns. let status = unsafe { - // Safety: `AServiceManager_registerLazyService` expects valid `AIBinder` and C - // string pointers. Caller retains ownership of both - // pointers. `AServiceManager_registerLazyService` creates a new strong reference - // and copies the string, so both pointers need only be valid until the - // call returns. - sys::AServiceManager_registerLazyService(binder.as_native_mut(), instance.as_ptr()) }; status_result(status) @@ -491,10 +498,8 @@ pub fn register_lazy_service(identifier: &str, mut binder: SpIBinder) -> Result< /// /// Consider using [`LazyServiceGuard`] rather than calling this directly. pub fn force_lazy_services_persist(persist: bool) { - unsafe { - // Safety: No borrowing or transfer of ownership occurs here. - sys::AServiceManager_forceLazyServicesPersist(persist) - } + // Safety: No borrowing or transfer of ownership occurs here. + unsafe { sys::AServiceManager_forceLazyServicesPersist(persist) } } /// An RAII object to ensure a process which registers lazy services is not killed. During the @@ -576,8 +581,6 @@ impl Interface for () {} /// Determine whether the current thread is currently executing an incoming /// transaction. pub fn is_handling_transaction() -> bool { - unsafe { - // Safety: This method is always safe to call. - sys::AIBinder_isHandlingTransaction() - } + // Safety: This method is always safe to call. + unsafe { sys::AIBinder_isHandlingTransaction() } } diff --git a/libs/binder/rust/src/parcel.rs b/libs/binder/rust/src/parcel.rs index e4c568eab2..3c615edbc0 100644 --- a/libs/binder/rust/src/parcel.rs +++ b/libs/binder/rust/src/parcel.rs @@ -52,11 +52,8 @@ pub struct Parcel { ptr: NonNull<sys::AParcel>, } -/// # Safety -/// -/// This type guarantees that it owns the AParcel and that all access to -/// the AParcel happens through the Parcel, so it is ok to send across -/// threads. +/// Safety: This type guarantees that it owns the AParcel and that all access to +/// the AParcel happens through the Parcel, so it is ok to send across threads. unsafe impl Send for Parcel {} /// Container for a message (data and object references) that can be sent @@ -73,11 +70,9 @@ pub struct BorrowedParcel<'a> { impl Parcel { /// Create a new empty `Parcel`. pub fn new() -> Parcel { - let ptr = unsafe { - // Safety: If `AParcel_create` succeeds, it always returns - // a valid pointer. If it fails, the process will crash. - sys::AParcel_create() - }; + // Safety: If `AParcel_create` succeeds, it always returns + // a valid pointer. If it fails, the process will crash. + let ptr = unsafe { sys::AParcel_create() }; Self { ptr: NonNull::new(ptr).expect("AParcel_create returned null pointer") } } @@ -171,10 +166,8 @@ impl<'a> BorrowedParcel<'a> { } } -/// # Safety -/// -/// The `Parcel` constructors guarantee that a `Parcel` object will always -/// contain a valid pointer to an `AParcel`. +/// Safety: The `Parcel` constructors guarantee that a `Parcel` object will +/// always contain a valid pointer to an `AParcel`. unsafe impl AsNative<sys::AParcel> for Parcel { fn as_native(&self) -> *const sys::AParcel { self.ptr.as_ptr() @@ -185,10 +178,8 @@ unsafe impl AsNative<sys::AParcel> for Parcel { } } -/// # Safety -/// -/// The `BorrowedParcel` constructors guarantee that a `BorrowedParcel` object -/// will always contain a valid pointer to an `AParcel`. +/// Safety: The `BorrowedParcel` constructors guarantee that a `BorrowedParcel` +/// object will always contain a valid pointer to an `AParcel`. unsafe impl<'a> AsNative<sys::AParcel> for BorrowedParcel<'a> { fn as_native(&self) -> *const sys::AParcel { self.ptr.as_ptr() @@ -203,10 +194,8 @@ unsafe impl<'a> AsNative<sys::AParcel> for BorrowedParcel<'a> { impl<'a> BorrowedParcel<'a> { /// Data written to parcelable is zero'd before being deleted or reallocated. pub fn mark_sensitive(&mut self) { - unsafe { - // Safety: guaranteed to have a parcel object, and this method never fails - sys::AParcel_markSensitive(self.as_native()) - } + // Safety: guaranteed to have a parcel object, and this method never fails + unsafe { sys::AParcel_markSensitive(self.as_native()) } } /// Write a type that implements [`Serialize`] to the parcel. @@ -265,11 +254,15 @@ impl<'a> BorrowedParcel<'a> { f(&mut subparcel)?; } let end = self.get_data_position(); + // Safety: start is less than the current size of the parcel data + // buffer, because we just got it with `get_data_position`. unsafe { self.set_data_position(start)?; } assert!(end >= start); self.write(&(end - start))?; + // Safety: end is less than the current size of the parcel data + // buffer, because we just got it with `get_data_position`. unsafe { self.set_data_position(end)?; } @@ -278,20 +271,16 @@ impl<'a> BorrowedParcel<'a> { /// Returns the current position in the parcel data. pub fn get_data_position(&self) -> i32 { - unsafe { - // Safety: `BorrowedParcel` always contains a valid pointer to an - // `AParcel`, and this call is otherwise safe. - sys::AParcel_getDataPosition(self.as_native()) - } + // Safety: `BorrowedParcel` always contains a valid pointer to an + // `AParcel`, and this call is otherwise safe. + unsafe { sys::AParcel_getDataPosition(self.as_native()) } } /// Returns the total size of the parcel. pub fn get_data_size(&self) -> i32 { - unsafe { - // Safety: `BorrowedParcel` always contains a valid pointer to an - // `AParcel`, and this call is otherwise safe. - sys::AParcel_getDataSize(self.as_native()) - } + // Safety: `BorrowedParcel` always contains a valid pointer to an + // `AParcel`, and this call is otherwise safe. + unsafe { sys::AParcel_getDataSize(self.as_native()) } } /// Move the current read/write position in the parcel. @@ -304,7 +293,9 @@ impl<'a> BorrowedParcel<'a> { /// accesses are bounds checked, this call is still safe, but we can't rely /// on that. pub unsafe fn set_data_position(&self, pos: i32) -> Result<()> { - status_result(sys::AParcel_setDataPosition(self.as_native(), pos)) + // Safety: `BorrowedParcel` always contains a valid pointer to an + // `AParcel`, and the caller guarantees that `pos` is within bounds. + status_result(unsafe { sys::AParcel_setDataPosition(self.as_native(), pos) }) } /// Append a subset of another parcel. @@ -317,10 +308,10 @@ impl<'a> BorrowedParcel<'a> { start: i32, size: i32, ) -> Result<()> { + // Safety: `Parcel::appendFrom` from C++ checks that `start` + // and `size` are in bounds, and returns an error otherwise. + // Both `self` and `other` always contain valid pointers. let status = unsafe { - // Safety: `Parcel::appendFrom` from C++ checks that `start` - // and `size` are in bounds, and returns an error otherwise. - // Both `self` and `other` always contain valid pointers. sys::AParcel_appendFrom(other.as_native(), self.as_native_mut(), start, size) }; status_result(status) @@ -418,7 +409,9 @@ impl Parcel { /// accesses are bounds checked, this call is still safe, but we can't rely /// on that. pub unsafe fn set_data_position(&self, pos: i32) -> Result<()> { - self.borrowed_ref().set_data_position(pos) + // Safety: We have the same safety requirements as + // `BorrowedParcel::set_data_position`. + unsafe { self.borrowed_ref().set_data_position(pos) } } /// Append a subset of another parcel. @@ -461,7 +454,7 @@ impl<'a> BorrowedParcel<'a> { /// and call a closure with the sub-parcel as its parameter. /// The closure can keep reading data from the sub-parcel /// until it runs out of input data. The closure is responsible - /// for calling [`ReadableSubParcel::has_more_data`] to check for + /// for calling `ReadableSubParcel::has_more_data` to check for /// more data before every read, at least until Rust generators /// are stabilized. /// After the closure returns, skip to the end of the current @@ -504,7 +497,10 @@ impl<'a> BorrowedParcel<'a> { f(subparcel)?; // Advance the data position to the actual end, - // in case the closure read less data than was available + // in case the closure read less data than was available. + // + // Safety: end must be less than the current size of the parcel, because + // we checked above against `get_data_size`. unsafe { self.set_data_position(end)?; } @@ -595,7 +591,7 @@ impl Parcel { /// and call a closure with the sub-parcel as its parameter. /// The closure can keep reading data from the sub-parcel /// until it runs out of input data. The closure is responsible - /// for calling [`ReadableSubParcel::has_more_data`] to check for + /// for calling `ReadableSubParcel::has_more_data` to check for /// more data before every read, at least until Rust generators /// are stabilized. /// After the closure returns, skip to the end of the current @@ -649,17 +645,17 @@ impl Parcel { // Internal APIs impl<'a> BorrowedParcel<'a> { pub(crate) fn write_binder(&mut self, binder: Option<&SpIBinder>) -> Result<()> { + // Safety: `BorrowedParcel` always contains a valid pointer to an + // `AParcel`. `AsNative` for `Option<SpIBinder`> will either return + // null or a valid pointer to an `AIBinder`, both of which are + // valid, safe inputs to `AParcel_writeStrongBinder`. + // + // This call does not take ownership of the binder. However, it does + // require a mutable pointer, which we cannot extract from an + // immutable reference, so we clone the binder, incrementing the + // refcount before the call. The refcount will be immediately + // decremented when this temporary is dropped. unsafe { - // Safety: `BorrowedParcel` always contains a valid pointer to an - // `AParcel`. `AsNative` for `Option<SpIBinder`> will either return - // null or a valid pointer to an `AIBinder`, both of which are - // valid, safe inputs to `AParcel_writeStrongBinder`. - // - // This call does not take ownership of the binder. However, it does - // require a mutable pointer, which we cannot extract from an - // immutable reference, so we clone the binder, incrementing the - // refcount before the call. The refcount will be immediately - // decremented when this temporary is dropped. status_result(sys::AParcel_writeStrongBinder( self.as_native_mut(), binder.cloned().as_native_mut(), @@ -669,33 +665,28 @@ impl<'a> BorrowedParcel<'a> { pub(crate) fn read_binder(&self) -> Result<Option<SpIBinder>> { let mut binder = ptr::null_mut(); - let status = unsafe { - // Safety: `BorrowedParcel` always contains a valid pointer to an - // `AParcel`. We pass a valid, mutable out pointer to the `binder` - // parameter. After this call, `binder` will be either null or a - // valid pointer to an `AIBinder` owned by the caller. - sys::AParcel_readStrongBinder(self.as_native(), &mut binder) - }; + // Safety: `BorrowedParcel` always contains a valid pointer to an + // `AParcel`. We pass a valid, mutable out pointer to the `binder` + // parameter. After this call, `binder` will be either null or a + // valid pointer to an `AIBinder` owned by the caller. + let status = unsafe { sys::AParcel_readStrongBinder(self.as_native(), &mut binder) }; status_result(status)?; - Ok(unsafe { - // Safety: `binder` is either null or a valid, owned pointer at this - // point, so can be safely passed to `SpIBinder::from_raw`. - SpIBinder::from_raw(binder) - }) + // Safety: `binder` is either null or a valid, owned pointer at this + // point, so can be safely passed to `SpIBinder::from_raw`. + Ok(unsafe { SpIBinder::from_raw(binder) }) } } impl Drop for Parcel { fn drop(&mut self) { // Run the C++ Parcel complete object destructor - unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`. Since we own the parcel, we can safely delete it - // here. - sys::AParcel_delete(self.ptr.as_ptr()) - } + // + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`. Since we own the parcel, we can safely delete it + // here. + unsafe { sys::AParcel_delete(self.ptr.as_ptr()) } } } @@ -732,6 +723,8 @@ fn test_read_write() { parcel.write(&1i32).unwrap(); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { parcel.set_data_position(start).unwrap(); } @@ -748,6 +741,8 @@ fn test_read_data() { parcel.write(&b"Hello, Binder!\0"[..]).unwrap(); // Skip over string length + // SAFETY: str_start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(str_start).is_ok()); } @@ -756,42 +751,56 @@ fn test_read_data() { assert!(parcel.read::<bool>().unwrap()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert_eq!(parcel.read::<i8>().unwrap(), 72i8); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert_eq!(parcel.read::<u16>().unwrap(), 25928); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert_eq!(parcel.read::<i32>().unwrap(), 1819043144); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert_eq!(parcel.read::<u32>().unwrap(), 1819043144); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert_eq!(parcel.read::<i64>().unwrap(), 4764857262830019912); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert_eq!(parcel.read::<u64>().unwrap(), 4764857262830019912); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -799,6 +808,8 @@ fn test_read_data() { assert_eq!(parcel.read::<f32>().unwrap(), 1143139100000000000000000000.0); assert_eq!(parcel.read::<f32>().unwrap(), 40.043392); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -806,6 +817,8 @@ fn test_read_data() { assert_eq!(parcel.read::<f64>().unwrap(), 34732488246.197815); // Skip back to before the string length + // SAFETY: str_start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(str_start).is_ok()); } @@ -819,15 +832,21 @@ fn test_utf8_utf16_conversions() { let start = parcel.get_data_position(); assert!(parcel.write("Hello, Binder!").is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert_eq!(parcel.read::<Option<String>>().unwrap().unwrap(), "Hello, Binder!",); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert!(parcel.write("Embedded null \0 inside a string").is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -835,6 +854,8 @@ fn test_utf8_utf16_conversions() { parcel.read::<Option<String>>().unwrap().unwrap(), "Embedded null \0 inside a string", ); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -849,6 +870,8 @@ fn test_utf8_utf16_conversions() { let s3 = "Some more text here."; assert!(parcel.write(&[s1, s2, s3][..]).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -874,6 +897,8 @@ fn test_sized_write() { assert_eq!(parcel.get_data_position(), start + expected_len); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { parcel.set_data_position(start).unwrap(); } @@ -893,6 +918,8 @@ fn test_append_from() { assert_eq!(4, parcel2.get_data_size()); assert_eq!(Ok(()), parcel2.append_all_from(&parcel1)); assert_eq!(8, parcel2.get_data_size()); + // SAFETY: 0 is less than the current size of the parcel data buffer, because the parcel is not + // empty. unsafe { parcel2.set_data_position(0).unwrap(); } @@ -903,6 +930,8 @@ fn test_append_from() { assert_eq!(Ok(()), parcel2.append_from(&parcel1, 0, 2)); assert_eq!(Ok(()), parcel2.append_from(&parcel1, 2, 2)); assert_eq!(4, parcel2.get_data_size()); + // SAFETY: 0 is less than the current size of the parcel data buffer, because the parcel is not + // empty. unsafe { parcel2.set_data_position(0).unwrap(); } @@ -911,6 +940,8 @@ fn test_append_from() { let mut parcel2 = Parcel::new(); assert_eq!(Ok(()), parcel2.append_from(&parcel1, 0, 2)); assert_eq!(2, parcel2.get_data_size()); + // SAFETY: 0 is less than the current size of the parcel data buffer, because the parcel is not + // empty. unsafe { parcel2.set_data_position(0).unwrap(); } diff --git a/libs/binder/rust/src/parcel/file_descriptor.rs b/libs/binder/rust/src/parcel/file_descriptor.rs index 7fe37f3b68..5c688fa71b 100644 --- a/libs/binder/rust/src/parcel/file_descriptor.rs +++ b/libs/binder/rust/src/parcel/file_descriptor.rs @@ -73,14 +73,12 @@ impl Eq for ParcelFileDescriptor {} impl Serialize for ParcelFileDescriptor { fn serialize(&self, parcel: &mut BorrowedParcel<'_>) -> Result<()> { let fd = self.0.as_raw_fd(); - let status = unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`. Likewise, `ParcelFileDescriptor` always contains a - // valid file, so we can borrow a valid file - // descriptor. `AParcel_writeParcelFileDescriptor` does NOT take - // ownership of the fd, so we need not duplicate it first. - sys::AParcel_writeParcelFileDescriptor(parcel.as_native_mut(), fd) - }; + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`. Likewise, `ParcelFileDescriptor` always contains a + // valid file, so we can borrow a valid file + // descriptor. `AParcel_writeParcelFileDescriptor` does NOT take + // ownership of the fd, so we need not duplicate it first. + let status = unsafe { sys::AParcel_writeParcelFileDescriptor(parcel.as_native_mut(), fd) }; status_result(status) } } @@ -92,13 +90,12 @@ impl SerializeOption for ParcelFileDescriptor { if let Some(f) = this { f.serialize(parcel) } else { - let status = unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`. `AParcel_writeParcelFileDescriptor` accepts the - // value `-1` as the file descriptor to signify serializing a - // null file descriptor. - sys::AParcel_writeParcelFileDescriptor(parcel.as_native_mut(), -1i32) - }; + let status = + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`. `AParcel_writeParcelFileDescriptor` accepts the + // value `-1` as the file descriptor to signify serializing a + // null file descriptor. + unsafe { sys::AParcel_writeParcelFileDescriptor(parcel.as_native_mut(), -1i32) }; status_result(status) } } @@ -107,25 +104,23 @@ impl SerializeOption for ParcelFileDescriptor { impl DeserializeOption for ParcelFileDescriptor { fn deserialize_option(parcel: &BorrowedParcel<'_>) -> Result<Option<Self>> { let mut fd = -1i32; + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`. We pass a valid mutable pointer to an i32, which + // `AParcel_readParcelFileDescriptor` assigns the valid file + // descriptor into, or `-1` if deserializing a null file + // descriptor. The read function passes ownership of the file + // descriptor to its caller if it was non-null, so we must take + // ownership of the file and ensure that it is eventually closed. unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`. We pass a valid mutable pointer to an i32, which - // `AParcel_readParcelFileDescriptor` assigns the valid file - // descriptor into, or `-1` if deserializing a null file - // descriptor. The read function passes ownership of the file - // descriptor to its caller if it was non-null, so we must take - // ownership of the file and ensure that it is eventually closed. status_result(sys::AParcel_readParcelFileDescriptor(parcel.as_native(), &mut fd))?; } if fd < 0 { Ok(None) } else { - let file = unsafe { - // Safety: At this point, we know that the file descriptor was - // not -1, so must be a valid, owned file descriptor which we - // can safely turn into a `File`. - File::from_raw_fd(fd) - }; + // Safety: At this point, we know that the file descriptor was + // not -1, so must be a valid, owned file descriptor which we + // can safely turn into a `File`. + let file = unsafe { File::from_raw_fd(fd) }; Ok(Some(ParcelFileDescriptor::new(file))) } } diff --git a/libs/binder/rust/src/parcel/parcelable.rs b/libs/binder/rust/src/parcel/parcelable.rs index 5d8c11cf94..9008a3cc0e 100644 --- a/libs/binder/rust/src/parcel/parcelable.rs +++ b/libs/binder/rust/src/parcel/parcelable.rs @@ -50,14 +50,14 @@ pub trait Parcelable { fn read_from_parcel(&mut self, parcel: &BorrowedParcel<'_>) -> Result<()>; } -/// A struct whose instances can be written to a [`Parcel`]. +/// A struct whose instances can be written to a [`crate::parcel::Parcel`]. // Might be able to hook this up as a serde backend in the future? pub trait Serialize { - /// Serialize this instance into the given [`Parcel`]. + /// Serialize this instance into the given [`crate::parcel::Parcel`]. fn serialize(&self, parcel: &mut BorrowedParcel<'_>) -> Result<()>; } -/// A struct whose instances can be restored from a [`Parcel`]. +/// A struct whose instances can be restored from a [`crate::parcel::Parcel`]. // Might be able to hook this up as a serde backend in the future? pub trait Deserialize: Sized { /// Type for the uninitialized value of this type. Will be either `Self` @@ -80,10 +80,10 @@ pub trait Deserialize: Sized { /// Convert an initialized value of type `Self` into `Self::UninitType`. fn from_init(value: Self) -> Self::UninitType; - /// Deserialize an instance from the given [`Parcel`]. + /// Deserialize an instance from the given [`crate::parcel::Parcel`]. fn deserialize(parcel: &BorrowedParcel<'_>) -> Result<Self>; - /// Deserialize an instance from the given [`Parcel`] onto the + /// Deserialize an instance from the given [`crate::parcel::Parcel`] onto the /// current object. This operation will overwrite the old value /// partially or completely, depending on how much data is available. fn deserialize_from(&mut self, parcel: &BorrowedParcel<'_>) -> Result<()> { @@ -102,8 +102,8 @@ pub trait Deserialize: Sized { pub trait SerializeArray: Serialize + Sized { /// Serialize an array of this type into the given parcel. fn serialize_array(slice: &[Self], parcel: &mut BorrowedParcel<'_>) -> Result<()> { + // Safety: Safe FFI, slice will always be a safe pointer to pass. let res = unsafe { - // Safety: Safe FFI, slice will always be a safe pointer to pass. sys::AParcel_writeParcelableArray( parcel.as_native_mut(), slice.as_ptr() as *const c_void, @@ -117,7 +117,9 @@ pub trait SerializeArray: Serialize + Sized { /// Callback to serialize an element of a generic parcelable array. /// -/// Safety: We are relying on binder_ndk to not overrun our slice. As long as it +/// # Safety +/// +/// We are relying on binder_ndk to not overrun our slice. As long as it /// doesn't provide an index larger than the length of the original slice in /// serialize_array, this operation is safe. The index provided is zero-based. unsafe extern "C" fn serialize_element<T: Serialize>( @@ -125,9 +127,14 @@ unsafe extern "C" fn serialize_element<T: Serialize>( array: *const c_void, index: usize, ) -> status_t { - let slice: &[T] = slice::from_raw_parts(array.cast(), index + 1); - - let mut parcel = match BorrowedParcel::from_raw(parcel) { + // Safety: The caller guarantees that `array` is a valid pointer of the + // appropriate type. + let slice: &[T] = unsafe { slice::from_raw_parts(array.cast(), index + 1) }; + + // Safety: The caller must give us a parcel pointer which is either null or + // valid at least for the duration of this function call. We don't keep the + // resulting value beyond the function. + let mut parcel = match unsafe { BorrowedParcel::from_raw(parcel) } { None => return StatusCode::UNEXPECTED_NULL as status_t, Some(p) => p, }; @@ -142,9 +149,9 @@ pub trait DeserializeArray: Deserialize { /// Deserialize an array of type from the given parcel. fn deserialize_array(parcel: &BorrowedParcel<'_>) -> Result<Option<Vec<Self>>> { let mut vec: Option<Vec<Self::UninitType>> = None; + // Safety: Safe FFI, vec is the correct opaque type expected by + // allocate_vec and deserialize_element. let res = unsafe { - // Safety: Safe FFI, vec is the correct opaque type expected by - // allocate_vec and deserialize_element. sys::AParcel_readParcelableArray( parcel.as_native(), &mut vec as *mut _ as *mut c_void, @@ -153,21 +160,21 @@ pub trait DeserializeArray: Deserialize { ) }; status_result(res)?; - let vec: Option<Vec<Self>> = unsafe { - // Safety: We are assuming that the NDK correctly initialized every - // element of the vector by now, so we know that all the - // UninitTypes are now properly initialized. We can transmute from - // Vec<T::UninitType> to Vec<T> because T::UninitType has the same - // alignment and size as T, so the pointer to the vector allocation - // will be compatible. - mem::transmute(vec) - }; + // Safety: We are assuming that the NDK correctly initialized every + // element of the vector by now, so we know that all the + // UninitTypes are now properly initialized. We can transmute from + // Vec<T::UninitType> to Vec<T> because T::UninitType has the same + // alignment and size as T, so the pointer to the vector allocation + // will be compatible. + let vec: Option<Vec<Self>> = unsafe { mem::transmute(vec) }; Ok(vec) } } /// Callback to deserialize a parcelable element. /// +/// # Safety +/// /// The opaque array data pointer must be a mutable pointer to an /// `Option<Vec<T::UninitType>>` with at least enough elements for `index` to be valid /// (zero-based). @@ -176,13 +183,18 @@ unsafe extern "C" fn deserialize_element<T: Deserialize>( array: *mut c_void, index: usize, ) -> status_t { - let vec = &mut *(array as *mut Option<Vec<T::UninitType>>); + // Safety: The caller guarantees that `array` is a valid pointer of the + // appropriate type. + let vec = unsafe { &mut *(array as *mut Option<Vec<T::UninitType>>) }; let vec = match vec { Some(v) => v, None => return StatusCode::BAD_INDEX as status_t, }; - let parcel = match BorrowedParcel::from_raw(parcel as *mut _) { + // Safety: The caller must give us a parcel pointer which is either null or + // valid at least for the duration of this function call. We don't keep the + // resulting value beyond the function. + let parcel = match unsafe { BorrowedParcel::from_raw(parcel as *mut _) } { None => return StatusCode::UNEXPECTED_NULL as status_t, Some(p) => p, }; @@ -254,16 +266,21 @@ pub trait DeserializeOption: Deserialize { /// /// The opaque data pointer passed to the array read function must be a mutable /// pointer to an `Option<Vec<T::UninitType>>`. `buffer` will be assigned a mutable pointer -/// to the allocated vector data if this function returns true. +/// to the allocated vector data if this function returns true. `buffer` must be a valid pointer. unsafe extern "C" fn allocate_vec_with_buffer<T: Deserialize>( data: *mut c_void, len: i32, buffer: *mut *mut T, ) -> bool { - let res = allocate_vec::<T>(data, len); - let vec = &mut *(data as *mut Option<Vec<T::UninitType>>); + // Safety: We have the same safety requirements as `allocate_vec` for `data`. + let res = unsafe { allocate_vec::<T>(data, len) }; + // Safety: The caller guarantees that `data` is a valid mutable pointer to the appropriate type. + let vec = unsafe { &mut *(data as *mut Option<Vec<T::UninitType>>) }; if let Some(new_vec) = vec { - *buffer = new_vec.as_mut_ptr() as *mut T; + // Safety: The caller guarantees that `buffer` is a valid pointer. + unsafe { + *buffer = new_vec.as_mut_ptr() as *mut T; + } } res } @@ -275,7 +292,8 @@ unsafe extern "C" fn allocate_vec_with_buffer<T: Deserialize>( /// The opaque data pointer passed to the array read function must be a mutable /// pointer to an `Option<Vec<T::UninitType>>`. unsafe extern "C" fn allocate_vec<T: Deserialize>(data: *mut c_void, len: i32) -> bool { - let vec = &mut *(data as *mut Option<Vec<T::UninitType>>); + // Safety: The caller guarantees that `data` is a valid mutable pointer to the appropriate type. + let vec = unsafe { &mut *(data as *mut Option<Vec<T::UninitType>>) }; if len < 0 { *vec = None; return true; @@ -286,7 +304,10 @@ unsafe extern "C" fn allocate_vec<T: Deserialize>(data: *mut c_void, len: i32) - let mut new_vec: Vec<T::UninitType> = Vec::with_capacity(len as usize); new_vec.resize_with(len as usize, T::uninit); - ptr::write(vec, Some(new_vec)); + // Safety: The caller guarantees that vec is a valid mutable pointer to the appropriate type. + unsafe { + ptr::write(vec, Some(new_vec)); + } true } @@ -305,21 +326,21 @@ unsafe fn vec_assume_init<T: Deserialize>(vec: Vec<T::UninitType>) -> Vec<T> { // Assert at compile time that `T` and `T::UninitType` have the same size and alignment. let _ = T::ASSERT_UNINIT_SIZE_AND_ALIGNMENT; - // We can convert from Vec<T::UninitType> to Vec<T> because T::UninitType - // has the same alignment and size as T, so the pointer to the vector - // allocation will be compatible. let mut vec = ManuallyDrop::new(vec); - Vec::from_raw_parts(vec.as_mut_ptr().cast(), vec.len(), vec.capacity()) + // Safety: We can convert from Vec<T::UninitType> to Vec<T> because + // T::UninitType has the same alignment and size as T, so the pointer to the + // vector allocation will be compatible. + unsafe { Vec::from_raw_parts(vec.as_mut_ptr().cast(), vec.len(), vec.capacity()) } } macro_rules! impl_parcelable { {Serialize, $ty:ty, $write_fn:path} => { impl Serialize for $ty { fn serialize(&self, parcel: &mut BorrowedParcel<'_>) -> Result<()> { + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`, and any `$ty` literal value is safe to pass to + // `$write_fn`. unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`, and any `$ty` literal value is safe to pass to - // `$write_fn`. status_result($write_fn(parcel.as_native_mut(), *self)) } } @@ -333,11 +354,11 @@ macro_rules! impl_parcelable { fn from_init(value: Self) -> Self::UninitType { value } fn deserialize(parcel: &BorrowedParcel<'_>) -> Result<Self> { let mut val = Self::default(); + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`. We pass a valid, mutable pointer to `val`, a + // literal of type `$ty`, and `$read_fn` will write the + // value read into `val` if successful unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`. We pass a valid, mutable pointer to `val`, a - // literal of type `$ty`, and `$read_fn` will write the - // value read into `val` if successful status_result($read_fn(parcel.as_native(), &mut val))? }; Ok(val) @@ -348,13 +369,13 @@ macro_rules! impl_parcelable { {SerializeArray, $ty:ty, $write_array_fn:path} => { impl SerializeArray for $ty { fn serialize_array(slice: &[Self], parcel: &mut BorrowedParcel<'_>) -> Result<()> { + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`. If the slice is > 0 length, `slice.as_ptr()` + // will be a valid pointer to an array of elements of type + // `$ty`. If the slice length is 0, `slice.as_ptr()` may be + // dangling, but this is safe since the pointer is not + // dereferenced if the length parameter is 0. let status = unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`. If the slice is > 0 length, `slice.as_ptr()` - // will be a valid pointer to an array of elements of type - // `$ty`. If the slice length is 0, `slice.as_ptr()` may be - // dangling, but this is safe since the pointer is not - // dereferenced if the length parameter is 0. $write_array_fn( parcel.as_native_mut(), slice.as_ptr(), @@ -373,11 +394,11 @@ macro_rules! impl_parcelable { impl DeserializeArray for $ty { fn deserialize_array(parcel: &BorrowedParcel<'_>) -> Result<Option<Vec<Self>>> { let mut vec: Option<Vec<Self::UninitType>> = None; + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`. `allocate_vec<T>` expects the opaque pointer to + // be of type `*mut Option<Vec<T::UninitType>>`, so `&mut vec` is + // correct for it. let status = unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`. `allocate_vec<T>` expects the opaque pointer to - // be of type `*mut Option<Vec<T::UninitType>>`, so `&mut vec` is - // correct for it. $read_array_fn( parcel.as_native(), &mut vec as *mut _ as *mut c_void, @@ -385,11 +406,11 @@ macro_rules! impl_parcelable { ) }; status_result(status)?; + // Safety: We are assuming that the NDK correctly + // initialized every element of the vector by now, so we + // know that all the UninitTypes are now properly + // initialized. let vec: Option<Vec<Self>> = unsafe { - // Safety: We are assuming that the NDK correctly - // initialized every element of the vector by now, so we - // know that all the UninitTypes are now properly - // initialized. vec.map(|vec| vec_assume_init(vec)) }; Ok(vec) @@ -479,13 +500,13 @@ impl Deserialize for u8 { impl SerializeArray for u8 { fn serialize_array(slice: &[Self], parcel: &mut BorrowedParcel<'_>) -> Result<()> { + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`. If the slice is > 0 length, `slice.as_ptr()` will be a + // valid pointer to an array of elements of type `$ty`. If the slice + // length is 0, `slice.as_ptr()` may be dangling, but this is safe + // since the pointer is not dereferenced if the length parameter is + // 0. let status = unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`. If the slice is > 0 length, `slice.as_ptr()` will be a - // valid pointer to an array of elements of type `$ty`. If the slice - // length is 0, `slice.as_ptr()` may be dangling, but this is safe - // since the pointer is not dereferenced if the length parameter is - // 0. sys::AParcel_writeByteArray( parcel.as_native_mut(), slice.as_ptr() as *const i8, @@ -518,13 +539,13 @@ impl Deserialize for i16 { impl SerializeArray for i16 { fn serialize_array(slice: &[Self], parcel: &mut BorrowedParcel<'_>) -> Result<()> { + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`. If the slice is > 0 length, `slice.as_ptr()` will be a + // valid pointer to an array of elements of type `$ty`. If the slice + // length is 0, `slice.as_ptr()` may be dangling, but this is safe + // since the pointer is not dereferenced if the length parameter is + // 0. let status = unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`. If the slice is > 0 length, `slice.as_ptr()` will be a - // valid pointer to an array of elements of type `$ty`. If the slice - // length is 0, `slice.as_ptr()` may be dangling, but this is safe - // since the pointer is not dereferenced if the length parameter is - // 0. sys::AParcel_writeCharArray( parcel.as_native_mut(), slice.as_ptr() as *const u16, @@ -538,22 +559,22 @@ impl SerializeArray for i16 { impl SerializeOption for str { fn serialize_option(this: Option<&Self>, parcel: &mut BorrowedParcel<'_>) -> Result<()> { match this { + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`. If the string pointer is null, + // `AParcel_writeString` requires that the length is -1 to + // indicate that we want to serialize a null string. None => unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`. If the string pointer is null, - // `AParcel_writeString` requires that the length is -1 to - // indicate that we want to serialize a null string. status_result(sys::AParcel_writeString(parcel.as_native_mut(), ptr::null(), -1)) }, + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`. `AParcel_writeString` assumes that we pass a utf-8 + // string pointer of `length` bytes, which is what str in Rust + // is. The docstring for `AParcel_writeString` says that the + // string input should be null-terminated, but it doesn't + // actually rely on that fact in the code. If this ever becomes + // necessary, we will need to null-terminate the str buffer + // before sending it. Some(s) => unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`. `AParcel_writeString` assumes that we pass a utf-8 - // string pointer of `length` bytes, which is what str in Rust - // is. The docstring for `AParcel_writeString` says that the - // string input should be null-terminated, but it doesn't - // actually rely on that fact in the code. If this ever becomes - // necessary, we will need to null-terminate the str buffer - // before sending it. status_result(sys::AParcel_writeString( parcel.as_native_mut(), s.as_ptr() as *const c_char, @@ -597,11 +618,11 @@ impl Deserialize for Option<String> { fn deserialize(parcel: &BorrowedParcel<'_>) -> Result<Self> { let mut vec: Option<Vec<u8>> = None; + // Safety: `Parcel` always contains a valid pointer to an `AParcel`. + // `Option<Vec<u8>>` is equivalent to the expected `Option<Vec<i8>>` + // for `allocate_vec`, so `vec` is safe to pass as the opaque data + // pointer on platforms where char is signed. let status = unsafe { - // Safety: `Parcel` always contains a valid pointer to an `AParcel`. - // `Option<Vec<u8>>` is equivalent to the expected `Option<Vec<i8>>` - // for `allocate_vec`, so `vec` is safe to pass as the opaque data - // pointer on platforms where char is signed. sys::AParcel_readString( parcel.as_native(), &mut vec as *mut _ as *mut c_void, @@ -751,11 +772,11 @@ impl Deserialize for Stability { impl Serialize for Status { fn serialize(&self, parcel: &mut BorrowedParcel<'_>) -> Result<()> { + // Safety: `Parcel` always contains a valid pointer to an `AParcel` + // and `Status` always contains a valid pointer to an `AStatus`, so + // both parameters are valid and safe. This call does not take + // ownership of either of its parameters. unsafe { - // Safety: `Parcel` always contains a valid pointer to an `AParcel` - // and `Status` always contains a valid pointer to an `AStatus`, so - // both parameters are valid and safe. This call does not take - // ownership of either of its parameters. status_result(sys::AParcel_writeStatusHeader(parcel.as_native_mut(), self.as_native())) } } @@ -772,21 +793,18 @@ impl Deserialize for Status { fn deserialize(parcel: &BorrowedParcel<'_>) -> Result<Self> { let mut status_ptr = ptr::null_mut(); - let ret_status = unsafe { - // Safety: `Parcel` always contains a valid pointer to an - // `AParcel`. We pass a mutable out pointer which will be - // assigned a valid `AStatus` pointer if the function returns - // status OK. This function passes ownership of the status - // pointer to the caller, if it was assigned. - sys::AParcel_readStatusHeader(parcel.as_native(), &mut status_ptr) - }; + let ret_status = + // Safety: `Parcel` always contains a valid pointer to an + // `AParcel`. We pass a mutable out pointer which will be + // assigned a valid `AStatus` pointer if the function returns + // status OK. This function passes ownership of the status + // pointer to the caller, if it was assigned. + unsafe { sys::AParcel_readStatusHeader(parcel.as_native(), &mut status_ptr) }; status_result(ret_status)?; - Ok(unsafe { - // Safety: At this point, the return status of the read call was ok, - // so we know that `status_ptr` is a valid, owned pointer to an - // `AStatus`, from which we can safely construct a `Status` object. - Status::from_ptr(status_ptr) - }) + // Safety: At this point, the return status of the read call was ok, + // so we know that `status_ptr` is a valid, owned pointer to an + // `AStatus`, from which we can safely construct a `Status` object. + Ok(unsafe { Status::from_ptr(status_ptr) }) } } @@ -880,7 +898,6 @@ impl<T: DeserializeOption> Deserialize for Option<T> { /// `Serialize`, `SerializeArray` and `SerializeOption` for /// structured parcelables. The target type must implement the /// `Parcelable` trait. -/// ``` #[macro_export] macro_rules! impl_serialize_for_parcelable { ($parcelable:ident) => { @@ -1070,6 +1087,8 @@ mod tests { assert!(custom.serialize(&mut parcel.borrowed()).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1092,6 +1111,8 @@ mod tests { assert!(bools.serialize(&mut parcel.borrowed()).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1101,6 +1122,8 @@ mod tests { assert_eq!(parcel.read::<u32>().unwrap(), 0); assert_eq!(parcel.read::<u32>().unwrap(), 0); assert_eq!(parcel.read::<u32>().unwrap(), 1); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1116,12 +1139,17 @@ mod tests { assert!(parcel.write(&u8s[..]).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert_eq!(parcel.read::<u32>().unwrap(), 4); // 4 items assert_eq!(parcel.read::<u32>().unwrap(), 0x752aff65); // bytes + + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1131,18 +1159,25 @@ mod tests { let i8s = [-128i8, 127, 42, -117]; + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert!(parcel.write(&i8s[..]).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert_eq!(parcel.read::<u32>().unwrap(), 4); // 4 items assert_eq!(parcel.read::<u32>().unwrap(), 0x8b2a7f80); // bytes + + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1152,10 +1187,14 @@ mod tests { let u16s = [u16::max_value(), 12_345, 42, 117]; + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert!(u16s.serialize(&mut parcel.borrowed()).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1165,6 +1204,9 @@ mod tests { assert_eq!(parcel.read::<u32>().unwrap(), 12345); // 12,345 assert_eq!(parcel.read::<u32>().unwrap(), 42); // 42 assert_eq!(parcel.read::<u32>().unwrap(), 117); // 117 + + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1175,10 +1217,14 @@ mod tests { let i16s = [i16::max_value(), i16::min_value(), 42, -117]; + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert!(i16s.serialize(&mut parcel.borrowed()).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1188,6 +1234,9 @@ mod tests { assert_eq!(parcel.read::<u32>().unwrap(), 0x8000); // i16::min_value() assert_eq!(parcel.read::<u32>().unwrap(), 42); // 42 assert_eq!(parcel.read::<u32>().unwrap(), 0xff8b); // -117 + + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1198,10 +1247,14 @@ mod tests { let u32s = [u32::max_value(), 12_345, 42, 117]; + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert!(u32s.serialize(&mut parcel.borrowed()).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1211,6 +1264,9 @@ mod tests { assert_eq!(parcel.read::<u32>().unwrap(), 12345); // 12,345 assert_eq!(parcel.read::<u32>().unwrap(), 42); // 42 assert_eq!(parcel.read::<u32>().unwrap(), 117); // 117 + + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1221,10 +1277,14 @@ mod tests { let i32s = [i32::max_value(), i32::min_value(), 42, -117]; + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert!(i32s.serialize(&mut parcel.borrowed()).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1234,6 +1294,9 @@ mod tests { assert_eq!(parcel.read::<u32>().unwrap(), 0x80000000); // i32::min_value() assert_eq!(parcel.read::<u32>().unwrap(), 42); // 42 assert_eq!(parcel.read::<u32>().unwrap(), 0xffffff8b); // -117 + + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1244,10 +1307,14 @@ mod tests { let u64s = [u64::max_value(), 12_345, 42, 117]; + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert!(u64s.serialize(&mut parcel.borrowed()).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1258,10 +1325,14 @@ mod tests { let i64s = [i64::max_value(), i64::min_value(), 42, -117]; + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert!(i64s.serialize(&mut parcel.borrowed()).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1272,10 +1343,14 @@ mod tests { let f32s = [std::f32::NAN, std::f32::INFINITY, 1.23456789, std::f32::EPSILON]; + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert!(f32s.serialize(&mut parcel.borrowed()).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1288,10 +1363,14 @@ mod tests { let f64s = [std::f64::NAN, std::f64::INFINITY, 1.234567890123456789, std::f64::EPSILON]; + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert!(f64s.serialize(&mut parcel.borrowed()).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } @@ -1309,10 +1388,14 @@ mod tests { let strs = [s1, s2, s3, s4]; + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } assert!(strs.serialize(&mut parcel.borrowed()).is_ok()); + // SAFETY: start is less than the current size of the parcel data buffer, because we haven't + // made it any shorter since we got the position. unsafe { assert!(parcel.set_data_position(start).is_ok()); } diff --git a/libs/binder/rust/src/parcel/parcelable_holder.rs b/libs/binder/rust/src/parcel/parcelable_holder.rs index eb82fb7fd6..f90611361f 100644 --- a/libs/binder/rust/src/parcel/parcelable_holder.rs +++ b/libs/binder/rust/src/parcel/parcelable_holder.rs @@ -133,8 +133,8 @@ impl ParcelableHolder { } } ParcelableHolderData::Parcel(ref mut parcel) => { + // Safety: 0 should always be a valid position. unsafe { - // Safety: 0 should always be a valid position. parcel.set_data_position(0)?; } @@ -214,15 +214,15 @@ impl Parcelable for ParcelableHolder { parcelable.write_to_parcel(parcel)?; let end = parcel.get_data_position(); + // Safety: we got the position from `get_data_position`. unsafe { - // Safety: we got the position from `get_data_position`. parcel.set_data_position(length_start)?; } assert!(end >= data_start); parcel.write(&(end - data_start))?; + // Safety: we got the position from `get_data_position`. unsafe { - // Safety: we got the position from `get_data_position`. parcel.set_data_position(end)?; } @@ -260,11 +260,11 @@ impl Parcelable for ParcelableHolder { new_parcel.append_from(parcel, data_start, data_size)?; *self.data.get_mut().unwrap() = ParcelableHolderData::Parcel(new_parcel); + // Safety: `append_from` checks if `data_size` overflows + // `parcel` and returns `BAD_VALUE` if that happens. We also + // explicitly check for negative and zero `data_size` above, + // so `data_end` is guaranteed to be greater than `data_start`. unsafe { - // Safety: `append_from` checks if `data_size` overflows - // `parcel` and returns `BAD_VALUE` if that happens. We also - // explicitly check for negative and zero `data_size` above, - // so `data_end` is guaranteed to be greater than `data_start`. parcel.set_data_position(data_end)?; } diff --git a/libs/binder/rust/src/proxy.rs b/libs/binder/rust/src/proxy.rs index 036f6b4f01..dad3379bdc 100644 --- a/libs/binder/rust/src/proxy.rs +++ b/libs/binder/rust/src/proxy.rs @@ -49,14 +49,12 @@ impl fmt::Debug for SpIBinder { } } -/// # Safety -/// -/// An `SpIBinder` is an immutable handle to a C++ IBinder, which is thread-safe +/// Safety: An `SpIBinder` is an immutable handle to a C++ IBinder, which is +/// thread-safe. unsafe impl Send for SpIBinder {} -/// # Safety -/// -/// An `SpIBinder` is an immutable handle to a C++ IBinder, which is thread-safe +/// Safety: An `SpIBinder` is an immutable handle to a C++ IBinder, which is +/// thread-safe. unsafe impl Sync for SpIBinder {} impl SpIBinder { @@ -97,11 +95,9 @@ impl SpIBinder { /// Return true if this binder object is hosted in a different process than /// the current one. pub fn is_remote(&self) -> bool { - unsafe { - // Safety: `SpIBinder` guarantees that it always contains a valid - // `AIBinder` pointer. - sys::AIBinder_isRemote(self.as_native()) - } + // Safety: `SpIBinder` guarantees that it always contains a valid + // `AIBinder` pointer. + unsafe { sys::AIBinder_isRemote(self.as_native()) } } /// Try to convert this Binder object into a trait object for the given @@ -116,12 +112,12 @@ impl SpIBinder { /// Return the interface class of this binder object, if associated with /// one. pub fn get_class(&mut self) -> Option<InterfaceClass> { + // Safety: `SpIBinder` guarantees that it always contains a valid + // `AIBinder` pointer. `AIBinder_getClass` returns either a null + // pointer or a valid pointer to an `AIBinder_Class`. After mapping + // null to None, we can safely construct an `InterfaceClass` if the + // pointer was non-null. unsafe { - // Safety: `SpIBinder` guarantees that it always contains a valid - // `AIBinder` pointer. `AIBinder_getClass` returns either a null - // pointer or a valid pointer to an `AIBinder_Class`. After mapping - // null to None, we can safely construct an `InterfaceClass` if the - // pointer was non-null. let class = sys::AIBinder_getClass(self.as_native_mut()); class.as_ref().map(|p| InterfaceClass::from_ptr(p)) } @@ -152,7 +148,8 @@ pub mod unstable_api { /// /// See `SpIBinder::from_raw`. pub unsafe fn new_spibinder(ptr: *mut sys::AIBinder) -> Option<SpIBinder> { - SpIBinder::from_raw(ptr) + // Safety: The caller makes the same guarantees as this requires. + unsafe { SpIBinder::from_raw(ptr) } } } @@ -171,30 +168,24 @@ pub trait AssociateClass { impl AssociateClass for SpIBinder { fn associate_class(&mut self, class: InterfaceClass) -> bool { - unsafe { - // Safety: `SpIBinder` guarantees that it always contains a valid - // `AIBinder` pointer. An `InterfaceClass` can always be converted - // into a valid `AIBinder_Class` pointer, so these parameters are - // always safe. - sys::AIBinder_associateClass(self.as_native_mut(), class.into()) - } + // Safety: `SpIBinder` guarantees that it always contains a valid + // `AIBinder` pointer. An `InterfaceClass` can always be converted + // into a valid `AIBinder_Class` pointer, so these parameters are + // always safe. + unsafe { sys::AIBinder_associateClass(self.as_native_mut(), class.into()) } } } impl Ord for SpIBinder { fn cmp(&self, other: &Self) -> Ordering { - let less_than = unsafe { - // Safety: SpIBinder always holds a valid `AIBinder` pointer, so - // this pointer is always safe to pass to `AIBinder_lt` (null is - // also safe to pass to this function, but we should never do that). - sys::AIBinder_lt(self.0.as_ptr(), other.0.as_ptr()) - }; - let greater_than = unsafe { - // Safety: SpIBinder always holds a valid `AIBinder` pointer, so - // this pointer is always safe to pass to `AIBinder_lt` (null is - // also safe to pass to this function, but we should never do that). - sys::AIBinder_lt(other.0.as_ptr(), self.0.as_ptr()) - }; + // Safety: SpIBinder always holds a valid `AIBinder` pointer, so this + // pointer is always safe to pass to `AIBinder_lt` (null is also safe to + // pass to this function, but we should never do that). + let less_than = unsafe { sys::AIBinder_lt(self.0.as_ptr(), other.0.as_ptr()) }; + // Safety: SpIBinder always holds a valid `AIBinder` pointer, so this + // pointer is always safe to pass to `AIBinder_lt` (null is also safe to + // pass to this function, but we should never do that). + let greater_than = unsafe { sys::AIBinder_lt(other.0.as_ptr(), self.0.as_ptr()) }; if !less_than && !greater_than { Ordering::Equal } else if less_than { @@ -221,10 +212,10 @@ impl Eq for SpIBinder {} impl Clone for SpIBinder { fn clone(&self) -> Self { + // Safety: Cloning a strong reference must increment the reference + // count. We are guaranteed by the `SpIBinder` constructor + // invariants that `self.0` is always a valid `AIBinder` pointer. unsafe { - // Safety: Cloning a strong reference must increment the reference - // count. We are guaranteed by the `SpIBinder` constructor - // invariants that `self.0` is always a valid `AIBinder` pointer. sys::AIBinder_incStrong(self.0.as_ptr()); } Self(self.0) @@ -235,9 +226,9 @@ impl Drop for SpIBinder { // We hold a strong reference to the IBinder in SpIBinder and need to give up // this reference on drop. fn drop(&mut self) { + // Safety: SpIBinder always holds a valid `AIBinder` pointer, so we + // know this pointer is safe to pass to `AIBinder_decStrong` here. unsafe { - // Safety: SpIBinder always holds a valid `AIBinder` pointer, so we - // know this pointer is safe to pass to `AIBinder_decStrong` here. sys::AIBinder_decStrong(self.as_native_mut()); } } @@ -246,26 +237,24 @@ impl Drop for SpIBinder { impl<T: AsNative<sys::AIBinder>> IBinderInternal for T { fn prepare_transact(&self) -> Result<Parcel> { let mut input = ptr::null_mut(); + // Safety: `SpIBinder` guarantees that `self` always contains a + // valid pointer to an `AIBinder`. It is safe to cast from an + // immutable pointer to a mutable pointer here, because + // `AIBinder_prepareTransaction` only calls immutable `AIBinder` + // methods but the parameter is unfortunately not marked as const. + // + // After the call, input will be either a valid, owned `AParcel` + // pointer, or null. let status = unsafe { - // Safety: `SpIBinder` guarantees that `self` always contains a - // valid pointer to an `AIBinder`. It is safe to cast from an - // immutable pointer to a mutable pointer here, because - // `AIBinder_prepareTransaction` only calls immutable `AIBinder` - // methods but the parameter is unfortunately not marked as const. - // - // After the call, input will be either a valid, owned `AParcel` - // pointer, or null. sys::AIBinder_prepareTransaction(self.as_native() as *mut sys::AIBinder, &mut input) }; status_result(status)?; - unsafe { - // Safety: At this point, `input` is either a valid, owned `AParcel` - // pointer, or null. `OwnedParcel::from_raw` safely handles both cases, - // taking ownership of the parcel. - Parcel::from_raw(input).ok_or(StatusCode::UNEXPECTED_NULL) - } + // Safety: At this point, `input` is either a valid, owned `AParcel` + // pointer, or null. `OwnedParcel::from_raw` safely handles both cases, + // taking ownership of the parcel. + unsafe { Parcel::from_raw(input).ok_or(StatusCode::UNEXPECTED_NULL) } } fn submit_transact( @@ -275,23 +264,23 @@ impl<T: AsNative<sys::AIBinder>> IBinderInternal for T { flags: TransactionFlags, ) -> Result<Parcel> { let mut reply = ptr::null_mut(); + // Safety: `SpIBinder` guarantees that `self` always contains a + // valid pointer to an `AIBinder`. Although `IBinder::transact` is + // not a const method, it is still safe to cast our immutable + // pointer to mutable for the call. First, `IBinder::transact` is + // thread-safe, so concurrency is not an issue. The only way that + // `transact` can affect any visible, mutable state in the current + // process is by calling `onTransact` for a local service. However, + // in order for transactions to be thread-safe, this method must + // dynamically lock its data before modifying it. We enforce this + // property in Rust by requiring `Sync` for remotable objects and + // only providing `on_transact` with an immutable reference to + // `self`. + // + // This call takes ownership of the `data` parcel pointer, and + // passes ownership of the `reply` out parameter to its caller. It + // does not affect ownership of the `binder` parameter. let status = unsafe { - // Safety: `SpIBinder` guarantees that `self` always contains a - // valid pointer to an `AIBinder`. Although `IBinder::transact` is - // not a const method, it is still safe to cast our immutable - // pointer to mutable for the call. First, `IBinder::transact` is - // thread-safe, so concurrency is not an issue. The only way that - // `transact` can affect any visible, mutable state in the current - // process is by calling `onTransact` for a local service. However, - // in order for transactions to be thread-safe, this method must - // dynamically lock its data before modifying it. We enforce this - // property in Rust by requiring `Sync` for remotable objects and - // only providing `on_transact` with an immutable reference to - // `self`. - // - // This call takes ownership of the `data` parcel pointer, and - // passes ownership of the `reply` out parameter to its caller. It - // does not affect ownership of the `binder` parameter. sys::AIBinder_transact( self.as_native() as *mut sys::AIBinder, code, @@ -302,45 +291,45 @@ impl<T: AsNative<sys::AIBinder>> IBinderInternal for T { }; status_result(status)?; - unsafe { - // Safety: `reply` is either a valid `AParcel` pointer or null - // after the call to `AIBinder_transact` above, so we can - // construct a `Parcel` out of it. `AIBinder_transact` passes - // ownership of the `reply` parcel to Rust, so we need to - // construct an owned variant. - Parcel::from_raw(reply).ok_or(StatusCode::UNEXPECTED_NULL) - } + // Safety: `reply` is either a valid `AParcel` pointer or null + // after the call to `AIBinder_transact` above, so we can + // construct a `Parcel` out of it. `AIBinder_transact` passes + // ownership of the `reply` parcel to Rust, so we need to + // construct an owned variant. + unsafe { Parcel::from_raw(reply).ok_or(StatusCode::UNEXPECTED_NULL) } } fn is_binder_alive(&self) -> bool { - unsafe { - // Safety: `SpIBinder` guarantees that `self` always contains a - // valid pointer to an `AIBinder`. - // - // This call does not affect ownership of its pointer parameter. - sys::AIBinder_isAlive(self.as_native()) - } + // Safety: `SpIBinder` guarantees that `self` always contains a valid + // pointer to an `AIBinder`. + // + // This call does not affect ownership of its pointer parameter. + unsafe { sys::AIBinder_isAlive(self.as_native()) } } #[cfg(not(android_vndk))] fn set_requesting_sid(&mut self, enable: bool) { + // Safety: `SpIBinder` guarantees that `self` always contains a valid + // pointer to an `AIBinder`. + // + // This call does not affect ownership of its pointer parameter. unsafe { sys::AIBinder_setRequestingSid(self.as_native_mut(), enable) }; } fn dump<F: AsRawFd>(&mut self, fp: &F, args: &[&str]) -> Result<()> { let args: Vec<_> = args.iter().map(|a| CString::new(*a).unwrap()).collect(); let mut arg_ptrs: Vec<_> = args.iter().map(|a| a.as_ptr()).collect(); + // Safety: `SpIBinder` guarantees that `self` always contains a + // valid pointer to an `AIBinder`. `AsRawFd` guarantees that the + // file descriptor parameter is always be a valid open file. The + // `args` pointer parameter is a valid pointer to an array of C + // strings that will outlive the call since `args` lives for the + // whole function scope. + // + // This call does not affect ownership of its binder pointer + // parameter and does not take ownership of the file or args array + // parameters. let status = unsafe { - // Safety: `SpIBinder` guarantees that `self` always contains a - // valid pointer to an `AIBinder`. `AsRawFd` guarantees that the - // file descriptor parameter is always be a valid open file. The - // `args` pointer parameter is a valid pointer to an array of C - // strings that will outlive the call since `args` lives for the - // whole function scope. - // - // This call does not affect ownership of its binder pointer - // parameter and does not take ownership of the file or args array - // parameters. sys::AIBinder_dump( self.as_native_mut(), fp.as_raw_fd(), @@ -353,22 +342,18 @@ impl<T: AsNative<sys::AIBinder>> IBinderInternal for T { fn get_extension(&mut self) -> Result<Option<SpIBinder>> { let mut out = ptr::null_mut(); - let status = unsafe { - // Safety: `SpIBinder` guarantees that `self` always contains a - // valid pointer to an `AIBinder`. After this call, the `out` - // parameter will be either null, or a valid pointer to an - // `AIBinder`. - // - // This call passes ownership of the out pointer to its caller - // (assuming it is set to a non-null value). - sys::AIBinder_getExtension(self.as_native_mut(), &mut out) - }; - let ibinder = unsafe { - // Safety: The call above guarantees that `out` is either null or a - // valid, owned pointer to an `AIBinder`, both of which are safe to - // pass to `SpIBinder::from_raw`. - SpIBinder::from_raw(out) - }; + // Safety: `SpIBinder` guarantees that `self` always contains a + // valid pointer to an `AIBinder`. After this call, the `out` + // parameter will be either null, or a valid pointer to an + // `AIBinder`. + // + // This call passes ownership of the out pointer to its caller + // (assuming it is set to a non-null value). + let status = unsafe { sys::AIBinder_getExtension(self.as_native_mut(), &mut out) }; + // Safety: The call above guarantees that `out` is either null or a + // valid, owned pointer to an `AIBinder`, both of which are safe to + // pass to `SpIBinder::from_raw`. + let ibinder = unsafe { SpIBinder::from_raw(out) }; status_result(status)?; Ok(ibinder) @@ -377,17 +362,17 @@ impl<T: AsNative<sys::AIBinder>> IBinderInternal for T { impl<T: AsNative<sys::AIBinder>> IBinder for T { fn link_to_death(&mut self, recipient: &mut DeathRecipient) -> Result<()> { + // Safety: `SpIBinder` guarantees that `self` always contains a + // valid pointer to an `AIBinder`. `recipient` can always be + // converted into a valid pointer to an + // `AIBinder_DeathRecipient`. + // + // The cookie is also the correct pointer, and by calling new_cookie, + // we have created a new ref-count to the cookie, which linkToDeath + // takes ownership of. Once the DeathRecipient is unlinked for any + // reason (including if this call fails), the onUnlinked callback + // will consume that ref-count. status_result(unsafe { - // Safety: `SpIBinder` guarantees that `self` always contains a - // valid pointer to an `AIBinder`. `recipient` can always be - // converted into a valid pointer to an - // `AIBinder_DeathRecipient`. - // - // The cookie is also the correct pointer, and by calling new_cookie, - // we have created a new ref-count to the cookie, which linkToDeath - // takes ownership of. Once the DeathRecipient is unlinked for any - // reason (including if this call fails), the onUnlinked callback - // will consume that ref-count. sys::AIBinder_linkToDeath( self.as_native_mut(), recipient.as_native_mut(), @@ -397,13 +382,13 @@ impl<T: AsNative<sys::AIBinder>> IBinder for T { } fn unlink_to_death(&mut self, recipient: &mut DeathRecipient) -> Result<()> { + // Safety: `SpIBinder` guarantees that `self` always contains a + // valid pointer to an `AIBinder`. `recipient` can always be + // converted into a valid pointer to an + // `AIBinder_DeathRecipient`. Any value is safe to pass as the + // cookie, although we depend on this value being set by + // `get_cookie` when the death recipient callback is called. status_result(unsafe { - // Safety: `SpIBinder` guarantees that `self` always contains a - // valid pointer to an `AIBinder`. `recipient` can always be - // converted into a valid pointer to an - // `AIBinder_DeathRecipient`. Any value is safe to pass as the - // cookie, although we depend on this value being set by - // `get_cookie` when the death recipient callback is called. sys::AIBinder_unlinkToDeath( self.as_native_mut(), recipient.as_native_mut(), @@ -413,13 +398,11 @@ impl<T: AsNative<sys::AIBinder>> IBinder for T { } fn ping_binder(&mut self) -> Result<()> { - let status = unsafe { - // Safety: `SpIBinder` guarantees that `self` always contains a - // valid pointer to an `AIBinder`. - // - // This call does not affect ownership of its pointer parameter. - sys::AIBinder_ping(self.as_native_mut()) - }; + // Safety: `SpIBinder` guarantees that `self` always contains a + // valid pointer to an `AIBinder`. + // + // This call does not affect ownership of its pointer parameter. + let status = unsafe { sys::AIBinder_ping(self.as_native_mut()) }; status_result(status) } } @@ -472,35 +455,31 @@ impl fmt::Debug for WpIBinder { } } -/// # Safety -/// -/// A `WpIBinder` is an immutable handle to a C++ IBinder, which is thread-safe. +/// Safety: A `WpIBinder` is an immutable handle to a C++ IBinder, which is +/// thread-safe. unsafe impl Send for WpIBinder {} -/// # Safety -/// -/// A `WpIBinder` is an immutable handle to a C++ IBinder, which is thread-safe. +/// Safety: A `WpIBinder` is an immutable handle to a C++ IBinder, which is +/// thread-safe. unsafe impl Sync for WpIBinder {} impl WpIBinder { /// Create a new weak reference from an object that can be converted into a /// raw `AIBinder` pointer. fn new<B: AsNative<sys::AIBinder>>(binder: &mut B) -> WpIBinder { - let ptr = unsafe { - // Safety: `SpIBinder` guarantees that `binder` always contains a - // valid pointer to an `AIBinder`. - sys::AIBinder_Weak_new(binder.as_native_mut()) - }; + // Safety: `SpIBinder` guarantees that `binder` always contains a valid + // pointer to an `AIBinder`. + let ptr = unsafe { sys::AIBinder_Weak_new(binder.as_native_mut()) }; Self(ptr::NonNull::new(ptr).expect("Unexpected null pointer from AIBinder_Weak_new")) } /// Promote this weak reference to a strong reference to the binder object. pub fn promote(&self) -> Option<SpIBinder> { + // Safety: `WpIBinder` always contains a valid weak reference, so we can + // pass this pointer to `AIBinder_Weak_promote`. Returns either null or + // an AIBinder owned by the caller, both of which are valid to pass to + // `SpIBinder::from_raw`. unsafe { - // Safety: `WpIBinder` always contains a valid weak reference, so we - // can pass this pointer to `AIBinder_Weak_promote`. Returns either - // null or an AIBinder owned by the caller, both of which are valid - // to pass to `SpIBinder::from_raw`. let ptr = sys::AIBinder_Weak_promote(self.0.as_ptr()); SpIBinder::from_raw(ptr) } @@ -509,35 +488,27 @@ impl WpIBinder { impl Clone for WpIBinder { fn clone(&self) -> Self { - let ptr = unsafe { - // Safety: WpIBinder always holds a valid `AIBinder_Weak` pointer, - // so this pointer is always safe to pass to `AIBinder_Weak_clone` - // (although null is also a safe value to pass to this API). - // - // We get ownership of the returned pointer, so can construct a new - // WpIBinder object from it. - sys::AIBinder_Weak_clone(self.0.as_ptr()) - }; + // Safety: WpIBinder always holds a valid `AIBinder_Weak` pointer, so + // this pointer is always safe to pass to `AIBinder_Weak_clone` + // (although null is also a safe value to pass to this API). + // + // We get ownership of the returned pointer, so can construct a new + // WpIBinder object from it. + let ptr = unsafe { sys::AIBinder_Weak_clone(self.0.as_ptr()) }; Self(ptr::NonNull::new(ptr).expect("Unexpected null pointer from AIBinder_Weak_clone")) } } impl Ord for WpIBinder { fn cmp(&self, other: &Self) -> Ordering { - let less_than = unsafe { - // Safety: WpIBinder always holds a valid `AIBinder_Weak` pointer, - // so this pointer is always safe to pass to `AIBinder_Weak_lt` - // (null is also safe to pass to this function, but we should never - // do that). - sys::AIBinder_Weak_lt(self.0.as_ptr(), other.0.as_ptr()) - }; - let greater_than = unsafe { - // Safety: WpIBinder always holds a valid `AIBinder_Weak` pointer, - // so this pointer is always safe to pass to `AIBinder_Weak_lt` - // (null is also safe to pass to this function, but we should never - // do that). - sys::AIBinder_Weak_lt(other.0.as_ptr(), self.0.as_ptr()) - }; + // Safety: WpIBinder always holds a valid `AIBinder_Weak` pointer, so + // this pointer is always safe to pass to `AIBinder_Weak_lt` (null is + // also safe to pass to this function, but we should never do that). + let less_than = unsafe { sys::AIBinder_Weak_lt(self.0.as_ptr(), other.0.as_ptr()) }; + // Safety: WpIBinder always holds a valid `AIBinder_Weak` pointer, so + // this pointer is always safe to pass to `AIBinder_Weak_lt` (null is + // also safe to pass to this function, but we should never do that). + let greater_than = unsafe { sys::AIBinder_Weak_lt(other.0.as_ptr(), self.0.as_ptr()) }; if !less_than && !greater_than { Ordering::Equal } else if less_than { @@ -564,9 +535,9 @@ impl Eq for WpIBinder {} impl Drop for WpIBinder { fn drop(&mut self) { + // Safety: WpIBinder always holds a valid `AIBinder_Weak` pointer, so we + // know this pointer is safe to pass to `AIBinder_Weak_delete` here. unsafe { - // Safety: WpIBinder always holds a valid `AIBinder_Weak` pointer, so we - // know this pointer is safe to pass to `AIBinder_Weak_delete` here. sys::AIBinder_Weak_delete(self.0.as_ptr()); } } @@ -574,7 +545,7 @@ impl Drop for WpIBinder { /// Rust wrapper around DeathRecipient objects. /// -/// The cookie in this struct represents an Arc<F> for the owned callback. +/// The cookie in this struct represents an `Arc<F>` for the owned callback. /// This struct owns a ref-count of it, and so does every binder that we /// have been linked with. /// @@ -592,17 +563,13 @@ struct DeathRecipientVtable { cookie_decr_refcount: unsafe extern "C" fn(*mut c_void), } -/// # Safety -/// -/// A `DeathRecipient` is a wrapper around `AIBinder_DeathRecipient` and a pointer -/// to a `Fn` which is `Sync` and `Send` (the cookie field). As +/// Safety: A `DeathRecipient` is a wrapper around `AIBinder_DeathRecipient` and +/// a pointer to a `Fn` which is `Sync` and `Send` (the cookie field). As /// `AIBinder_DeathRecipient` is threadsafe, this structure is too. unsafe impl Send for DeathRecipient {} -/// # Safety -/// -/// A `DeathRecipient` is a wrapper around `AIBinder_DeathRecipient` and a pointer -/// to a `Fn` which is `Sync` and `Send` (the cookie field). As +/// Safety: A `DeathRecipient` is a wrapper around `AIBinder_DeathRecipient` and +/// a pointer to a `Fn` which is `Sync` and `Send` (the cookie field). As /// `AIBinder_DeathRecipient` is threadsafe, this structure is too. unsafe impl Sync for DeathRecipient {} @@ -614,19 +581,17 @@ impl DeathRecipient { F: Fn() + Send + Sync + 'static, { let callback: *const F = Arc::into_raw(Arc::new(callback)); - let recipient = unsafe { - // Safety: The function pointer is a valid death recipient callback. - // - // This call returns an owned `AIBinder_DeathRecipient` pointer - // which must be destroyed via `AIBinder_DeathRecipient_delete` when - // no longer needed. - sys::AIBinder_DeathRecipient_new(Some(Self::binder_died::<F>)) - }; + // Safety: The function pointer is a valid death recipient callback. + // + // This call returns an owned `AIBinder_DeathRecipient` pointer which + // must be destroyed via `AIBinder_DeathRecipient_delete` when no longer + // needed. + let recipient = unsafe { sys::AIBinder_DeathRecipient_new(Some(Self::binder_died::<F>)) }; + // Safety: The function pointer is a valid onUnlinked callback. + // + // All uses of linkToDeath in this file correctly increment the + // ref-count that this onUnlinked callback will decrement. unsafe { - // Safety: The function pointer is a valid onUnlinked callback. - // - // All uses of linkToDeath in this file correctly increment the - // ref-count that this onUnlinked callback will decrement. sys::AIBinder_DeathRecipient_setOnUnlinked( recipient, Some(Self::cookie_decr_refcount::<F>), @@ -648,7 +613,12 @@ impl DeathRecipient { /// /// The caller must handle the returned ref-count correctly. unsafe fn new_cookie(&self) -> *mut c_void { - (self.vtable.cookie_incr_refcount)(self.cookie); + // Safety: `cookie_incr_refcount` points to + // `Self::cookie_incr_refcount`, and `self.cookie` is the cookie for an + // Arc<F>. + unsafe { + (self.vtable.cookie_incr_refcount)(self.cookie); + } // Return a raw pointer with ownership of a ref-count self.cookie @@ -667,13 +637,14 @@ impl DeathRecipient { /// /// # Safety /// - /// The `cookie` parameter must be the cookie for an Arc<F> and + /// The `cookie` parameter must be the cookie for an `Arc<F>` and /// the caller must hold a ref-count to it. unsafe extern "C" fn binder_died<F>(cookie: *mut c_void) where F: Fn() + Send + Sync + 'static, { - let callback = (cookie as *const F).as_ref().unwrap(); + // Safety: The caller promises that `cookie` is for an Arc<F>. + let callback = unsafe { (cookie as *const F).as_ref().unwrap() }; callback(); } @@ -682,34 +653,34 @@ impl DeathRecipient { /// /// # Safety /// - /// The `cookie` parameter must be the cookie for an Arc<F> and + /// The `cookie` parameter must be the cookie for an `Arc<F>` and /// the owner must give up a ref-count to it. unsafe extern "C" fn cookie_decr_refcount<F>(cookie: *mut c_void) where F: Fn() + Send + Sync + 'static, { - drop(Arc::from_raw(cookie as *const F)); + // Safety: The caller promises that `cookie` is for an Arc<F>. + drop(unsafe { Arc::from_raw(cookie as *const F) }); } /// Callback that increments the ref-count. /// /// # Safety /// - /// The `cookie` parameter must be the cookie for an Arc<F> and + /// The `cookie` parameter must be the cookie for an `Arc<F>` and /// the owner must handle the created ref-count properly. unsafe extern "C" fn cookie_incr_refcount<F>(cookie: *mut c_void) where F: Fn() + Send + Sync + 'static, { - let arc = mem::ManuallyDrop::new(Arc::from_raw(cookie as *const F)); + // Safety: The caller promises that `cookie` is for an Arc<F>. + let arc = mem::ManuallyDrop::new(unsafe { Arc::from_raw(cookie as *const F) }); mem::forget(Arc::clone(&arc)); } } -/// # Safety -/// -/// A `DeathRecipient` is always constructed with a valid raw pointer to an -/// `AIBinder_DeathRecipient`, so it is always type-safe to extract this +/// Safety: A `DeathRecipient` is always constructed with a valid raw pointer to +/// an `AIBinder_DeathRecipient`, so it is always type-safe to extract this /// pointer. unsafe impl AsNative<sys::AIBinder_DeathRecipient> for DeathRecipient { fn as_native(&self) -> *const sys::AIBinder_DeathRecipient { @@ -723,18 +694,19 @@ unsafe impl AsNative<sys::AIBinder_DeathRecipient> for DeathRecipient { impl Drop for DeathRecipient { fn drop(&mut self) { + // Safety: `self.recipient` is always a valid, owned + // `AIBinder_DeathRecipient` pointer returned by + // `AIBinder_DeathRecipient_new` when `self` was created. This delete + // method can only be called once when `self` is dropped. unsafe { - // Safety: `self.recipient` is always a valid, owned - // `AIBinder_DeathRecipient` pointer returned by - // `AIBinder_DeathRecipient_new` when `self` was created. This - // delete method can only be called once when `self` is dropped. sys::AIBinder_DeathRecipient_delete(self.recipient); + } - // Safety: We own a ref-count to the cookie, and so does every - // linked binder. This call gives up our ref-count. The linked - // binders should already have given up their ref-count, or should - // do so shortly. - (self.vtable.cookie_decr_refcount)(self.cookie) + // Safety: We own a ref-count to the cookie, and so does every linked + // binder. This call gives up our ref-count. The linked binders should + // already have given up their ref-count, or should do so shortly. + unsafe { + (self.vtable.cookie_decr_refcount)(self.cookie); } } } @@ -754,11 +726,9 @@ pub trait Proxy: Sized + Interface { fn from_binder(binder: SpIBinder) -> Result<Self>; } -/// # Safety -/// -/// This is a convenience method that wraps `AsNative` for `SpIBinder` to allow -/// invocation of `IBinder` methods directly from `Interface` objects. It shares -/// the same safety as the implementation for `SpIBinder`. +/// Safety: This is a convenience method that wraps `AsNative` for `SpIBinder` +/// to allow invocation of `IBinder` methods directly from `Interface` objects. +/// It shares the same safety as the implementation for `SpIBinder`. unsafe impl<T: Proxy> AsNative<sys::AIBinder> for T { fn as_native(&self) -> *const sys::AIBinder { self.as_binder().as_native() @@ -773,24 +743,20 @@ unsafe impl<T: Proxy> AsNative<sys::AIBinder> for T { /// exist. pub fn get_service(name: &str) -> Option<SpIBinder> { let name = CString::new(name).ok()?; - unsafe { - // Safety: `AServiceManager_getService` returns either a null pointer or - // a valid pointer to an owned `AIBinder`. Either of these values is - // safe to pass to `SpIBinder::from_raw`. - SpIBinder::from_raw(sys::AServiceManager_getService(name.as_ptr())) - } + // Safety: `AServiceManager_getService` returns either a null pointer or a + // valid pointer to an owned `AIBinder`. Either of these values is safe to + // pass to `SpIBinder::from_raw`. + unsafe { SpIBinder::from_raw(sys::AServiceManager_getService(name.as_ptr())) } } /// Retrieve an existing service, or start it if it is configured as a dynamic /// service and isn't yet started. pub fn wait_for_service(name: &str) -> Option<SpIBinder> { let name = CString::new(name).ok()?; - unsafe { - // Safety: `AServiceManager_waitforService` returns either a null - // pointer or a valid pointer to an owned `AIBinder`. Either of these - // values is safe to pass to `SpIBinder::from_raw`. - SpIBinder::from_raw(sys::AServiceManager_waitForService(name.as_ptr())) - } + // Safety: `AServiceManager_waitforService` returns either a null pointer or + // a valid pointer to an owned `AIBinder`. Either of these values is safe to + // pass to `SpIBinder::from_raw`. + unsafe { SpIBinder::from_raw(sys::AServiceManager_waitForService(name.as_ptr())) } } /// Retrieve an existing service for a particular interface, blocking for a few @@ -809,12 +775,10 @@ pub fn wait_for_interface<T: FromIBinder + ?Sized>(name: &str) -> Result<Strong< pub fn is_declared(interface: &str) -> Result<bool> { let interface = CString::new(interface).or(Err(StatusCode::UNEXPECTED_NULL))?; - unsafe { - // Safety: `interface` is a valid null-terminated C-style string and is - // only borrowed for the lifetime of the call. The `interface` local - // outlives this call as it lives for the function scope. - Ok(sys::AServiceManager_isDeclared(interface.as_ptr())) - } + // Safety: `interface` is a valid null-terminated C-style string and is only + // borrowed for the lifetime of the call. The `interface` local outlives + // this call as it lives for the function scope. + unsafe { Ok(sys::AServiceManager_isDeclared(interface.as_ptr())) } } /// Retrieve all declared instances for a particular interface @@ -827,11 +791,13 @@ pub fn get_declared_instances(interface: &str) -> Result<Vec<String>> { // CString, and outlives this callback. The null handling here is just // to avoid the possibility of unwinding across C code if this crate is // ever compiled with panic=unwind. - if let Some(instances) = opaque.cast::<Vec<CString>>().as_mut() { + if let Some(instances) = unsafe { opaque.cast::<Vec<CString>>().as_mut() } { // Safety: instance is a valid null-terminated C string with a // lifetime at least as long as this function, and we immediately // copy it into an owned CString. - instances.push(CStr::from_ptr(instance).to_owned()); + unsafe { + instances.push(CStr::from_ptr(instance).to_owned()); + } } else { eprintln!("Opaque pointer was null in get_declared_instances callback!"); } @@ -839,10 +805,10 @@ pub fn get_declared_instances(interface: &str) -> Result<Vec<String>> { let interface = CString::new(interface).or(Err(StatusCode::UNEXPECTED_NULL))?; let mut instances: Vec<CString> = vec![]; + // Safety: `interface` and `instances` are borrowed for the length of this + // call and both outlive the call. `interface` is guaranteed to be a valid + // null-terminated C-style string. unsafe { - // Safety: `interface` and `instances` are borrowed for the length of - // this call and both outlive the call. `interface` is guaranteed to be - // a valid null-terminated C-style string. sys::AServiceManager_forEachDeclaredInstance( interface.as_ptr(), &mut instances as *mut _ as *mut c_void, @@ -860,10 +826,8 @@ pub fn get_declared_instances(interface: &str) -> Result<Vec<String>> { }) } -/// # Safety -/// -/// `SpIBinder` guarantees that `binder` always contains a valid pointer to an -/// `AIBinder`, so we can trivially extract this pointer here. +/// Safety: `SpIBinder` guarantees that `binder` always contains a valid pointer +/// to an `AIBinder`, so we can trivially extract this pointer here. unsafe impl AsNative<sys::AIBinder> for SpIBinder { fn as_native(&self) -> *const sys::AIBinder { self.0.as_ptr() diff --git a/libs/binder/rust/src/state.rs b/libs/binder/rust/src/state.rs index 4886c5fe86..a3a2562eb1 100644 --- a/libs/binder/rust/src/state.rs +++ b/libs/binder/rust/src/state.rs @@ -35,8 +35,8 @@ impl ProcessState { /// not work: the callbacks will be queued but never called as there is no /// thread to call them on. pub fn start_thread_pool() { + // Safety: Safe FFI unsafe { - // Safety: Safe FFI sys::ABinderProcess_startThreadPool(); } } @@ -48,8 +48,8 @@ impl ProcessState { /// called, this is 15. If it is called additional times, the thread pool /// size can only be increased. pub fn set_thread_pool_max_thread_count(num_threads: u32) { + // Safety: Safe FFI unsafe { - // Safety: Safe FFI sys::ABinderProcess_setThreadPoolMaxThreadCount(num_threads); } } @@ -62,8 +62,8 @@ impl ProcessState { /// [`set_thread_pool_max_thread_count`](Self::set_thread_pool_max_thread_count) /// and [`start_thread_pool`](Self::start_thread_pool). pub fn join_thread_pool() { + // Safety: Safe FFI unsafe { - // Safety: Safe FFI sys::ABinderProcess_joinThreadPool(); } } @@ -86,10 +86,8 @@ impl ThreadState { /// \return calling uid or the current process's UID if this thread isn't /// processing a transaction. pub fn get_calling_uid() -> uid_t { - unsafe { - // Safety: Safe FFI - sys::AIBinder_getCallingUid() - } + // Safety: Safe FFI + unsafe { sys::AIBinder_getCallingUid() } } /// This returns the calling PID assuming that this thread is called from a @@ -111,10 +109,8 @@ impl ThreadState { /// If the transaction being processed is a oneway transaction, then this /// method will return 0. pub fn get_calling_pid() -> pid_t { - unsafe { - // Safety: Safe FFI - sys::AIBinder_getCallingPid() - } + // Safety: Safe FFI + unsafe { sys::AIBinder_getCallingPid() } } /// Determine whether the current thread is currently executing an incoming transaction. @@ -122,10 +118,8 @@ impl ThreadState { /// \return true if the current thread is currently executing an incoming transaction, and false /// otherwise. pub fn is_handling_transaction() -> bool { - unsafe { - // Safety: Safe FFI - sys::AIBinder_isHandlingTransaction() - } + // Safety: Safe FFI + unsafe { sys::AIBinder_isHandlingTransaction() } } /// This function makes the client's security context available to the diff --git a/libs/binder/rust/tests/integration.rs b/libs/binder/rust/tests/integration.rs index ca2cedc19d..c049b807df 100644 --- a/libs/binder/rust/tests/integration.rs +++ b/libs/binder/rust/tests/integration.rs @@ -545,6 +545,11 @@ mod tests { } fn get_expected_selinux_context() -> &'static str { + // SAFETY: The pointer we pass to `getcon` is valid because it comes from a reference, and + // `getcon` doesn't retain it after it returns. If `getcon` succeeds then `out_ptr` will + // point to a valid C string, otherwise it will remain null. We check for null, so the + // pointer we pass to `CStr::from_ptr` must be a valid pointer to a C string. There is a + // memory leak as we don't call `freecon`, but that's fine because this is just a test. unsafe { let mut out_ptr = ptr::null_mut(); assert_eq!(selinux_sys::getcon(&mut out_ptr), 0); diff --git a/libs/binder/rust/tests/ndk_rust_interop.rs b/libs/binder/rust/tests/ndk_rust_interop.rs index 415ede1a7b..37f182eb08 100644 --- a/libs/binder/rust/tests/ndk_rust_interop.rs +++ b/libs/binder/rust/tests/ndk_rust_interop.rs @@ -28,10 +28,11 @@ use std::os::raw::{c_char, c_int}; /// /// # Safety /// -/// service_name must be a valid, non-null C-style string (null-terminated). +/// service_name must be a valid, non-null C-style string (nul-terminated). #[no_mangle] pub unsafe extern "C" fn rust_call_ndk(service_name: *const c_char) -> c_int { - let service_name = CStr::from_ptr(service_name).to_str().unwrap(); + // SAFETY: Our caller promises that service_name is a valid C string. + let service_name = unsafe { CStr::from_ptr(service_name) }.to_str().unwrap(); // The Rust class descriptor pointer will not match the NDK one, but the // descriptor strings match so this needs to still associate. @@ -85,10 +86,11 @@ impl IBinderRustNdkInteropTest for Service { /// /// # Safety /// -/// service_name must be a valid, non-null C-style string (null-terminated). +/// service_name must be a valid, non-null C-style string (nul-terminated). #[no_mangle] pub unsafe extern "C" fn rust_start_service(service_name: *const c_char) -> c_int { - let service_name = CStr::from_ptr(service_name).to_str().unwrap(); + // SAFETY: Our caller promises that service_name is a valid C string. + let service_name = unsafe { CStr::from_ptr(service_name) }.to_str().unwrap(); let service = BnBinderRustNdkInteropTest::new_binder(Service, BinderFeatures::default()); match binder::add_service(service_name, service.as_binder()) { Ok(_) => StatusCode::OK as c_int, diff --git a/libs/binder/rust/tests/parcel_fuzzer/parcel_fuzzer.rs b/libs/binder/rust/tests/parcel_fuzzer/parcel_fuzzer.rs index 29bf92cb97..ce0f742934 100644 --- a/libs/binder/rust/tests/parcel_fuzzer/parcel_fuzzer.rs +++ b/libs/binder/rust/tests/parcel_fuzzer/parcel_fuzzer.rs @@ -105,9 +105,9 @@ fn do_read_fuzz(read_operations: Vec<ReadOperation>, data: &[u8]) { for operation in read_operations { match operation { ReadOperation::SetDataPosition { pos } => { + // Safety: Safe if pos is less than current size of the parcel. + // It relies on C++ code for bound checks unsafe { - // Safety: Safe if pos is less than current size of the parcel. - // It relies on C++ code for bound checks match parcel.set_data_position(pos) { Ok(result) => result, Err(e) => println!("error occurred while setting data position: {:?}", e), diff --git a/libs/binder/rust/tests/serialization.rs b/libs/binder/rust/tests/serialization.rs index 6220db4b28..2b6c282530 100644 --- a/libs/binder/rust/tests/serialization.rs +++ b/libs/binder/rust/tests/serialization.rs @@ -26,7 +26,7 @@ use binder::{ use binder::binder_impl::{Binder, BorrowedParcel, TransactionCode}; use std::ffi::{c_void, CStr, CString}; -use std::sync::Once; +use std::sync::OnceLock; #[allow( non_camel_case_types, @@ -70,20 +70,18 @@ macro_rules! assert { }; } -static SERVICE_ONCE: Once = Once::new(); -static mut SERVICE: Option<SpIBinder> = None; +static SERVICE: OnceLock<SpIBinder> = OnceLock::new(); /// Start binder service and return a raw AIBinder pointer to it. /// /// Safe to call multiple times, only creates the service once. #[no_mangle] pub extern "C" fn rust_service() -> *mut c_void { - unsafe { - SERVICE_ONCE.call_once(|| { - SERVICE = Some(BnReadParcelTest::new_binder((), BinderFeatures::default()).as_binder()); - }); - SERVICE.as_ref().unwrap().as_raw().cast() - } + let service = SERVICE + .get_or_init(|| BnReadParcelTest::new_binder((), BinderFeatures::default()).as_binder()); + // SAFETY: The SpIBinder will remain alive as long as the program is running because it is in + // the static SERVICE, so the pointer is valid forever. + unsafe { service.as_raw().cast() } } /// Empty interface just to use the declare_binder_interface macro @@ -113,11 +111,13 @@ fn on_transact( bindings::Transaction_TEST_BOOL => { assert!(parcel.read::<bool>()?); assert!(!parcel.read::<bool>()?); + // SAFETY: Just reading an extern constant. assert_eq!(parcel.read::<Vec<bool>>()?, unsafe { bindings::TESTDATA_BOOL }); assert_eq!(parcel.read::<Option<Vec<bool>>>()?, None); reply.write(&true)?; reply.write(&false)?; + // SAFETY: Just reading an extern constant. reply.write(&unsafe { bindings::TESTDATA_BOOL }[..])?; reply.write(&(None as Option<Vec<bool>>))?; } @@ -125,14 +125,18 @@ fn on_transact( assert_eq!(parcel.read::<i8>()?, 0); assert_eq!(parcel.read::<i8>()?, 1); assert_eq!(parcel.read::<i8>()?, i8::max_value()); + // SAFETY: Just reading an extern constant. assert_eq!(parcel.read::<Vec<i8>>()?, unsafe { bindings::TESTDATA_I8 }); + // SAFETY: Just reading an extern constant. assert_eq!(parcel.read::<Vec<u8>>()?, unsafe { bindings::TESTDATA_U8 }); assert_eq!(parcel.read::<Option<Vec<i8>>>()?, None); reply.write(&0i8)?; reply.write(&1i8)?; reply.write(&i8::max_value())?; + // SAFETY: Just reading an extern constant. reply.write(&unsafe { bindings::TESTDATA_I8 }[..])?; + // SAFETY: Just reading an extern constant. reply.write(&unsafe { bindings::TESTDATA_U8 }[..])?; reply.write(&(None as Option<Vec<i8>>))?; } @@ -140,12 +144,14 @@ fn on_transact( assert_eq!(parcel.read::<u16>()?, 0); assert_eq!(parcel.read::<u16>()?, 1); assert_eq!(parcel.read::<u16>()?, u16::max_value()); + // SAFETY: Just reading an extern constant. assert_eq!(parcel.read::<Vec<u16>>()?, unsafe { bindings::TESTDATA_CHARS }); assert_eq!(parcel.read::<Option<Vec<u16>>>()?, None); reply.write(&0u16)?; reply.write(&1u16)?; reply.write(&u16::max_value())?; + // SAFETY: Just reading an extern constant. reply.write(&unsafe { bindings::TESTDATA_CHARS }[..])?; reply.write(&(None as Option<Vec<u16>>))?; } @@ -153,12 +159,14 @@ fn on_transact( assert_eq!(parcel.read::<i32>()?, 0); assert_eq!(parcel.read::<i32>()?, 1); assert_eq!(parcel.read::<i32>()?, i32::max_value()); + // SAFETY: Just reading an extern constant. assert_eq!(parcel.read::<Vec<i32>>()?, unsafe { bindings::TESTDATA_I32 }); assert_eq!(parcel.read::<Option<Vec<i32>>>()?, None); reply.write(&0i32)?; reply.write(&1i32)?; reply.write(&i32::max_value())?; + // SAFETY: Just reading an extern constant. reply.write(&unsafe { bindings::TESTDATA_I32 }[..])?; reply.write(&(None as Option<Vec<i32>>))?; } @@ -166,12 +174,14 @@ fn on_transact( assert_eq!(parcel.read::<i64>()?, 0); assert_eq!(parcel.read::<i64>()?, 1); assert_eq!(parcel.read::<i64>()?, i64::max_value()); + // SAFETY: Just reading an extern constant. assert_eq!(parcel.read::<Vec<i64>>()?, unsafe { bindings::TESTDATA_I64 }); assert_eq!(parcel.read::<Option<Vec<i64>>>()?, None); reply.write(&0i64)?; reply.write(&1i64)?; reply.write(&i64::max_value())?; + // SAFETY: Just reading an extern constant. reply.write(&unsafe { bindings::TESTDATA_I64 }[..])?; reply.write(&(None as Option<Vec<i64>>))?; } @@ -179,12 +189,14 @@ fn on_transact( assert_eq!(parcel.read::<u64>()?, 0); assert_eq!(parcel.read::<u64>()?, 1); assert_eq!(parcel.read::<u64>()?, u64::max_value()); + // SAFETY: Just reading an extern constant. assert_eq!(parcel.read::<Vec<u64>>()?, unsafe { bindings::TESTDATA_U64 }); assert_eq!(parcel.read::<Option<Vec<u64>>>()?, None); reply.write(&0u64)?; reply.write(&1u64)?; reply.write(&u64::max_value())?; + // SAFETY: Just reading an extern constant. reply.write(&unsafe { bindings::TESTDATA_U64 }[..])?; reply.write(&(None as Option<Vec<u64>>))?; } @@ -192,10 +204,12 @@ fn on_transact( assert_eq!(parcel.read::<f32>()?, 0f32); let floats = parcel.read::<Vec<f32>>()?; assert!(floats[0].is_nan()); + // SAFETY: Just reading an extern constant. assert_eq!(floats[1..], unsafe { bindings::TESTDATA_FLOAT }[1..]); assert_eq!(parcel.read::<Option<Vec<f32>>>()?, None); reply.write(&0f32)?; + // SAFETY: Just reading an extern constant. reply.write(&unsafe { bindings::TESTDATA_FLOAT }[..])?; reply.write(&(None as Option<Vec<f32>>))?; } @@ -203,10 +217,12 @@ fn on_transact( assert_eq!(parcel.read::<f64>()?, 0f64); let doubles = parcel.read::<Vec<f64>>()?; assert!(doubles[0].is_nan()); + // SAFETY: Just reading an extern constant. assert_eq!(doubles[1..], unsafe { bindings::TESTDATA_DOUBLE }[1..]); assert_eq!(parcel.read::<Option<Vec<f64>>>()?, None); reply.write(&0f64)?; + // SAFETY: Just reading an extern constant. reply.write(&unsafe { bindings::TESTDATA_DOUBLE }[..])?; reply.write(&(None as Option<Vec<f64>>))?; } @@ -216,14 +232,17 @@ fn on_transact( let s: Option<String> = parcel.read()?; assert_eq!(s, None); let s: Option<Vec<Option<String>>> = parcel.read()?; + // SAFETY: Just reading an extern constant. for (s, expected) in s.unwrap().iter().zip(unsafe { bindings::TESTDATA_STRS }.iter()) { let expected = + // SAFETY: Just reading an extern constant. unsafe { expected.as_ref().and_then(|e| CStr::from_ptr(e).to_str().ok()) }; assert_eq!(s.as_deref(), expected); } let s: Option<Vec<Option<String>>> = parcel.read()?; assert_eq!(s, None); + // SAFETY: Just reading an extern constant. let strings: Vec<Option<String>> = unsafe { bindings::TESTDATA_STRS .iter() @@ -258,8 +277,7 @@ fn on_transact( assert!(ibinders[1].is_none()); assert!(parcel.read::<Option<Vec<Option<SpIBinder>>>>()?.is_none()); - let service = - unsafe { SERVICE.as_ref().expect("Global binder service not initialized").clone() }; + let service = SERVICE.get().expect("Global binder service not initialized").clone(); reply.write(&service)?; reply.write(&(None as Option<&SpIBinder>))?; reply.write(&[Some(&service), None][..])?; diff --git a/libs/binder/tests/Android.bp b/libs/binder/tests/Android.bp index 41856f9b79..cd3e7c0fef 100644 --- a/libs/binder/tests/Android.bp +++ b/libs/binder/tests/Android.bp @@ -77,6 +77,8 @@ cc_test { static_libs: [ "binderRecordReplayTestIface-cpp", "binderReadParcelIface-cpp", + "libbinder_random_parcel_seeds", + "libbinder_random_parcel", ], test_suites: ["general-tests"], require_root: true, diff --git a/libs/binder/tests/binderAllocationLimits.cpp b/libs/binder/tests/binderAllocationLimits.cpp index bc40864020..6712c9cece 100644 --- a/libs/binder/tests/binderAllocationLimits.cpp +++ b/libs/binder/tests/binderAllocationLimits.cpp @@ -216,16 +216,16 @@ TEST(RpcBinderAllocation, SetupRpcServer) { auto server = RpcServer::make(); server->setRootObject(sp<BBinder>::make()); - CHECK_EQ(OK, server->setupUnixDomainServer(addr.c_str())); + ASSERT_EQ(OK, server->setupUnixDomainServer(addr.c_str())); std::thread([server]() { server->join(); }).detach(); - status_t status; auto session = RpcSession::make(); - status = session->setupUnixDomainClient(addr.c_str()); - CHECK_EQ(status, OK) << "Could not connect: " << addr << ": " << statusToString(status).c_str(); + status_t status = session->setupUnixDomainClient(addr.c_str()); + ASSERT_EQ(status, OK) << "Could not connect: " << addr << ": " << statusToString(status).c_str(); auto remoteBinder = session->getRootObject(); + ASSERT_NE(remoteBinder, nullptr); size_t mallocs = 0, totalBytes = 0; { @@ -233,7 +233,7 @@ TEST(RpcBinderAllocation, SetupRpcServer) { mallocs++; totalBytes += bytes; }); - CHECK_EQ(OK, remoteBinder->pingBinder()); + ASSERT_EQ(OK, remoteBinder->pingBinder()); } EXPECT_EQ(mallocs, 1); EXPECT_EQ(totalBytes, 40); diff --git a/libs/binder/tests/binderRecordReplayTest.cpp b/libs/binder/tests/binderRecordReplayTest.cpp index 17d5c8a219..6773c95ed6 100644 --- a/libs/binder/tests/binderRecordReplayTest.cpp +++ b/libs/binder/tests/binderRecordReplayTest.cpp @@ -15,6 +15,7 @@ */ #include <BnBinderRecordReplayTest.h> +#include <android-base/file.h> #include <android-base/logging.h> #include <android-base/unique_fd.h> #include <binder/Binder.h> @@ -23,6 +24,11 @@ #include <binder/IPCThreadState.h> #include <binder/IServiceManager.h> #include <binder/RecordedTransaction.h> + +#include <fuzzbinder/libbinder_driver.h> +#include <fuzzer/FuzzedDataProvider.h> +#include <fuzzseeds/random_parcel_seeds.h> + #include <gtest/gtest.h> #include <sys/prctl.h> @@ -30,6 +36,7 @@ #include "parcelables/SingleDataParcelable.h" using namespace android; +using android::generateSeedsFromRecording; using android::binder::Status; using android::binder::debug::RecordedTransaction; using parcelables::SingleDataParcelable; @@ -84,6 +91,44 @@ public: GENERATE_GETTER_SETTER(SingleDataParcelableArray, std::vector<SingleDataParcelable>); }; +std::vector<uint8_t> retrieveData(base::borrowed_fd fd) { + struct stat fdStat; + EXPECT_TRUE(fstat(fd.get(), &fdStat) != -1); + EXPECT_TRUE(fdStat.st_size != 0); + + std::vector<uint8_t> buffer(fdStat.st_size); + auto readResult = android::base::ReadFully(fd, buffer.data(), fdStat.st_size); + EXPECT_TRUE(readResult != 0); + return std::move(buffer); +} + +void replayFuzzService(const sp<BpBinder>& binder, const RecordedTransaction& transaction) { + base::unique_fd seedFd(open("/data/local/tmp/replayFuzzService", + O_RDWR | O_CREAT | O_CLOEXEC | O_TRUNC, 0666)); + ASSERT_TRUE(seedFd.ok()); + + // generate corpus from this transaction. + generateSeedsFromRecording(seedFd, transaction); + + // Read the data which has been written to seed corpus + ASSERT_EQ(0, lseek(seedFd.get(), 0, SEEK_SET)); + std::vector<uint8_t> seedData = retrieveData(seedFd); + + // use fuzzService to replay the corpus + FuzzedDataProvider provider(seedData.data(), seedData.size()); + fuzzService(binder, std::move(provider)); +} + +void replayBinder(const sp<BpBinder>& binder, const RecordedTransaction& transaction) { + // TODO: move logic to replay RecordedTransaction into RecordedTransaction + Parcel data; + data.setData(transaction.getDataParcel().data(), transaction.getDataParcel().dataSize()); + auto result = binder->transact(transaction.getCode(), data, nullptr, transaction.getFlags()); + + // make sure recording does the thing we expect it to do + EXPECT_EQ(OK, result); +} + class BinderRecordReplayTest : public ::testing::Test { public: void SetUp() override { @@ -98,48 +143,46 @@ public: template <typename T, typename U> void recordReplay(Status (IBinderRecordReplayTest::*set)(T), U recordedValue, Status (IBinderRecordReplayTest::*get)(U*), U changedValue) { - base::unique_fd fd(open("/data/local/tmp/binderRecordReplayTest.rec", - O_RDWR | O_CREAT | O_CLOEXEC, 0666)); - ASSERT_TRUE(fd.ok()); - - // record a transaction - mBpBinder->startRecordingBinder(fd); - auto status = (*mInterface.*set)(recordedValue); - EXPECT_TRUE(status.isOk()); - mBpBinder->stopRecordingBinder(); - - // test transaction does the thing we expect it to do - U output; - status = (*mInterface.*get)(&output); - EXPECT_TRUE(status.isOk()); - EXPECT_EQ(output, recordedValue); - - // write over the existing state - status = (*mInterface.*set)(changedValue); - EXPECT_TRUE(status.isOk()); - - status = (*mInterface.*get)(&output); - EXPECT_TRUE(status.isOk()); - - EXPECT_EQ(output, changedValue); - - // replay transaction - ASSERT_EQ(0, lseek(fd.get(), 0, SEEK_SET)); - std::optional<RecordedTransaction> transaction = RecordedTransaction::fromFile(fd); - ASSERT_NE(transaction, std::nullopt); - - // TODO: move logic to replay RecordedTransaction into RecordedTransaction - Parcel data; - data.setData(transaction->getDataParcel().data(), transaction->getDataParcel().dataSize()); - auto result = - mBpBinder->transact(transaction->getCode(), data, nullptr, transaction->getFlags()); - - // make sure recording does the thing we expect it to do - EXPECT_EQ(OK, result); - - status = (*mInterface.*get)(&output); - EXPECT_TRUE(status.isOk()); - EXPECT_EQ(output, recordedValue); + auto replayFunctions = {&replayBinder, &replayFuzzService}; + for (auto replayFunc : replayFunctions) { + base::unique_fd fd(open("/data/local/tmp/binderRecordReplayTest.rec", + O_RDWR | O_CREAT | O_CLOEXEC, 0666)); + ASSERT_TRUE(fd.ok()); + + // record a transaction + mBpBinder->startRecordingBinder(fd); + auto status = (*mInterface.*set)(recordedValue); + EXPECT_TRUE(status.isOk()); + mBpBinder->stopRecordingBinder(); + + // test transaction does the thing we expect it to do + U output; + status = (*mInterface.*get)(&output); + EXPECT_TRUE(status.isOk()); + EXPECT_EQ(output, recordedValue); + + // write over the existing state + status = (*mInterface.*set)(changedValue); + EXPECT_TRUE(status.isOk()); + + status = (*mInterface.*get)(&output); + EXPECT_TRUE(status.isOk()); + + EXPECT_EQ(output, changedValue); + + // replay transaction + ASSERT_EQ(0, lseek(fd.get(), 0, SEEK_SET)); + std::optional<RecordedTransaction> transaction = RecordedTransaction::fromFile(fd); + ASSERT_NE(transaction, std::nullopt); + + const RecordedTransaction& recordedTransaction = *transaction; + // call replay function with recorded transaction + (*replayFunc)(mBpBinder, recordedTransaction); + + status = (*mInterface.*get)(&output); + EXPECT_TRUE(status.isOk()); + EXPECT_EQ(output, recordedValue); + } } private: diff --git a/libs/binder/tests/binderRpcTest.cpp b/libs/binder/tests/binderRpcTest.cpp index d352ce5bca..4c3c68e2e7 100644 --- a/libs/binder/tests/binderRpcTest.cpp +++ b/libs/binder/tests/binderRpcTest.cpp @@ -249,12 +249,12 @@ std::unique_ptr<ProcessSession> BinderRpc::createRpcTestSocketServerProcessEtc( CHECK_EQ(options.numIncomingConnectionsBySession.size(), options.numSessions); } - SocketType socketType = std::get<0>(GetParam()); - RpcSecurity rpcSecurity = std::get<1>(GetParam()); - uint32_t clientVersion = std::get<2>(GetParam()); - uint32_t serverVersion = std::get<3>(GetParam()); - bool singleThreaded = std::get<4>(GetParam()); - bool noKernel = std::get<5>(GetParam()); + SocketType socketType = GetParam().type; + RpcSecurity rpcSecurity = GetParam().security; + uint32_t clientVersion = GetParam().clientVersion; + uint32_t serverVersion = GetParam().serverVersion; + bool singleThreaded = GetParam().singleThreaded; + bool noKernel = GetParam().noKernel; std::string path = android::base::GetExecutableDirectory(); auto servicePath = android::base::StringPrintf("%s/binder_rpc_test_service%s%s", path.c_str(), @@ -674,7 +674,7 @@ TEST_P(BinderRpc, SessionWithIncomingThreadpoolDoesntLeak) { // session 0 - will check for leaks in destrutor of proc // session 1 - we want to make sure it gets deleted when we drop all references to it auto proc = createRpcTestSocketServerProcess( - {.numThreads = 1, .numIncomingConnectionsBySession = {0, 1}, .numSessions = 2}); + {.numThreads = 1, .numSessions = 2, .numIncomingConnectionsBySession = {0, 1}}); wp<RpcSession> session = proc.proc->sessions.at(1).session; @@ -1121,12 +1121,27 @@ TEST_P(BinderRpc, Fds) { } #ifdef BINDER_RPC_TO_TRUSTY_TEST -INSTANTIATE_TEST_CASE_P(Trusty, BinderRpc, - ::testing::Combine(::testing::Values(SocketType::TIPC), - ::testing::Values(RpcSecurity::RAW), - ::testing::ValuesIn(testVersions()), - ::testing::ValuesIn(testVersions()), - ::testing::Values(true), ::testing::Values(true)), + +static std::vector<BinderRpc::ParamType> getTrustyBinderRpcParams() { + std::vector<BinderRpc::ParamType> ret; + + for (const auto& clientVersion : testVersions()) { + for (const auto& serverVersion : testVersions()) { + ret.push_back(BinderRpc::ParamType{ + .type = SocketType::TIPC, + .security = RpcSecurity::RAW, + .clientVersion = clientVersion, + .serverVersion = serverVersion, + .singleThreaded = true, + .noKernel = true, + }); + } + } + + return ret; +} + +INSTANTIATE_TEST_CASE_P(Trusty, BinderRpc, ::testing::ValuesIn(getTrustyBinderRpcParams()), BinderRpc::PrintParamInfo); #else // BINDER_RPC_TO_TRUSTY_TEST bool testSupportVsockLoopback() { @@ -1246,13 +1261,47 @@ static std::vector<SocketType> testSocketTypes(bool hasPreconnected = true) { return ret; } -INSTANTIATE_TEST_CASE_P(PerSocket, BinderRpc, - ::testing::Combine(::testing::ValuesIn(testSocketTypes()), - ::testing::ValuesIn(RpcSecurityValues()), - ::testing::ValuesIn(testVersions()), - ::testing::ValuesIn(testVersions()), - ::testing::Values(false, true), - ::testing::Values(false, true)), +static std::vector<BinderRpc::ParamType> getBinderRpcParams() { + std::vector<BinderRpc::ParamType> ret; + + constexpr bool full = false; + + for (const auto& type : testSocketTypes()) { + if (full || type == SocketType::UNIX) { + for (const auto& security : RpcSecurityValues()) { + for (const auto& clientVersion : testVersions()) { + for (const auto& serverVersion : testVersions()) { + for (bool singleThreaded : {false, true}) { + for (bool noKernel : {false, true}) { + ret.push_back(BinderRpc::ParamType{ + .type = type, + .security = security, + .clientVersion = clientVersion, + .serverVersion = serverVersion, + .singleThreaded = singleThreaded, + .noKernel = noKernel, + }); + } + } + } + } + } + } else { + ret.push_back(BinderRpc::ParamType{ + .type = type, + .security = RpcSecurity::RAW, + .clientVersion = RPC_WIRE_PROTOCOL_VERSION, + .serverVersion = RPC_WIRE_PROTOCOL_VERSION, + .singleThreaded = false, + .noKernel = false, + }); + } + } + + return ret; +} + +INSTANTIATE_TEST_CASE_P(PerSocket, BinderRpc, ::testing::ValuesIn(getBinderRpcParams()), BinderRpc::PrintParamInfo); class BinderRpcServerRootObject diff --git a/libs/binder/tests/binderRpcTestFixture.h b/libs/binder/tests/binderRpcTestFixture.h index 0b8920b5a6..2c9646b30e 100644 --- a/libs/binder/tests/binderRpcTestFixture.h +++ b/libs/binder/tests/binderRpcTestFixture.h @@ -106,15 +106,23 @@ struct BinderRpcTestProcessSession { } }; -class BinderRpc : public ::testing::TestWithParam< - std::tuple<SocketType, RpcSecurity, uint32_t, uint32_t, bool, bool>> { +struct BinderRpcParam { + SocketType type; + RpcSecurity security; + uint32_t clientVersion; + uint32_t serverVersion; + bool singleThreaded; + bool noKernel; +}; +class BinderRpc : public ::testing::TestWithParam<BinderRpcParam> { public: - SocketType socketType() const { return std::get<0>(GetParam()); } - RpcSecurity rpcSecurity() const { return std::get<1>(GetParam()); } - uint32_t clientVersion() const { return std::get<2>(GetParam()); } - uint32_t serverVersion() const { return std::get<3>(GetParam()); } - bool serverSingleThreaded() const { return std::get<4>(GetParam()); } - bool noKernel() const { return std::get<5>(GetParam()); } + // TODO: avoid unnecessary layer of indirection + SocketType socketType() const { return GetParam().type; } + RpcSecurity rpcSecurity() const { return GetParam().security; } + uint32_t clientVersion() const { return GetParam().clientVersion; } + uint32_t serverVersion() const { return GetParam().serverVersion; } + bool serverSingleThreaded() const { return GetParam().singleThreaded; } + bool noKernel() const { return GetParam().noKernel; } bool clientOrServerSingleThreaded() const { return !kEnableRpcThreads || serverSingleThreaded(); @@ -148,15 +156,16 @@ public: } static std::string PrintParamInfo(const testing::TestParamInfo<ParamType>& info) { - auto [type, security, clientVersion, serverVersion, singleThreaded, noKernel] = info.param; - auto ret = PrintToString(type) + "_" + newFactory(security)->toCString() + "_clientV" + - std::to_string(clientVersion) + "_serverV" + std::to_string(serverVersion); - if (singleThreaded) { + auto ret = PrintToString(info.param.type) + "_" + + newFactory(info.param.security)->toCString() + "_clientV" + + std::to_string(info.param.clientVersion) + "_serverV" + + std::to_string(info.param.serverVersion); + if (info.param.singleThreaded) { ret += "_single_threaded"; } else { ret += "_multi_threaded"; } - if (noKernel) { + if (info.param.noKernel) { ret += "_no_kernel"; } else { ret += "_with_kernel"; diff --git a/libs/binder/tests/binderRpcTestTrusty.cpp b/libs/binder/tests/binderRpcTestTrusty.cpp index 28be10db76..fcb83bdabd 100644 --- a/libs/binder/tests/binderRpcTestTrusty.cpp +++ b/libs/binder/tests/binderRpcTestTrusty.cpp @@ -57,9 +57,9 @@ std::unique_ptr<ProcessSession> BinderRpc::createRpcTestSocketServerProcessEtc( [](size_t n) { return n != 0; }), "Non-zero incoming connections on Trusty"); - RpcSecurity rpcSecurity = std::get<1>(GetParam()); - uint32_t clientVersion = std::get<2>(GetParam()); - uint32_t serverVersion = std::get<3>(GetParam()); + RpcSecurity rpcSecurity = GetParam().security; + uint32_t clientVersion = GetParam().clientVersion; + uint32_t serverVersion = GetParam().serverVersion; auto ret = std::make_unique<TrustyProcessSession>(); @@ -89,12 +89,27 @@ std::unique_ptr<ProcessSession> BinderRpc::createRpcTestSocketServerProcessEtc( return ret; } -INSTANTIATE_TEST_CASE_P(Trusty, BinderRpc, - ::testing::Combine(::testing::Values(SocketType::TIPC), - ::testing::Values(RpcSecurity::RAW), - ::testing::ValuesIn(testVersions()), - ::testing::ValuesIn(testVersions()), - ::testing::Values(false), ::testing::Values(true)), +static std::vector<BinderRpc::ParamType> getTrustyBinderRpcParams() { + std::vector<BinderRpc::ParamType> ret; + + for (const auto& clientVersion : testVersions()) { + for (const auto& serverVersion : testVersions()) { + ret.push_back(BinderRpc::ParamType{ + .type = SocketType::TIPC, + .security = RpcSecurity::RAW, + .clientVersion = clientVersion, + .serverVersion = serverVersion, + // TODO: should we test both versions here? + .singleThreaded = false, + .noKernel = true, + }); + } + } + + return ret; +} + +INSTANTIATE_TEST_CASE_P(Trusty, BinderRpc, ::testing::ValuesIn(getTrustyBinderRpcParams()), BinderRpc::PrintParamInfo); } // namespace android diff --git a/libs/binder/tests/binderRpcUniversalTests.cpp b/libs/binder/tests/binderRpcUniversalTests.cpp index 1f4601010c..e43508ee79 100644 --- a/libs/binder/tests/binderRpcUniversalTests.cpp +++ b/libs/binder/tests/binderRpcUniversalTests.cpp @@ -84,7 +84,7 @@ TEST_P(BinderRpc, SeparateRootObject) { GTEST_SKIP() << "This test requires a multi-threaded service"; } - SocketType type = std::get<0>(GetParam()); + SocketType type = GetParam().type; if (type == SocketType::PRECONNECTED || type == SocketType::UNIX || type == SocketType::UNIX_BOOTSTRAP || type == SocketType::UNIX_RAW) { // we can't get port numbers for unix sockets diff --git a/libs/binder/tests/parcel_fuzzer/Android.bp b/libs/binder/tests/parcel_fuzzer/Android.bp index 35866adf20..0d1503eda0 100644 --- a/libs/binder/tests/parcel_fuzzer/Android.bp +++ b/libs/binder/tests/parcel_fuzzer/Android.bp @@ -104,3 +104,28 @@ cc_library_static { local_include_dirs: ["include_random_parcel"], export_include_dirs: ["include_random_parcel"], } + +cc_library { + name: "libbinder_random_parcel_seeds", + host_supported: true, + vendor_available: true, + target: { + darwin: { + enabled: false, + }, + }, + srcs: [ + "random_parcel_seeds.cpp", + ], + shared_libs: [ + "libbase", + "libbinder", + "libbinder_ndk", + "libcutils", + "libutils", + ], + local_include_dirs: [ + "include_random_parcel_seeds", + ], + export_include_dirs: ["include_random_parcel_seeds"], +} diff --git a/libs/binder/tests/parcel_fuzzer/include_random_parcel_seeds/fuzzseeds/random_parcel_seeds.h b/libs/binder/tests/parcel_fuzzer/include_random_parcel_seeds/fuzzseeds/random_parcel_seeds.h new file mode 100644 index 0000000000..5755239c8b --- /dev/null +++ b/libs/binder/tests/parcel_fuzzer/include_random_parcel_seeds/fuzzseeds/random_parcel_seeds.h @@ -0,0 +1,47 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <android-base/file.h> +#include <android-base/hex.h> +#include <android-base/logging.h> + +#include <binder/Binder.h> +#include <binder/Parcel.h> +#include <binder/RecordedTransaction.h> + +#include <private/android_filesystem_config.h> + +#include <vector> + +using android::Parcel; +using android::base::HexString; +using std::vector; + +namespace android { +namespace impl { +// computes the bytes so that if they are passed to FuzzedDataProvider and +// provider.ConsumeIntegralInRange<T>(min, max) is called, it will return val +template <typename T> +void writeReversedBuffer(std::vector<std::byte>& integralBuffer, T min, T max, T val); + +// Calls writeInBuffer method with min and max numeric limits of type T. This method +// is reversal of ConsumeIntegral<T>() in FuzzedDataProvider +template <typename T> +void writeReversedBuffer(std::vector<std::byte>& integralBuffer, T val); +} // namespace impl +void generateSeedsFromRecording(base::borrowed_fd fd, + const binder::debug::RecordedTransaction& transaction); +} // namespace android diff --git a/libs/binder/tests/parcel_fuzzer/libbinder_driver.cpp b/libs/binder/tests/parcel_fuzzer/libbinder_driver.cpp index 45c3a90044..47d2a0a701 100644 --- a/libs/binder/tests/parcel_fuzzer/libbinder_driver.cpp +++ b/libs/binder/tests/parcel_fuzzer/libbinder_driver.cpp @@ -21,6 +21,8 @@ #include <binder/IPCThreadState.h> #include <binder/ProcessState.h> +#include <private/android_filesystem_config.h> + namespace android { void fuzzService(const sp<IBinder>& binder, FuzzedDataProvider&& provider) { @@ -33,6 +35,11 @@ void fuzzService(const std::vector<sp<IBinder>>& binders, FuzzedDataProvider&& p .extraFds = {}, }; + // Reserved bytes so that we don't have to change fuzzers and seed corpus if + // we introduce anything new in fuzzService. + std::vector<uint8_t> reservedBytes = provider.ConsumeBytes<uint8_t>(8); + (void)reservedBytes; + // always refresh the calling identity, because we sometimes set it below, but also, // the code we're fuzzing might reset it IPCThreadState::self()->clearCallingIdentity(); @@ -40,7 +47,12 @@ void fuzzService(const std::vector<sp<IBinder>>& binders, FuzzedDataProvider&& p // Always take so that a perturbation of just the one ConsumeBool byte will always // take the same path, but with a different UID. Without this, the fuzzer needs to // guess both the change in value and the shift at the same time. - int64_t maybeSetUid = provider.ConsumeIntegral<int64_t>(); + int64_t maybeSetUid = provider.PickValueInArray<int64_t>( + {static_cast<int64_t>(AID_ROOT) << 32, static_cast<int64_t>(AID_SYSTEM) << 32, + provider.ConsumeIntegralInRange<int64_t>(static_cast<int64_t>(AID_ROOT) << 32, + static_cast<int64_t>(AID_USER) << 32), + provider.ConsumeIntegral<int64_t>()}); + if (provider.ConsumeBool()) { // set calling uid IPCThreadState::self()->restoreCallingIdentity(maybeSetUid); diff --git a/libs/binder/tests/parcel_fuzzer/random_parcel_seeds.cpp b/libs/binder/tests/parcel_fuzzer/random_parcel_seeds.cpp new file mode 100644 index 0000000000..9e3e2aba77 --- /dev/null +++ b/libs/binder/tests/parcel_fuzzer/random_parcel_seeds.cpp @@ -0,0 +1,146 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <android-base/file.h> +#include <android-base/logging.h> + +#include <binder/RecordedTransaction.h> + +#include <fuzzseeds/random_parcel_seeds.h> + +using android::base::WriteFully; + +namespace android { +namespace impl { +template <typename T> +std::vector<uint8_t> reverseBytes(T min, T max, T val) { + uint64_t range = static_cast<uint64_t>(max) - min; + uint64_t result = val - min; + size_t offset = 0; + + std::vector<uint8_t> reverseData; + uint8_t reversed = 0; + reversed |= result; + + while (offset < sizeof(T) * CHAR_BIT && (range >> offset) > 0) { + reverseData.push_back(reversed); + reversed = 0; + reversed |= (result >> CHAR_BIT); + result = result >> CHAR_BIT; + offset += CHAR_BIT; + } + + return std::move(reverseData); +} + +template <typename T> +void writeReversedBuffer(std::vector<uint8_t>& integralBuffer, T min, T max, T val) { + std::vector<uint8_t> reversedData = reverseBytes(min, max, val); + // ConsumeIntegral Calls read buffer from the end. Keep inserting at the front of the buffer + // so that we can align fuzzService operations with seed generation for readability. + integralBuffer.insert(integralBuffer.begin(), reversedData.begin(), reversedData.end()); +} + +template <typename T> +void writeReversedBuffer(std::vector<uint8_t>& integralBuffer, T val) { + // For ConsumeIntegral<T>() calls, FuzzedDataProvider uses numeric limits min and max + // as range + writeReversedBuffer(integralBuffer, std::numeric_limits<T>::min(), + std::numeric_limits<T>::max(), val); +} + +} // namespace impl + +void generateSeedsFromRecording(base::borrowed_fd fd, + const binder::debug::RecordedTransaction& transaction) { + // Write Reserved bytes for future use + std::vector<uint8_t> reservedBytes(8); + CHECK(WriteFully(fd, reservedBytes.data(), reservedBytes.size())) << fd.get(); + + std::vector<uint8_t> integralBuffer; + + // Write UID array : Array elements are initialized in the order that they are declared + // UID array index 2 element + // int64_t aidRoot = 0; + impl::writeReversedBuffer(integralBuffer, static_cast<int64_t>(AID_ROOT) << 32, + static_cast<int64_t>(AID_USER) << 32, + static_cast<int64_t>(AID_ROOT) << 32); + + // UID array index 3 element + impl::writeReversedBuffer(integralBuffer, static_cast<int64_t>(AID_ROOT) << 32); + + // always pick AID_ROOT -> index 0 + size_t uidIndex = 0; + impl::writeReversedBuffer(integralBuffer, static_cast<size_t>(0), static_cast<size_t>(3), + uidIndex); + + // Never set uid in seed corpus + uint8_t writeUid = 0; + impl::writeReversedBuffer(integralBuffer, writeUid); + + // Read random code. this will be from recorded transaction + uint8_t selectCode = 1; + impl::writeReversedBuffer(integralBuffer, selectCode); + + // Get from recorded transaction + uint32_t code = transaction.getCode(); + impl::writeReversedBuffer(integralBuffer, code); + + // Get from recorded transaction + uint32_t flags = transaction.getFlags(); + impl::writeReversedBuffer(integralBuffer, flags); + + // always fuzz primary binder + size_t extraBindersIndex = 0; + impl::writeReversedBuffer(integralBuffer, static_cast<size_t>(0), static_cast<size_t>(0), + extraBindersIndex); + + const Parcel& dataParcel = transaction.getDataParcel(); + + // This buffer holds the bytes which will be used for fillRandomParcel API + std::vector<uint8_t> fillParcelBuffer; + + // Don't take rpc path + uint8_t rpcBranch = 0; + impl::writeReversedBuffer(fillParcelBuffer, rpcBranch); + + // Implicit branch on this path -> options->writeHeader(p, provider) + uint8_t writeHeaderInternal = 0; + impl::writeReversedBuffer(fillParcelBuffer, writeHeaderInternal); + + // Choose to write data in parcel + size_t fillFuncIndex = 0; + impl::writeReversedBuffer(fillParcelBuffer, static_cast<size_t>(0), static_cast<size_t>(2), + fillFuncIndex); + + // Write parcel data size from recorded transaction + size_t toWrite = transaction.getDataParcel().dataBufferSize(); + impl::writeReversedBuffer(fillParcelBuffer, static_cast<size_t>(0), toWrite, toWrite); + + // Write parcel data with size towrite from recorded transaction + CHECK(WriteFully(fd, dataParcel.data(), toWrite)) << fd.get(); + + // Write Fill Parcel buffer size in integralBuffer so that fuzzService knows size of data + size_t subDataSize = toWrite + fillParcelBuffer.size(); + impl::writeReversedBuffer(integralBuffer, static_cast<size_t>(0), subDataSize, subDataSize); + + // Write fill parcel buffer + CHECK(WriteFully(fd, fillParcelBuffer.data(), fillParcelBuffer.size())) << fd.get(); + + // Write the integralBuffer to data + CHECK(WriteFully(fd, integralBuffer.data(), integralBuffer.size())) << fd.get(); +} +} // namespace android diff --git a/libs/binder/tests/parcel_fuzzer/test_fuzzer/Android.bp b/libs/binder/tests/parcel_fuzzer/test_fuzzer/Android.bp index 690c39afc9..96092b10d3 100644 --- a/libs/binder/tests/parcel_fuzzer/test_fuzzer/Android.bp +++ b/libs/binder/tests/parcel_fuzzer/test_fuzzer/Android.bp @@ -36,8 +36,8 @@ cc_fuzz { triage_assignee: "waghpawan@google.com", // This fuzzer should be used only test fuzzService locally - fuzz_on_haiku_host: false, - fuzz_on_haiku_device: false, + fuzz_on_haiku_host: true, + fuzz_on_haiku_device: true, }, } diff --git a/libs/binder/tests/parcel_fuzzer/test_fuzzer/TestServiceFuzzer.cpp b/libs/binder/tests/parcel_fuzzer/test_fuzzer/TestServiceFuzzer.cpp index 7fbf2d0670..46205d7689 100644 --- a/libs/binder/tests/parcel_fuzzer/test_fuzzer/TestServiceFuzzer.cpp +++ b/libs/binder/tests/parcel_fuzzer/test_fuzzer/TestServiceFuzzer.cpp @@ -20,6 +20,8 @@ #include <binder/IPCThreadState.h> #include <log/log.h> +#include <private/android_filesystem_config.h> + using android::binder::Status; namespace android { @@ -29,6 +31,8 @@ enum class CrashType { ON_PLAIN, ON_BINDER, ON_KNOWN_UID, + ON_SYSTEM_AID, + ON_ROOT_AID, }; // This service is to verify that fuzzService is functioning properly @@ -48,6 +52,18 @@ public: } break; } + case CrashType::ON_SYSTEM_AID: { + if (IPCThreadState::self()->getCallingUid() == AID_SYSTEM) { + LOG_ALWAYS_FATAL("Expected crash, AID_SYSTEM."); + } + break; + } + case CrashType::ON_ROOT_AID: { + if (IPCThreadState::self()->getCallingUid() == AID_ROOT) { + LOG_ALWAYS_FATAL("Expected crash, AID_ROOT."); + } + break; + } default: break; } @@ -99,6 +115,10 @@ extern "C" int LLVMFuzzerInitialize(int* argc, char*** argv) { gCrashType = CrashType::ON_PLAIN; } else if (arg == "KNOWN_UID") { gCrashType = CrashType::ON_KNOWN_UID; + } else if (arg == "AID_SYSTEM") { + gCrashType = CrashType::ON_SYSTEM_AID; + } else if (arg == "AID_ROOT") { + gCrashType = CrashType::ON_ROOT_AID; } else if (arg == "BINDER") { gCrashType = CrashType::ON_BINDER; } else { diff --git a/libs/binder/tests/parcel_fuzzer/test_fuzzer/run_fuzz_service_test.sh b/libs/binder/tests/parcel_fuzzer/test_fuzzer/run_fuzz_service_test.sh index e568035af1..25906d8aeb 100755 --- a/libs/binder/tests/parcel_fuzzer/test_fuzzer/run_fuzz_service_test.sh +++ b/libs/binder/tests/parcel_fuzzer/test_fuzzer/run_fuzz_service_test.sh @@ -27,7 +27,7 @@ then exit 1 fi -for CRASH_TYPE in PLAIN KNOWN_UID BINDER; do +for CRASH_TYPE in PLAIN KNOWN_UID AID_SYSTEM AID_ROOT BINDER; do echo "INFO: Running fuzzer : test_service_fuzzer_should_crash $CRASH_TYPE" ./test_service_fuzzer_should_crash "$CRASH_TYPE" -max_total_time=30 &>"$FUZZER_OUT" diff --git a/libs/binder/tests/rpc_fuzzer/main.cpp b/libs/binder/tests/rpc_fuzzer/main.cpp index b8ae84d5c0..dcc8b8ebf7 100644 --- a/libs/binder/tests/rpc_fuzzer/main.cpp +++ b/libs/binder/tests/rpc_fuzzer/main.cpp @@ -135,7 +135,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { // b/260736889 - limit arbitrarily, due to thread resource exhaustion, which currently // aborts. Servers should consider RpcServer::setConnectionFilter instead. - constexpr size_t kMaxConnections = 1000; + constexpr size_t kMaxConnections = 10; while (provider.remaining_bytes() > 0) { if (connections.empty() || diff --git a/libs/binder/trusty/fuzzer/Android.bp b/libs/binder/trusty/fuzzer/Android.bp new file mode 100644 index 0000000000..2f1f54b0a6 --- /dev/null +++ b/libs/binder/trusty/fuzzer/Android.bp @@ -0,0 +1,39 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package { + default_applicable_licenses: ["Android-Apache-2.0"], +} + +cc_fuzz { + name: "trusty_binder_fuzzer", + defaults: ["trusty_fuzzer_defaults"], + srcs: [":trusty_tipc_fuzzer"], + cflags: [ + "-DTRUSTY_APP_PORT=\"com.android.trusty.binder.test.service\"", + "-DTRUSTY_APP_UUID=\"d42f06c5-9dc5-455b-9914-cf094116cfa8\"", + "-DTRUSTY_APP_FILENAME=\"binder-test-service.syms.elf\"", + ], +} + +cc_fuzz { + name: "trusty_binder_rpc_fuzzer", + defaults: ["trusty_fuzzer_defaults"], + srcs: [":trusty_tipc_fuzzer"], + cflags: [ + "-DTRUSTY_APP_PORT=\"com.android.trusty.binderRpcTestService.V0\"", + "-DTRUSTY_APP_UUID=\"87e424e5-69d7-4bbd-8b7c-7e24812cbc94\"", + "-DTRUSTY_APP_FILENAME=\"binderRpcTestService.syms.elf\"", + ], +} diff --git a/libs/fakeservicemanager/FakeServiceManager.cpp b/libs/fakeservicemanager/FakeServiceManager.cpp index 3272bbc1aa..80661c1cbd 100644 --- a/libs/fakeservicemanager/FakeServiceManager.cpp +++ b/libs/fakeservicemanager/FakeServiceManager.cpp @@ -26,6 +26,8 @@ sp<IBinder> FakeServiceManager::getService( const String16& name) const { } sp<IBinder> FakeServiceManager::checkService( const String16& name) const { + std::lock_guard<std::mutex> l(mMutex); + auto it = mNameToService.find(name); if (it == mNameToService.end()) { return nullptr; @@ -36,6 +38,8 @@ sp<IBinder> FakeServiceManager::checkService( const String16& name) const { status_t FakeServiceManager::addService(const String16& name, const sp<IBinder>& service, bool /*allowIsolated*/, int /*dumpsysFlags*/) { + std::lock_guard<std::mutex> l(mMutex); + if (service == nullptr) { return UNEXPECTED_NULL; } @@ -44,6 +48,8 @@ status_t FakeServiceManager::addService(const String16& name, const sp<IBinder>& } Vector<String16> FakeServiceManager::listServices(int /*dumpsysFlags*/) { + std::lock_guard<std::mutex> l(mMutex); + Vector<String16> services; for (auto const& [name, service] : mNameToService) { (void) service; @@ -61,10 +67,14 @@ sp<IBinder> FakeServiceManager::waitForService(const String16& name) { } bool FakeServiceManager::isDeclared(const String16& name) { + std::lock_guard<std::mutex> l(mMutex); + return mNameToService.find(name) != mNameToService.end(); } Vector<String16> FakeServiceManager::getDeclaredInstances(const String16& name) { + std::lock_guard<std::mutex> l(mMutex); + Vector<String16> out; const String16 prefix = name + String16("/"); for (const auto& [registeredName, service] : mNameToService) { @@ -108,6 +118,8 @@ std::vector<IServiceManager::ServiceDebugInfo> FakeServiceManager::getServiceDeb } void FakeServiceManager::clear() { + std::lock_guard<std::mutex> l(mMutex); + mNameToService.clear(); } } // namespace android diff --git a/libs/fakeservicemanager/include/fakeservicemanager/FakeServiceManager.h b/libs/fakeservicemanager/include/fakeservicemanager/FakeServiceManager.h index 97add24ac8..f62241d9c2 100644 --- a/libs/fakeservicemanager/include/fakeservicemanager/FakeServiceManager.h +++ b/libs/fakeservicemanager/include/fakeservicemanager/FakeServiceManager.h @@ -19,6 +19,7 @@ #include <binder/IServiceManager.h> #include <map> +#include <mutex> #include <optional> #include <vector> @@ -68,6 +69,7 @@ public: void clear(); private: + mutable std::mutex mMutex; std::map<String16, sp<IBinder>> mNameToService; }; diff --git a/libs/gui/OWNERS b/libs/gui/OWNERS index 826a418aab..070f6bf532 100644 --- a/libs/gui/OWNERS +++ b/libs/gui/OWNERS @@ -1,3 +1,5 @@ +# Bug component: 1075131 + chrisforbes@google.com jreck@google.com diff --git a/libs/renderengine/OWNERS b/libs/renderengine/OWNERS index 5d23a5ef8d..66e1aa1ca1 100644 --- a/libs/renderengine/OWNERS +++ b/libs/renderengine/OWNERS @@ -1,3 +1,5 @@ +# Bug component: 1075131 + adyabr@google.com alecmouri@google.com djsollen@google.com diff --git a/libs/sensor/SensorManager.cpp b/libs/sensor/SensorManager.cpp index 40061cde61..9f814f1c48 100644 --- a/libs/sensor/SensorManager.cpp +++ b/libs/sensor/SensorManager.cpp @@ -176,11 +176,8 @@ status_t SensorManager::assertStateLocked() { mSensors = mSensorServer->getSensorList(mOpPackageName); size_t count = mSensors.size(); - if (count == 0) { - ALOGE("Failed to get Sensor list"); - mSensorServer.clear(); - return UNKNOWN_ERROR; - } + // If count is 0, mSensorList will be non-null. This is old + // existing behavior and callers expect this. mSensorList = static_cast<Sensor const**>(malloc(count * sizeof(Sensor*))); LOG_ALWAYS_FATAL_IF(mSensorList == nullptr, "mSensorList NULL"); diff --git a/libs/ui/DebugUtils.cpp b/libs/ui/DebugUtils.cpp index 073da89758..8675f14d43 100644 --- a/libs/ui/DebugUtils.cpp +++ b/libs/ui/DebugUtils.cpp @@ -304,6 +304,12 @@ std::string decodePixelFormat(android::PixelFormat format) { return std::string("BGRA_8888"); case android::PIXEL_FORMAT_R_8: return std::string("R_8"); + case android::PIXEL_FORMAT_R_16_UINT: + return std::string("R_16_UINT"); + case android::PIXEL_FORMAT_RG_1616_UINT: + return std::string("RG_1616_UINT"); + case android::PIXEL_FORMAT_RGBA_10101010: + return std::string("RGBA_10101010"); default: return StringPrintf("Unknown %#08x", format); } diff --git a/libs/ui/include/ui/FatVector.h b/libs/ui/include/ui/FatVector.h index cb61e6a320..494272b1a8 100644 --- a/libs/ui/include/ui/FatVector.h +++ b/libs/ui/include/ui/FatVector.h @@ -65,6 +65,17 @@ public: free(p); } } + + // The STL checks that this member type is present so that + // std::allocator_traits<InlineStdAllocator<T, SIZE>>::rebind_alloc<Other> + // works. std::vector won't be able to construct an + // InlineStdAllocator<Other, SIZE>, because InlineStdAllocator has no + // default constructor, but vector presumably doesn't rebind the allocator + // because it doesn't allocate internal node types. + template <class Other> + struct rebind { + typedef InlineStdAllocator<Other, SIZE> other; + }; Allocation& mAllocation; }; diff --git a/services/inputflinger/OWNERS b/services/inputflinger/OWNERS index c88bfe97ca..21d208f577 100644 --- a/services/inputflinger/OWNERS +++ b/services/inputflinger/OWNERS @@ -1 +1,2 @@ +# Bug component: 136048 include platform/frameworks/base:/INPUT_OWNERS diff --git a/services/sensorservice/SensorFusion.cpp b/services/sensorservice/SensorFusion.cpp index e27b52b23e..5c00260008 100644 --- a/services/sensorservice/SensorFusion.cpp +++ b/services/sensorservice/SensorFusion.cpp @@ -19,6 +19,7 @@ #include "SensorService.h" #include <android/util/ProtoOutputStream.h> +#include <cutils/properties.h> #include <frameworks/base/core/proto/android/service/sensor_service.proto.h> namespace android { @@ -60,10 +61,12 @@ SensorFusion::SensorFusion() mGyro = uncalibratedGyro; } - // 200 Hz for gyro events is a good compromise between precision - // and power/cpu usage. - mEstimatedGyroRate = 200; - mTargetDelayNs = 1000000000LL/mEstimatedGyroRate; + // Wearable devices will want to tune this parameter + // to 100 (Hz) in device.mk to save some power. + int32_t value = property_get_int32( + "sensors.aosp_low_power_sensor_fusion.maximum_rate", 200); + mEstimatedGyroRate = static_cast<float>(value); + mTargetDelayNs = 1000000000LL / mEstimatedGyroRate; for (int i = 0; i<NUM_FUSION_MODE; ++i) { mFusions[i].init(i); diff --git a/services/sensorservice/tests/sensorservicetest.cpp b/services/sensorservice/tests/sensorservicetest.cpp index caf7f03361..1406bb38e7 100644 --- a/services/sensorservice/tests/sensorservicetest.cpp +++ b/services/sensorservice/tests/sensorservicetest.cpp @@ -130,7 +130,7 @@ int main() { printf("ALOOPER_POLL_TIMEOUT\n"); break; case ALOOPER_POLL_ERROR: - printf("ALOOPER_POLL_TIMEOUT\n"); + printf("ALOOPER_POLL_ERROR\n"); break; default: printf("ugh? poll returned %d\n", ret); diff --git a/services/surfaceflinger/OWNERS b/services/surfaceflinger/OWNERS index 4e7da829f2..3270e4c95c 100644 --- a/services/surfaceflinger/OWNERS +++ b/services/surfaceflinger/OWNERS @@ -1,6 +1,9 @@ +# Bug component: 1075131 + adyabr@google.com alecmouri@google.com chaviw@google.com +domlaskowski@google.com lpy@google.com pdwilliams@google.com racarr@google.com diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 8c46515bf1..64348a544c 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -6755,7 +6755,7 @@ ftl::SharedFuture<FenceResult> SurfaceFlinger::captureScreenCommon( if (captureListener) { // TODO: The future returned by std::async blocks the main thread. Return a chain of // futures to the Binder thread instead. - std::async([=]() mutable { + (void)std::async([=]() mutable { ATRACE_NAME("captureListener is nonnull!"); auto fenceResult = renderFuture.get(); // TODO(b/232535621): Change ScreenCaptureResults to store a FenceResult. diff --git a/services/vibratorservice/OWNERS b/services/vibratorservice/OWNERS index d073e2bd46..031b333fab 100644 --- a/services/vibratorservice/OWNERS +++ b/services/vibratorservice/OWNERS @@ -1 +1,3 @@ +# Bug component: 345036 + include platform/frameworks/base:/services/core/java/com/android/server/vibrator/OWNERS diff --git a/vulkan/libvulkan/swapchain.cpp b/vulkan/libvulkan/swapchain.cpp index c46036a5e2..64393be3fe 100644 --- a/vulkan/libvulkan/swapchain.cpp +++ b/vulkan/libvulkan/swapchain.cpp @@ -16,6 +16,7 @@ #define ATRACE_TAG ATRACE_TAG_GRAPHICS +#include <aidl/android/hardware/graphics/common/PixelFormat.h> #include <android/hardware/graphics/common/1.0/types.h> #include <grallocusage/GrallocUsageConversion.h> #include <graphicsenv/GraphicsEnv.h> @@ -25,8 +26,6 @@ #include <sync/sync.h> #include <system/window.h> #include <ui/BufferQueueDefs.h> -#include <ui/DebugUtils.h> -#include <ui/PixelFormat.h> #include <utils/StrongPointer.h> #include <utils/Timers.h> #include <utils/Trace.h> @@ -37,6 +36,7 @@ #include "driver.h" +using PixelFormat = aidl::android::hardware::graphics::common::PixelFormat; using android::hardware::graphics::common::V1_0::BufferUsage; namespace vulkan { @@ -489,27 +489,27 @@ void copy_ready_timings(Swapchain& swapchain, *count = num_copied; } -android::PixelFormat GetNativePixelFormat(VkFormat format) { - android::PixelFormat native_format = android::PIXEL_FORMAT_RGBA_8888; +PixelFormat GetNativePixelFormat(VkFormat format) { + PixelFormat native_format = PixelFormat::RGBA_8888; switch (format) { case VK_FORMAT_R8G8B8A8_UNORM: case VK_FORMAT_R8G8B8A8_SRGB: - native_format = android::PIXEL_FORMAT_RGBA_8888; + native_format = PixelFormat::RGBA_8888; break; case VK_FORMAT_R5G6B5_UNORM_PACK16: - native_format = android::PIXEL_FORMAT_RGB_565; + native_format = PixelFormat::RGB_565; break; case VK_FORMAT_R16G16B16A16_SFLOAT: - native_format = android::PIXEL_FORMAT_RGBA_FP16; + native_format = PixelFormat::RGBA_FP16; break; case VK_FORMAT_A2B10G10R10_UNORM_PACK32: - native_format = android::PIXEL_FORMAT_RGBA_1010102; + native_format = PixelFormat::RGBA_1010102; break; case VK_FORMAT_R8_UNORM: - native_format = android::PIXEL_FORMAT_R_8; + native_format = PixelFormat::R_8; break; case VK_FORMAT_R10X6G10X6B10X6A10X6_UNORM_4PACK16: - native_format = android::PIXEL_FORMAT_RGBA_10101010; + native_format = PixelFormat::RGBA_10101010; break; default: ALOGV("unsupported swapchain format %d", format); @@ -1256,7 +1256,7 @@ VkResult CreateSwapchainKHR(VkDevice device, if (!allocator) allocator = &GetData(device).allocator; - android::PixelFormat native_pixel_format = + PixelFormat native_pixel_format = GetNativePixelFormat(create_info->imageFormat); android_dataspace native_dataspace = GetNativeDataspace(create_info->imageColorSpace); @@ -1351,10 +1351,11 @@ VkResult CreateSwapchainKHR(VkDevice device, const auto& dispatch = GetData(device).driver; - err = native_window_set_buffers_format(window, native_pixel_format); + err = native_window_set_buffers_format( + window, static_cast<int>(native_pixel_format)); if (err != android::OK) { ALOGE("native_window_set_buffers_format(%s) failed: %s (%d)", - decodePixelFormat(native_pixel_format).c_str(), strerror(-err), err); + toString(native_pixel_format).c_str(), strerror(-err), err); return VK_ERROR_SURFACE_LOST_KHR; } |