diff options
40 files changed, 816 insertions, 112 deletions
diff --git a/cmds/installd/otapreopt_script.sh b/cmds/installd/otapreopt_script.sh index b7ad33144b..5690e2fe42 100644 --- a/cmds/installd/otapreopt_script.sh +++ b/cmds/installd/otapreopt_script.sh @@ -23,6 +23,9 @@ TARGET_SLOT="$1" STATUS_FD="$2" +# "1" if the script is triggered by the `UpdateEngine.triggerPostinstall` API. Empty otherwise. +TRIGGERED_BY_API="$3" + # Maximum number of packages/steps. MAXIMUM_PACKAGES=1000 @@ -53,25 +56,43 @@ fi # A source that infinitely emits arbitrary lines. # When connected to STDIN of another process, this source keeps STDIN open until # the consumer process closes STDIN or this script dies. +# In practice, the pm command keeps consuming STDIN, so we don't need to worry +# about running out of buffer space. function infinite_source { while echo .; do sleep 1 done } +if [[ "$TRIGGERED_BY_API" = "1" ]]; then + # During OTA installation, the script is called the first time, and + # `TRIGGERED_BY_API` can never be "1". `TRIGGERED_BY_API` being "1" means this + # is the second call to this script, through the + # `UpdateEngine.triggerPostinstall` API. + # When we reach here, it means Pre-reboot Dexopt is enabled in asynchronous + # mode and the job scheduler determined that it's the time to run the job. + # Start Pre-reboot Dexopt now and wait for it to finish. + infinite_source | pm art on-ota-staged --start + exit $? +fi + PR_DEXOPT_JOB_VERSION="$(pm art pr-dexopt-job --version)" if (( $? == 0 )) && (( $PR_DEXOPT_JOB_VERSION >= 3 )); then # Delegate to Pre-reboot Dexopt, a feature of ART Service. # ART Service decides what to do with this request: # - If Pre-reboot Dexopt is disabled or unsupported, the command returns - # non-zero. This is always the case if the current system is Android 14 or - # earlier. + # non-zero. + # This is always the case if the current system is Android 14 or earlier. # - If Pre-reboot Dexopt is enabled in synchronous mode, the command blocks - # until Pre-reboot Dexopt finishes, and returns zero no matter it succeeds or - # not. This is the default behavior if the current system is Android 15. - # - If Pre-reboot Dexopt is enabled in asynchronous mode, the command schedules - # an asynchronous job and returns 0 immediately. The job will then run by the - # job scheduler when the device is idle and charging. + # until Pre-reboot Dexopt finishes, and returns zero no matter it succeeds + # or not. + # This is the default behavior if the current system is Android 15. + # - If Pre-reboot Dexopt is enabled in asynchronous mode, the command + # schedules an asynchronous job and returns 0 immediately. + # Later, when the device is idle and charging, the job will be run by the + # job scheduler. It will call this script again through the + # `UpdateEngine.triggerPostinstall` API, with `TRIGGERED_BY_API` being "1". + # This is always the case if the current system is Android 16 or later. if infinite_source | pm art on-ota-staged --slot "$TARGET_SLOT_SUFFIX"; then # Handled by Pre-reboot Dexopt. exit 0 diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp index 0a22588a90..bc7ae37ff0 100644 --- a/libs/binder/Binder.cpp +++ b/libs/binder/Binder.cpp @@ -38,6 +38,7 @@ #endif #include "BuildFlags.h" +#include "Constants.h" #include "OS.h" #include "RpcState.h" @@ -70,8 +71,6 @@ constexpr bool kEnableRecording = true; constexpr bool kEnableRecording = false; #endif -// Log any reply transactions for which the data exceeds this size -#define LOG_REPLIES_OVER_SIZE (300 * 1024) // --------------------------------------------------------------------------- IBinder::IBinder() @@ -412,7 +411,7 @@ status_t BBinder::transact( // In case this is being transacted on in the same process. if (reply != nullptr) { reply->setDataPosition(0); - if (reply->dataSize() > LOG_REPLIES_OVER_SIZE) { + if (reply->dataSize() > binder::kLogTransactionsOverBytes) { ALOGW("Large reply transaction of %zu bytes, interface descriptor %s, code %d", reply->dataSize(), String8(getInterfaceDescriptor()).c_str(), code); } diff --git a/libs/binder/BpBinder.cpp b/libs/binder/BpBinder.cpp index 444f06174e..c13e0f9e9c 100644 --- a/libs/binder/BpBinder.cpp +++ b/libs/binder/BpBinder.cpp @@ -28,6 +28,7 @@ #include <stdio.h> #include "BuildFlags.h" +#include "Constants.h" #include "file.h" //#undef ALOGV @@ -63,9 +64,6 @@ std::atomic<uint32_t> BpBinder::sBinderProxyCountWarned(0); static constexpr uint32_t kBinderProxyCountWarnInterval = 5000; -// Log any transactions for which the data exceeds this size -#define LOG_TRANSACTIONS_OVER_SIZE (300 * 1024) - enum { LIMIT_REACHED_MASK = 0x80000000, // A flag denoting that the limit has been reached WARNING_REACHED_MASK = 0x40000000, // A flag denoting that the warning has been reached @@ -403,9 +401,11 @@ status_t BpBinder::transact( status = IPCThreadState::self()->transact(binderHandle(), code, data, reply, flags); } - if (data.dataSize() > LOG_TRANSACTIONS_OVER_SIZE) { + + if (data.dataSize() > binder::kLogTransactionsOverBytes) { RpcMutexUniqueLock _l(mLock); - ALOGW("Large outgoing transaction of %zu bytes, interface descriptor %s, code %d", + ALOGW("Large outgoing transaction of %zu bytes, interface descriptor %s, code %d was " + "sent", data.dataSize(), String8(mDescriptorCache).c_str(), code); } diff --git a/libs/binder/Constants.h b/libs/binder/Constants.h new file mode 100644 index 0000000000..b75493cb8a --- /dev/null +++ b/libs/binder/Constants.h @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2025 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. + */ + +#pragma once + +namespace android::binder { + +/** + * See also BINDER_VM_SIZE. In kernel binder, the sum of all transactions must be allocated in this + * space. Large transactions are very error prone. In general, we should work to reduce this limit. + * The same limit is used in RPC binder for consistency. + */ +constexpr size_t kLogTransactionsOverBytes = 300 * 1024; + +/** + * See b/392575419 - this limit is chosen for a specific usecase, because RPC binder does not have + * support for shared memory in the Android Baklava timeframe. This was 100 KB during and before + * Android V. + * + * Keeping this low helps preserve overall system performance. Transactions of this size are far too + * expensive to make multiple copies over binder or sockets, and they should be avoided if at all + * possible and transition to shared memory. + */ +constexpr size_t kRpcTransactionLimitBytes = 600 * 1024; + +} // namespace android::binder diff --git a/libs/binder/IPCThreadState.cpp b/libs/binder/IPCThreadState.cpp index 623e7b9139..f7e0915f15 100644 --- a/libs/binder/IPCThreadState.cpp +++ b/libs/binder/IPCThreadState.cpp @@ -38,6 +38,10 @@ #include "Utils.h" #include "binder_module.h" +#if (defined(__ANDROID__) || defined(__Fuchsia__)) && !defined(BINDER_WITH_KERNEL_IPC) +#error Android and Fuchsia are expected to have BINDER_WITH_KERNEL_IPC +#endif + #if LOG_NDEBUG #define IF_LOG_TRANSACTIONS() if (false) @@ -1215,7 +1219,7 @@ status_t IPCThreadState::talkWithDriver(bool doReceive) std::string message = logStream.str(); ALOGI("%s", message.c_str()); } -#if defined(__ANDROID__) +#if defined(BINDER_WITH_KERNEL_IPC) if (ioctl(mProcess->mDriverFD, BINDER_WRITE_READ, &bwr) >= 0) err = NO_ERROR; else @@ -1604,7 +1608,7 @@ void IPCThreadState::threadDestructor(void *st) IPCThreadState* const self = static_cast<IPCThreadState*>(st); if (self) { self->flushCommands(); -#if defined(__ANDROID__) +#if defined(BINDER_WITH_KERNEL_IPC) if (self->mProcess->mDriverFD >= 0) { ioctl(self->mProcess->mDriverFD, BINDER_THREAD_EXIT, 0); } @@ -1620,7 +1624,7 @@ status_t IPCThreadState::getProcessFreezeInfo(pid_t pid, uint32_t *sync_received binder_frozen_status_info info = {}; info.pid = pid; -#if defined(__ANDROID__) +#if defined(BINDER_WITH_KERNEL_IPC) if (ioctl(self()->mProcess->mDriverFD, BINDER_GET_FROZEN_INFO, &info) < 0) ret = -errno; #endif @@ -1639,7 +1643,7 @@ status_t IPCThreadState::freeze(pid_t pid, bool enable, uint32_t timeout_ms) { info.timeout_ms = timeout_ms; -#if defined(__ANDROID__) +#if defined(BINDER_WITH_KERNEL_IPC) if (ioctl(self()->mProcess->mDriverFD, BINDER_FREEZE, &info) < 0) ret = -errno; #endif @@ -1657,7 +1661,7 @@ void IPCThreadState::logExtendedError() { if (!ProcessState::isDriverFeatureEnabled(ProcessState::DriverFeature::EXTENDED_ERROR)) return; -#if defined(__ANDROID__) +#if defined(BINDER_WITH_KERNEL_IPC) if (ioctl(self()->mProcess->mDriverFD, BINDER_GET_EXTENDED_ERROR, &ee) < 0) { ALOGE("Failed to get extended error: %s", strerror(errno)); return; diff --git a/libs/binder/IServiceManager.cpp b/libs/binder/IServiceManager.cpp index 719e445794..c9ca646472 100644 --- a/libs/binder/IServiceManager.cpp +++ b/libs/binder/IServiceManager.cpp @@ -43,7 +43,11 @@ #include <binder/IPermissionController.h> #endif -#ifdef __ANDROID__ +#if !(defined(__ANDROID__) || defined(__FUCHSIA)) +#define BINDER_SERVICEMANAGEMENT_DELEGATION_SUPPORT +#endif + +#if !defined(BINDER_SERVICEMANAGEMENT_DELEGATION_SUPPORT) #include <cutils/properties.h> #else #include "ServiceManagerHost.h" @@ -902,7 +906,7 @@ std::vector<IServiceManager::ServiceDebugInfo> CppBackendShim::getServiceDebugIn return ret; } -#ifndef __ANDROID__ +#if defined(BINDER_SERVICEMANAGEMENT_DELEGATION_SUPPORT) // CppBackendShim for host. Implements the old libbinder android::IServiceManager API. // The internal implementation of the AIDL interface android::os::IServiceManager calls into // on-device service manager. diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 65c6553e44..1e83c350b1 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -299,8 +299,13 @@ status_t Parcel::flattenBinder(const sp<IBinder>& binder) { obj.handle = handle; obj.cookie = 0; } else { +#if __linux__ int policy = local->getMinSchedulerPolicy(); int priority = local->getMinSchedulerPriority(); +#else + int policy = 0; + int priority = 0; +#endif if (policy != 0 || priority != 0) { // override value, since it is set explicitly diff --git a/libs/binder/ProcessState.cpp b/libs/binder/ProcessState.cpp index 0e1e9b4127..0bec37999b 100644 --- a/libs/binder/ProcessState.cpp +++ b/libs/binder/ProcessState.cpp @@ -48,6 +48,10 @@ #define DEFAULT_MAX_BINDER_THREADS 15 #define DEFAULT_ENABLE_ONEWAY_SPAM_DETECTION 1 +#if defined(__ANDROID__) || defined(__Fuchsia__) +#define EXPECT_BINDER_OPEN_SUCCESS +#endif + #ifdef __ANDROID_VNDK__ const char* kDefaultDriver = "/dev/vndbinder"; #else @@ -613,7 +617,7 @@ ProcessState::ProcessState(const char* driver) } } -#ifdef __ANDROID__ +#if defined(EXPECT_BINDER_OPEN_SUCCESS) LOG_ALWAYS_FATAL_IF(!opened.ok(), "Binder driver '%s' could not be opened. Error: %s. Terminating.", driver, error.c_str()); diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp index fe6e1a3318..03d974d186 100644 --- a/libs/binder/RpcState.cpp +++ b/libs/binder/RpcState.cpp @@ -23,6 +23,7 @@ #include <binder/IPCThreadState.h> #include <binder/RpcServer.h> +#include "Constants.h" #include "Debug.h" #include "RpcWireFormat.h" #include "Utils.h" @@ -337,6 +338,8 @@ std::string RpcState::BinderNode::toString() const { } RpcState::CommandData::CommandData(size_t size) : mSize(size) { + if (size == 0) return; + // The maximum size for regular binder is 1MB for all concurrent // transactions. A very small proportion of transactions are even // larger than a page, but we need to avoid allocating too much @@ -348,11 +351,11 @@ RpcState::CommandData::CommandData(size_t size) : mSize(size) { // transaction (in some cases, additional fixed size amounts are added), // though for rough consistency, we should avoid cases where this data type // is used for multiple dynamic allocations for a single transaction. - constexpr size_t kMaxTransactionAllocation = 100 * 1000; - if (size == 0) return; - if (size > kMaxTransactionAllocation) { - ALOGW("Transaction requested too much data allocation %zu", size); + if (size > binder::kRpcTransactionLimitBytes) { + ALOGE("Transaction requested too much data allocation: %zu bytes, failing.", size); return; + } else if (size > binder::kLogTransactionsOverBytes) { + ALOGW("Transaction too large: inefficient and in danger of breaking: %zu bytes.", size); } mData.reset(new (std::nothrow) uint8_t[size]); } diff --git a/libs/binder/ndk/include_platform/android/binder_stability.h b/libs/binder/ndk/include_platform/android/binder_stability.h index 089c775eca..80502056ee 100644 --- a/libs/binder/ndk/include_platform/android/binder_stability.h +++ b/libs/binder/ndk/include_platform/android/binder_stability.h @@ -27,6 +27,10 @@ constexpr binder_flags_t FLAG_PRIVATE_VENDOR = 0x10000000; #if defined(__ANDROID_VENDOR__) +#if defined(__ANDROID_PRODUCT__) +#error "build bug: product is not part of the vendor half of the Treble system/vendor split" +#endif + /** * Private addition to binder_flag_t. */ diff --git a/libs/binder/rust/Android.bp b/libs/binder/rust/Android.bp index 8404a48c26..adef9ea64b 100644 --- a/libs/binder/rust/Android.bp +++ b/libs/binder/rust/Android.bp @@ -16,6 +16,7 @@ rust_library { "libdowncast_rs", "liblibc", "liblog_rust", + "libzerocopy", ], host_supported: true, vendor_available: true, @@ -205,6 +206,7 @@ rust_test { "libdowncast_rs", "liblibc", "liblog_rust", + "libzerocopy", ], } diff --git a/libs/binder/rust/rpcbinder/Android.bp b/libs/binder/rust/rpcbinder/Android.bp index 4036551ef3..46651ceb48 100644 --- a/libs/binder/rust/rpcbinder/Android.bp +++ b/libs/binder/rust/rpcbinder/Android.bp @@ -26,6 +26,7 @@ rust_library { ], visibility: [ "//device/google/cuttlefish/shared/minidroid/sample", + "//hardware/interfaces/security/see:__subpackages__", "//packages/modules/Virtualization:__subpackages__", ], apex_available: [ diff --git a/libs/binder/rust/src/binder.rs b/libs/binder/rust/src/binder.rs index 8c0501ba2f..6a8a69843a 100644 --- a/libs/binder/rust/src/binder.rs +++ b/libs/binder/rust/src/binder.rs @@ -1201,5 +1201,45 @@ macro_rules! declare_binder_enum { Ok(v.map(|v| v.into_iter().map(Self).collect())) } } + + impl std::ops::BitOr for $enum { + type Output = Self; + fn bitor(self, rhs: Self) -> Self { + Self(self.0 | rhs.0) + } + } + + impl std::ops::BitOrAssign for $enum { + fn bitor_assign(&mut self, rhs: Self) { + self.0 = self.0 | rhs.0; + } + } + + impl std::ops::BitAnd for $enum { + type Output = Self; + fn bitand(self, rhs: Self) -> Self { + Self(self.0 & rhs.0) + } + } + + impl std::ops::BitAndAssign for $enum { + fn bitand_assign(&mut self, rhs: Self) { + self.0 = self.0 & rhs.0; + } + } + + impl std::ops::BitXor for $enum { + type Output = Self; + fn bitxor(self, rhs: Self) -> Self { + Self(self.0 ^ rhs.0) + } + } + + impl std::ops::BitXorAssign for $enum { + fn bitxor_assign(&mut self, rhs: Self) { + self.0 = self.0 ^ rhs.0; + } + } + }; } diff --git a/libs/binder/rust/src/lib.rs b/libs/binder/rust/src/lib.rs index 77b80fe1ec..0026f213f2 100644 --- a/libs/binder/rust/src/lib.rs +++ b/libs/binder/rust/src/lib.rs @@ -116,7 +116,7 @@ pub use binder::{BinderFeatures, FromIBinder, IBinder, Interface, Strong, Weak}; pub use error::{ExceptionCode, IntoBinderResult, Status, StatusCode}; pub use parcel::{ParcelFileDescriptor, Parcelable, ParcelableHolder}; #[cfg(not(trusty))] -pub use persistable_bundle::PersistableBundle; +pub use persistable_bundle::{PersistableBundle, ValueType}; pub use proxy::{DeathRecipient, SpIBinder, WpIBinder}; #[cfg(not(any(trusty, android_ndk)))] pub use service::{ diff --git a/libs/binder/rust/src/persistable_bundle.rs b/libs/binder/rust/src/persistable_bundle.rs index d71ed73532..8639c0d08c 100644 --- a/libs/binder/rust/src/persistable_bundle.rs +++ b/libs/binder/rust/src/persistable_bundle.rs @@ -22,19 +22,28 @@ use crate::{ }; use binder_ndk_sys::{ APersistableBundle, APersistableBundle_delete, APersistableBundle_dup, - APersistableBundle_erase, APersistableBundle_getBoolean, APersistableBundle_getBooleanVector, - APersistableBundle_getDouble, APersistableBundle_getDoubleVector, APersistableBundle_getInt, - APersistableBundle_getIntVector, APersistableBundle_getLong, APersistableBundle_getLongVector, - APersistableBundle_getPersistableBundle, APersistableBundle_isEqual, APersistableBundle_new, + APersistableBundle_erase, APersistableBundle_getBoolean, APersistableBundle_getBooleanKeys, + APersistableBundle_getBooleanVector, APersistableBundle_getBooleanVectorKeys, + APersistableBundle_getDouble, APersistableBundle_getDoubleKeys, + APersistableBundle_getDoubleVector, APersistableBundle_getDoubleVectorKeys, + APersistableBundle_getInt, APersistableBundle_getIntKeys, APersistableBundle_getIntVector, + APersistableBundle_getIntVectorKeys, APersistableBundle_getLong, + APersistableBundle_getLongKeys, APersistableBundle_getLongVector, + APersistableBundle_getLongVectorKeys, APersistableBundle_getPersistableBundle, + APersistableBundle_getPersistableBundleKeys, APersistableBundle_getString, + APersistableBundle_getStringKeys, APersistableBundle_getStringVector, + APersistableBundle_getStringVectorKeys, APersistableBundle_isEqual, APersistableBundle_new, APersistableBundle_putBoolean, APersistableBundle_putBooleanVector, APersistableBundle_putDouble, APersistableBundle_putDoubleVector, APersistableBundle_putInt, APersistableBundle_putIntVector, APersistableBundle_putLong, APersistableBundle_putLongVector, APersistableBundle_putPersistableBundle, APersistableBundle_putString, APersistableBundle_putStringVector, APersistableBundle_readFromParcel, APersistableBundle_size, - APersistableBundle_writeToParcel, APERSISTABLEBUNDLE_KEY_NOT_FOUND, + APersistableBundle_writeToParcel, APERSISTABLEBUNDLE_ALLOCATOR_FAILED, + APERSISTABLEBUNDLE_KEY_NOT_FOUND, }; -use std::ffi::{c_char, CString, NulError}; -use std::ptr::{null_mut, NonNull}; +use std::ffi::{c_char, c_void, CStr, CString, NulError}; +use std::ptr::{null_mut, slice_from_raw_parts_mut, NonNull}; +use zerocopy::FromZeros; /// A mapping from string keys to values of various types. #[derive(Debug)] @@ -374,6 +383,53 @@ impl PersistableBundle { } } + /// Gets the string value associated with the given key. + /// + /// Returns an error if the key contains a NUL character, or `Ok(None)` if the key doesn't exist + /// in the bundle. + pub fn get_string(&self, key: &str) -> Result<Option<String>, NulError> { + let key = CString::new(key)?; + let mut value = null_mut(); + let mut allocated_size: usize = 0; + // SAFETY: The wrapped `APersistableBundle` pointer is guaranteed to be valid for the + // lifetime of the `PersistableBundle`. The pointer returned by `key.as_ptr()` is guaranteed + // to be valid for the lifetime of `key`. The value pointer must be valid because it comes + // from a reference. + let value_size_bytes = unsafe { + APersistableBundle_getString( + self.0.as_ptr(), + key.as_ptr(), + &mut value, + Some(string_allocator), + (&raw mut allocated_size).cast(), + ) + }; + match value_size_bytes { + APERSISTABLEBUNDLE_KEY_NOT_FOUND => Ok(None), + APERSISTABLEBUNDLE_ALLOCATOR_FAILED => { + panic!("APersistableBundle_getString failed to allocate string"); + } + _ => { + let raw_slice = slice_from_raw_parts_mut(value.cast(), allocated_size); + // SAFETY: The pointer was returned from string_allocator, which used + // `Box::into_raw`, and we've got the appropriate size back from allocated_size. + let boxed_slice: Box<[u8]> = unsafe { Box::from_raw(raw_slice) }; + assert_eq!( + allocated_size, + usize::try_from(value_size_bytes) + .expect("APersistableBundle_getString returned negative value size") + + 1 + ); + let c_string = CString::from_vec_with_nul(boxed_slice.into()) + .expect("APersistableBundle_getString returned string missing NUL byte"); + let string = c_string + .into_string() + .expect("APersistableBundle_getString returned invalid UTF-8"); + Ok(Some(string)) + } + } + } + /// Gets the vector of `T` associated with the given key. /// /// Returns an error if the key contains a NUL character, or `Ok(None)` if the key doesn't exist @@ -388,9 +444,10 @@ impl PersistableBundle { /// call. It must allow a null pointer for the buffer, and must return the size in bytes of /// buffer it requires. If it is given a non-null buffer pointer it must write that number of /// bytes to the buffer, which must be a whole number of valid `T` values. - unsafe fn get_vec<T: Clone + Default>( + unsafe fn get_vec<T: Clone>( &self, key: &str, + default: T, get_func: unsafe extern "C" fn( *const APersistableBundle, *const c_char, @@ -404,9 +461,12 @@ impl PersistableBundle { // to be valid for the lifetime of `key`. A null pointer is allowed for the buffer. match unsafe { get_func(self.0.as_ptr(), key.as_ptr(), null_mut(), 0) } { APERSISTABLEBUNDLE_KEY_NOT_FOUND => Ok(None), + APERSISTABLEBUNDLE_ALLOCATOR_FAILED => { + panic!("APersistableBundle_getStringVector failed to allocate string"); + } required_buffer_size => { let mut value = vec![ - T::default(); + default; usize::try_from(required_buffer_size).expect( "APersistableBundle_get*Vector returned invalid size" ) / size_of::<T>() @@ -426,6 +486,9 @@ impl PersistableBundle { APERSISTABLEBUNDLE_KEY_NOT_FOUND => { panic!("APersistableBundle_get*Vector failed to find key after first finding it"); } + APERSISTABLEBUNDLE_ALLOCATOR_FAILED => { + panic!("APersistableBundle_getStringVector failed to allocate string"); + } _ => Ok(Some(value)), } } @@ -439,7 +502,7 @@ impl PersistableBundle { pub fn get_bool_vec(&self, key: &str) -> Result<Option<Vec<bool>>, NulError> { // SAFETY: APersistableBundle_getBooleanVector fulfils all the safety requirements of // `get_vec`. - unsafe { self.get_vec(key, APersistableBundle_getBooleanVector) } + unsafe { self.get_vec(key, Default::default(), APersistableBundle_getBooleanVector) } } /// Gets the i32 vector value associated with the given key. @@ -449,7 +512,7 @@ impl PersistableBundle { pub fn get_int_vec(&self, key: &str) -> Result<Option<Vec<i32>>, NulError> { // SAFETY: APersistableBundle_getIntVector fulfils all the safety requirements of // `get_vec`. - unsafe { self.get_vec(key, APersistableBundle_getIntVector) } + unsafe { self.get_vec(key, Default::default(), APersistableBundle_getIntVector) } } /// Gets the i64 vector value associated with the given key. @@ -459,7 +522,7 @@ impl PersistableBundle { pub fn get_long_vec(&self, key: &str) -> Result<Option<Vec<i64>>, NulError> { // SAFETY: APersistableBundle_getLongVector fulfils all the safety requirements of // `get_vec`. - unsafe { self.get_vec(key, APersistableBundle_getLongVector) } + unsafe { self.get_vec(key, Default::default(), APersistableBundle_getLongVector) } } /// Gets the f64 vector value associated with the given key. @@ -469,7 +532,45 @@ impl PersistableBundle { pub fn get_double_vec(&self, key: &str) -> Result<Option<Vec<f64>>, NulError> { // SAFETY: APersistableBundle_getDoubleVector fulfils all the safety requirements of // `get_vec`. - unsafe { self.get_vec(key, APersistableBundle_getDoubleVector) } + unsafe { self.get_vec(key, Default::default(), APersistableBundle_getDoubleVector) } + } + + /// Gets the string vector value associated with the given key. + /// + /// Returns an error if the key contains a NUL character, or `Ok(None)` if the key doesn't exist + /// in the bundle. + pub fn get_string_vec(&self, key: &str) -> Result<Option<Vec<String>>, NulError> { + if let Some(value) = + // SAFETY: `get_string_vector_with_allocator` fulfils all the safety requirements of + // `get_vec`. + unsafe { self.get_vec(key, null_mut(), get_string_vector_with_allocator) }? + { + Ok(Some( + value + .into_iter() + .map(|s| { + // SAFETY: The pointer was returned from `string_allocator`, which used + // `Box::into_raw`, and `APersistableBundle_getStringVector` should have + // written valid bytes to it including a NUL terminator in the last + // position. + let string_length = unsafe { CStr::from_ptr(s) }.count_bytes(); + let raw_slice = slice_from_raw_parts_mut(s.cast(), string_length + 1); + // SAFETY: The pointer was returned from `string_allocator`, which used + // `Box::into_raw`, and we've got the appropriate size back by checking the + // length of the string. + let boxed_slice: Box<[u8]> = unsafe { Box::from_raw(raw_slice) }; + let c_string = CString::from_vec_with_nul(boxed_slice.into()).expect( + "APersistableBundle_getStringVector returned string missing NUL byte", + ); + c_string + .into_string() + .expect("APersistableBundle_getStringVector returned invalid UTF-8") + }) + .collect(), + )) + } else { + Ok(None) + } } /// Gets the `PersistableBundle` value associated with the given key. @@ -493,6 +594,201 @@ impl PersistableBundle { Ok(None) } } + + /// Calls the appropriate `APersistableBundle_get*Keys` function for the given `value_type`, + /// with our `string_allocator` and a null context pointer. + /// + /// # Safety + /// + /// `out_keys` must either be null or point to a buffer of at least `buffer_size_bytes` bytes, + /// properly aligned for `T`, and not otherwise accessed for the duration of the call. + unsafe fn get_keys_raw( + &self, + value_type: ValueType, + out_keys: *mut *mut c_char, + buffer_size_bytes: i32, + ) -> i32 { + // SAFETY: The wrapped `APersistableBundle` pointer is guaranteed to be valid for the + // lifetime of the `PersistableBundle`. Our caller guarantees an appropriate value for + // `out_keys` and `buffer_size_bytes`. + unsafe { + match value_type { + ValueType::Boolean => APersistableBundle_getBooleanKeys( + self.0.as_ptr(), + out_keys, + buffer_size_bytes, + Some(string_allocator), + null_mut(), + ), + ValueType::Integer => APersistableBundle_getIntKeys( + self.0.as_ptr(), + out_keys, + buffer_size_bytes, + Some(string_allocator), + null_mut(), + ), + ValueType::Long => APersistableBundle_getLongKeys( + self.0.as_ptr(), + out_keys, + buffer_size_bytes, + Some(string_allocator), + null_mut(), + ), + ValueType::Double => APersistableBundle_getDoubleKeys( + self.0.as_ptr(), + out_keys, + buffer_size_bytes, + Some(string_allocator), + null_mut(), + ), + ValueType::String => APersistableBundle_getStringKeys( + self.0.as_ptr(), + out_keys, + buffer_size_bytes, + Some(string_allocator), + null_mut(), + ), + ValueType::BooleanVector => APersistableBundle_getBooleanVectorKeys( + self.0.as_ptr(), + out_keys, + buffer_size_bytes, + Some(string_allocator), + null_mut(), + ), + ValueType::IntegerVector => APersistableBundle_getIntVectorKeys( + self.0.as_ptr(), + out_keys, + buffer_size_bytes, + Some(string_allocator), + null_mut(), + ), + ValueType::LongVector => APersistableBundle_getLongVectorKeys( + self.0.as_ptr(), + out_keys, + buffer_size_bytes, + Some(string_allocator), + null_mut(), + ), + ValueType::DoubleVector => APersistableBundle_getDoubleVectorKeys( + self.0.as_ptr(), + out_keys, + buffer_size_bytes, + Some(string_allocator), + null_mut(), + ), + ValueType::StringVector => APersistableBundle_getStringVectorKeys( + self.0.as_ptr(), + out_keys, + buffer_size_bytes, + Some(string_allocator), + null_mut(), + ), + ValueType::PersistableBundle => APersistableBundle_getPersistableBundleKeys( + self.0.as_ptr(), + out_keys, + buffer_size_bytes, + Some(string_allocator), + null_mut(), + ), + } + } + } + + /// Gets all the keys associated with values of the given type. + pub fn keys_for_type(&self, value_type: ValueType) -> Vec<String> { + // SAFETY: A null pointer is allowed for the buffer. + match unsafe { self.get_keys_raw(value_type, null_mut(), 0) } { + APERSISTABLEBUNDLE_ALLOCATOR_FAILED => { + panic!("APersistableBundle_get*Keys failed to allocate string"); + } + required_buffer_size => { + let required_buffer_size_usize = usize::try_from(required_buffer_size) + .expect("APersistableBundle_get*Keys returned invalid size"); + assert_eq!(required_buffer_size_usize % size_of::<*mut c_char>(), 0); + let mut keys = + vec![null_mut(); required_buffer_size_usize / size_of::<*mut c_char>()]; + // SAFETY: The wrapped `APersistableBundle` pointer is guaranteed to be valid for + // the lifetime of the `PersistableBundle`. The keys buffer pointer is valid as it + // comes from the Vec we just allocated. + if unsafe { self.get_keys_raw(value_type, keys.as_mut_ptr(), required_buffer_size) } + == APERSISTABLEBUNDLE_ALLOCATOR_FAILED + { + panic!("APersistableBundle_get*Keys failed to allocate string"); + } + keys.into_iter() + .map(|key| { + // SAFETY: The pointer was returned from `string_allocator`, which used + // `Box::into_raw`, and `APersistableBundle_getStringVector` should have + // written valid bytes to it including a NUL terminator in the last + // position. + let string_length = unsafe { CStr::from_ptr(key) }.count_bytes(); + let raw_slice = slice_from_raw_parts_mut(key.cast(), string_length + 1); + // SAFETY: The pointer was returned from `string_allocator`, which used + // `Box::into_raw`, and we've got the appropriate size back by checking the + // length of the string. + let boxed_slice: Box<[u8]> = unsafe { Box::from_raw(raw_slice) }; + let c_string = CString::from_vec_with_nul(boxed_slice.into()) + .expect("APersistableBundle_get*Keys returned string missing NUL byte"); + c_string + .into_string() + .expect("APersistableBundle_get*Keys returned invalid UTF-8") + }) + .collect() + } + } + } + + /// Returns an iterator over all keys in the bundle, along with the type of their associated + /// value. + pub fn keys(&self) -> impl Iterator<Item = (String, ValueType)> + use<'_> { + [ + ValueType::Boolean, + ValueType::Integer, + ValueType::Long, + ValueType::Double, + ValueType::String, + ValueType::BooleanVector, + ValueType::IntegerVector, + ValueType::LongVector, + ValueType::DoubleVector, + ValueType::StringVector, + ValueType::PersistableBundle, + ] + .iter() + .flat_map(|value_type| { + self.keys_for_type(*value_type).into_iter().map(|key| (key, *value_type)) + }) + } +} + +/// Wrapper around `APersistableBundle_getStringVector` to pass `string_allocator` and a null +/// context pointer. +/// +/// # Safety +/// +/// * `bundle` must point to a valid `APersistableBundle` which is not modified for the duration of +/// the call. +/// * `key` must point to a valid NUL-terminated C string. +/// * `buffer` must either be null or point to a buffer of at least `buffer_size_bytes` bytes, +/// properly aligned for `T`, and not otherwise accessed for the duration of the call. +unsafe extern "C" fn get_string_vector_with_allocator( + bundle: *const APersistableBundle, + key: *const c_char, + buffer: *mut *mut c_char, + buffer_size_bytes: i32, +) -> i32 { + // SAFETY: The safety requirements are all guaranteed by our caller according to the safety + // documentation above. + unsafe { + APersistableBundle_getStringVector( + bundle, + key, + buffer, + buffer_size_bytes, + Some(string_allocator), + null_mut(), + ) + } } // SAFETY: The underlying *APersistableBundle can be moved between threads. @@ -558,9 +854,59 @@ impl UnstructuredParcelable for PersistableBundle { } } +/// Allocates a boxed slice of the given size in bytes, returns a pointer to it and writes its size +/// to `*context`. +/// +/// # Safety +/// +/// `context` must either be null or point to a `usize` to which we can write. +unsafe extern "C" fn string_allocator(size: i32, context: *mut c_void) -> *mut c_char { + let Ok(size) = size.try_into() else { + return null_mut(); + }; + let Ok(boxed_slice) = <[c_char]>::new_box_zeroed_with_elems(size) else { + return null_mut(); + }; + if !context.is_null() { + // SAFETY: The caller promised that `context` is either null or points to a `usize` to which + // we can write, and we just checked that it's not null. + unsafe { + *context.cast::<usize>() = size; + } + } + Box::into_raw(boxed_slice).cast() +} + impl_deserialize_for_unstructured_parcelable!(PersistableBundle); impl_serialize_for_unstructured_parcelable!(PersistableBundle); +/// The types which may be stored as values in a [`PersistableBundle`]. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum ValueType { + /// A `bool`. + Boolean, + /// An `i32`. + Integer, + /// An `i64`. + Long, + /// An `f64`. + Double, + /// A string. + String, + /// A vector of `bool`s. + BooleanVector, + /// A vector of `i32`s. + IntegerVector, + /// A vector of `i64`s. + LongVector, + /// A vector of `f64`s. + DoubleVector, + /// A vector of strings. + StringVector, + /// A nested `PersistableBundle`. + PersistableBundle, +} + #[cfg(test)] mod test { use super::*; @@ -589,6 +935,7 @@ mod test { assert_eq!(bundle.get_int_vec("foo"), Ok(None)); assert_eq!(bundle.get_long_vec("foo"), Ok(None)); assert_eq!(bundle.get_double_vec("foo"), Ok(None)); + assert_eq!(bundle.get_string("foo"), Ok(None)); } #[test] @@ -639,10 +986,15 @@ mod test { } #[test] - fn insert_string() { + fn insert_get_string() { let mut bundle = PersistableBundle::new(); + assert_eq!(bundle.insert_string("string", "foo"), Ok(())); - assert_eq!(bundle.size(), 1); + assert_eq!(bundle.insert_string("empty", ""), Ok(())); + assert_eq!(bundle.size(), 2); + + assert_eq!(bundle.get_string("string"), Ok(Some("foo".to_string()))); + assert_eq!(bundle.get_string("empty"), Ok(Some("".to_string()))); } #[test] @@ -675,6 +1027,10 @@ mod test { assert_eq!(bundle.get_int_vec("int"), Ok(Some(vec![42]))); assert_eq!(bundle.get_long_vec("long"), Ok(Some(vec![66, 67, 68]))); assert_eq!(bundle.get_double_vec("double"), Ok(Some(vec![123.4]))); + assert_eq!( + bundle.get_string_vec("string"), + Ok(Some(vec!["foo".to_string(), "bar".to_string(), "baz".to_string()])) + ); } #[test] @@ -688,4 +1044,33 @@ mod test { assert_eq!(bundle.get_persistable_bundle("bundle"), Ok(Some(sub_bundle))); } + + #[test] + fn get_keys() { + let mut bundle = PersistableBundle::new(); + + assert_eq!(bundle.keys_for_type(ValueType::Boolean), Vec::<String>::new()); + assert_eq!(bundle.keys_for_type(ValueType::Integer), Vec::<String>::new()); + assert_eq!(bundle.keys_for_type(ValueType::StringVector), Vec::<String>::new()); + + assert_eq!(bundle.insert_bool("bool1", false), Ok(())); + assert_eq!(bundle.insert_bool("bool2", true), Ok(())); + assert_eq!(bundle.insert_int("int", 42), Ok(())); + + assert_eq!( + bundle.keys_for_type(ValueType::Boolean), + vec!["bool1".to_string(), "bool2".to_string()] + ); + assert_eq!(bundle.keys_for_type(ValueType::Integer), vec!["int".to_string()]); + assert_eq!(bundle.keys_for_type(ValueType::StringVector), Vec::<String>::new()); + + assert_eq!( + bundle.keys().collect::<Vec<_>>(), + vec![ + ("bool1".to_string(), ValueType::Boolean), + ("bool2".to_string(), ValueType::Boolean), + ("int".to_string(), ValueType::Integer), + ] + ); + } } diff --git a/libs/binder/tests/IBinderRpcTest.aidl b/libs/binder/tests/IBinderRpcTest.aidl index 116476765a..dcd6461b53 100644 --- a/libs/binder/tests/IBinderRpcTest.aidl +++ b/libs/binder/tests/IBinderRpcTest.aidl @@ -34,6 +34,8 @@ interface IBinderRpcTest { void holdBinder(@nullable IBinder binder); @nullable IBinder getHeldBinder(); + byte[] repeatBytes(in byte[] bytes); + // Idea is client creates its own instance of IBinderRpcTest and calls this, // and the server calls 'binder' with (calls - 1) passing itself as 'binder', // going back and forth until calls = 0 diff --git a/libs/binder/tests/binderAllocationLimits.cpp b/libs/binder/tests/binderAllocationLimits.cpp index c0c0aae80b..339ce4bdf8 100644 --- a/libs/binder/tests/binderAllocationLimits.cpp +++ b/libs/binder/tests/binderAllocationLimits.cpp @@ -22,17 +22,33 @@ #include <binder/RpcServer.h> #include <binder/RpcSession.h> #include <cutils/trace.h> +#include <gtest/gtest-spi.h> #include <gtest/gtest.h> #include <utils/CallStack.h> #include <malloc.h> +#include <atomic> #include <functional> +#include <numeric> #include <vector> using namespace android::binder::impl; static android::String8 gEmpty(""); // make sure first allocation from optimization runs +struct State { + State(std::vector<size_t>&& expectedMallocs) : expectedMallocs(std::move(expectedMallocs)) {} + ~State() { + size_t num = numMallocs.load(); + if (expectedMallocs.size() != num) { + ADD_FAILURE() << "Expected " << expectedMallocs.size() << " allocations, but got " + << num; + } + } + const std::vector<size_t> expectedMallocs; + std::atomic<size_t> numMallocs; +}; + struct DestructionAction { DestructionAction(std::function<void()> f) : mF(std::move(f)) {} ~DestructionAction() { mF(); }; @@ -95,8 +111,7 @@ namespace LambdaHooks { // Action to execute when malloc is hit. Supports nesting. Malloc is not // restricted when the allocation hook is being processed. -__attribute__((warn_unused_result)) -DestructionAction OnMalloc(LambdaHooks::AllocationHook f) { +__attribute__((warn_unused_result)) DestructionAction OnMalloc(LambdaHooks::AllocationHook f) { MallocHooks before = MallocHooks::save(); LambdaHooks::lambdas.emplace_back(std::move(f)); LambdaHooks::lambda_malloc_hooks.overwrite(); @@ -106,6 +121,22 @@ DestructionAction OnMalloc(LambdaHooks::AllocationHook f) { }); } +DestructionAction setExpectedMallocs(std::vector<size_t>&& expected) { + auto state = std::make_shared<State>(std::move(expected)); + return OnMalloc([state = state](size_t bytes) { + size_t num = state->numMallocs.fetch_add(1); + if (num >= state->expectedMallocs.size() || state->expectedMallocs[num] != bytes) { + ADD_FAILURE() << "Unexpected allocation number " << num << " of size " << bytes + << " bytes" << std::endl + << android::CallStack::stackToString("UNEXPECTED ALLOCATION", + android::CallStack::getCurrent( + 4 /*ignoreDepth*/) + .get()) + << std::endl; + } + }); +} + // exported symbol, to force compiler not to optimize away pointers we set here const void* imaginary_use; @@ -119,16 +150,53 @@ TEST(TestTheTest, OnMalloc) { imaginary_use = new int[10]; } + delete[] reinterpret_cast<const int*>(imaginary_use); EXPECT_EQ(mallocs, 1u); } +TEST(TestTheTest, OnMallocWithExpectedMallocs) { + std::vector<size_t> expectedMallocs = { + 4, + 16, + 8, + }; + { + const auto on_malloc = setExpectedMallocs(std::move(expectedMallocs)); + imaginary_use = new int32_t[1]; + delete[] reinterpret_cast<const int*>(imaginary_use); + imaginary_use = new int32_t[4]; + delete[] reinterpret_cast<const int*>(imaginary_use); + imaginary_use = new int32_t[2]; + delete[] reinterpret_cast<const int*>(imaginary_use); + } +} + +TEST(TestTheTest, OnMallocWithExpectedMallocsWrongSize) { + std::vector<size_t> expectedMallocs = { + 4, + 16, + 100000, + }; + EXPECT_NONFATAL_FAILURE( + { + const auto on_malloc = setExpectedMallocs(std::move(expectedMallocs)); + imaginary_use = new int32_t[1]; + delete[] reinterpret_cast<const int*>(imaginary_use); + imaginary_use = new int32_t[4]; + delete[] reinterpret_cast<const int*>(imaginary_use); + imaginary_use = new int32_t[2]; + delete[] reinterpret_cast<const int*>(imaginary_use); + }, + "Unexpected allocation number 2 of size 8 bytes"); +} __attribute__((warn_unused_result)) DestructionAction ScopeDisallowMalloc() { return OnMalloc([&](size_t bytes) { - ADD_FAILURE() << "Unexpected allocation: " << bytes; + FAIL() << "Unexpected allocation: " << bytes; using android::CallStack; - std::cout << CallStack::stackToString("UNEXPECTED ALLOCATION", CallStack::getCurrent(4 /*ignoreDepth*/).get()) + std::cout << CallStack::stackToString("UNEXPECTED ALLOCATION", + CallStack::getCurrent(4 /*ignoreDepth*/).get()) << std::endl; }); } @@ -224,6 +292,51 @@ TEST(BinderAllocation, SmallTransaction) { EXPECT_EQ(mallocs, 1u); } +TEST(BinderAccessorAllocation, AddAccessorCheckService) { + // Need to call defaultServiceManager() before checking malloc because it + // will allocate an instance in the call_once + const auto sm = defaultServiceManager(); + const std::string kInstanceName1 = "foo.bar.IFoo/default"; + const std::string kInstanceName2 = "foo.bar.IFoo2/default"; + const String16 kInstanceName16(kInstanceName1.c_str()); + std::vector<size_t> expectedMallocs = { + // addAccessorProvider + 112, // new AccessorProvider + 16, // new AccessorProviderEntry + // checkService + 45, // String8 from String16 in CppShim::checkService + 128, // writeInterfaceToken + 16, // getInjectedAccessor, new AccessorProviderEntry + 66, // getInjectedAccessor, String16 + 45, // String8 from String16 in AccessorProvider::provide + }; + std::set<std::string> supportedInstances = {kInstanceName1, kInstanceName2}; + auto onMalloc = setExpectedMallocs(std::move(expectedMallocs)); + + auto receipt = + android::addAccessorProvider(std::move(supportedInstances), + [&](const String16&) -> sp<IBinder> { return nullptr; }); + EXPECT_FALSE(receipt.expired()); + + sp<IBinder> binder = sm->checkService(kInstanceName16); + + status_t status = android::removeAccessorProvider(receipt); +} + +TEST(BinderAccessorAllocation, AddAccessorEmpty) { + std::vector<size_t> expectedMallocs = { + 48, // From ALOGE with empty set of instances + }; + std::set<std::string> supportedInstances = {}; + auto onMalloc = setExpectedMallocs(std::move(expectedMallocs)); + + auto receipt = + android::addAccessorProvider(std::move(supportedInstances), + [&](const String16&) -> sp<IBinder> { return nullptr; }); + + EXPECT_TRUE(receipt.expired()); +} + TEST(RpcBinderAllocation, SetupRpcServer) { std::string tmp = getenv("TMPDIR") ?: "/tmp"; std::string addr = tmp + "/binderRpcBenchmark"; @@ -255,6 +368,7 @@ TEST(RpcBinderAllocation, SetupRpcServer) { } int main(int argc, char** argv) { + LOG(INFO) << "Priming static log variables for binderAllocationLimits."; if (getenv("LIBC_HOOKS_ENABLE") == nullptr) { CHECK(0 == setenv("LIBC_HOOKS_ENABLE", "1", true /*overwrite*/)); execv(argv[0], argv); diff --git a/libs/binder/tests/binderRpcTest.cpp b/libs/binder/tests/binderRpcTest.cpp index 9f656ec96c..170b2adfbb 100644 --- a/libs/binder/tests/binderRpcTest.cpp +++ b/libs/binder/tests/binderRpcTest.cpp @@ -711,6 +711,35 @@ TEST_P(BinderRpc, OnewayCallExhaustion) { proc.proc->sessions.erase(proc.proc->sessions.begin() + 1); } +// TODO(b/392717039): can we move this to universal tests? +TEST_P(BinderRpc, SendTooLargeVector) { + if (GetParam().singleThreaded) { + GTEST_SKIP() << "Requires multi-threaded server to test one of the sessions crashing."; + } + + auto proc = createRpcTestSocketServerProcess({.numSessions = 2}); + + // need a working transaction + EXPECT_EQ(OK, proc.rootBinder->pingBinder()); + + // see libbinder internal Constants.h + const size_t kTooLargeSize = 650 * 1024; + const std::vector<uint8_t> kTestValue(kTooLargeSize / sizeof(uint8_t), 42); + + // TODO(b/392717039): Telling a server to allocate too much data currently causes the session to + // close since RpcServer treats any transaction error as a failure. We likely want to change + // this behavior to be a soft failure, since it isn't hard to keep track of this state. + sp<IBinderRpcTest> rootIface2 = interface_cast<IBinderRpcTest>(proc.proc->sessions.at(1).root); + std::vector<uint8_t> result; + status_t res = rootIface2->repeatBytes(kTestValue, &result).transactionError(); + + // TODO(b/392717039): consistent error results always + EXPECT_TRUE(res == -ECONNRESET || res == DEAD_OBJECT) << statusToString(res); + + // died, so remove it for checks in destructor of proc + proc.proc->sessions.erase(proc.proc->sessions.begin() + 1); +} + TEST_P(BinderRpc, SessionWithIncomingThreadpoolDoesntLeak) { if (clientOrServerSingleThreaded()) { GTEST_SKIP() << "This test requires multiple threads"; @@ -2742,7 +2771,9 @@ INSTANTIATE_TEST_SUITE_P( int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); +#ifndef __ANDROID__ __android_log_set_logger(__android_log_stderr_logger); +#endif return RUN_ALL_TESTS(); } diff --git a/libs/binder/tests/binderRpcTestCommon.h b/libs/binder/tests/binderRpcTestCommon.h index dc22647b85..6e0024628a 100644 --- a/libs/binder/tests/binderRpcTestCommon.h +++ b/libs/binder/tests/binderRpcTestCommon.h @@ -348,6 +348,10 @@ public: *out = binder; return Status::ok(); } + Status repeatBytes(const std::vector<uint8_t>& bytes, std::vector<uint8_t>* out) override { + *out = bytes; + return Status::ok(); + } static sp<IBinder> mHeldBinder; Status holdBinder(const sp<IBinder>& binder) override { mHeldBinder = binder; diff --git a/libs/binder/tests/binderRpcTestService.cpp b/libs/binder/tests/binderRpcTestService.cpp index aef946481f..0084b9ada2 100644 --- a/libs/binder/tests/binderRpcTestService.cpp +++ b/libs/binder/tests/binderRpcTestService.cpp @@ -100,7 +100,9 @@ public: }; int main(int argc, char* argv[]) { +#ifndef __ANDROID__ __android_log_set_logger(__android_log_stderr_logger); +#endif LOG_ALWAYS_FATAL_IF(argc != 3, "Invalid number of arguments: %d", argc); unique_fd writeEnd(atoi(argv[1])); diff --git a/libs/binder/tests/binderRpcUniversalTests.cpp b/libs/binder/tests/binderRpcUniversalTests.cpp index 4b9dcf85d2..d227e6eeec 100644 --- a/libs/binder/tests/binderRpcUniversalTests.cpp +++ b/libs/binder/tests/binderRpcUniversalTests.cpp @@ -209,6 +209,18 @@ TEST_P(BinderRpc, RepeatBinder) { EXPECT_EQ(0, MyBinderRpcSession::gNum); } +TEST_P(BinderRpc, SendLargeVector) { + auto proc = createRpcTestSocketServerProcess({}); + + // see libbinder internal Constants.h + const size_t kLargeSize = 550 * 1024; + const std::vector<uint8_t> kTestValue(kLargeSize / sizeof(uint8_t), 42); + + std::vector<uint8_t> result; + EXPECT_OK(proc.rootIface->repeatBytes(kTestValue, &result)); + EXPECT_EQ(result, kTestValue); +} + TEST_P(BinderRpc, RepeatTheirBinder) { auto proc = createRpcTestSocketServerProcess({}); diff --git a/libs/binder/tests/binderStabilityIntegrationTest.cpp b/libs/binder/tests/binderStabilityIntegrationTest.cpp index a3fc9cc2c8..cbc4180480 100644 --- a/libs/binder/tests/binderStabilityIntegrationTest.cpp +++ b/libs/binder/tests/binderStabilityIntegrationTest.cpp @@ -47,6 +47,7 @@ TEST_P(BinderStabilityIntegrationTest, ExpectedStabilityForItsPartition) { Stability::Level level = Stability::Level::UNDECLARED; switch (partition) { + case Partition::PRODUCT: case Partition::SYSTEM: case Partition::SYSTEM_EXT: level = Stability::Level::SYSTEM; diff --git a/libs/binder/trusty/binderRpcTest/manifest.json b/libs/binder/trusty/binderRpcTest/manifest.json index 6e20b8a7ad..da0f2ed2d8 100644 --- a/libs/binder/trusty/binderRpcTest/manifest.json +++ b/libs/binder/trusty/binderRpcTest/manifest.json @@ -1,6 +1,6 @@ { "uuid": "9dbe9fb8-60fd-4bdd-af86-03e95d7ad78b", "app_name": "binderRpcTest", - "min_heap": 262144, + "min_heap": 4194304, "min_stack": 20480 } diff --git a/libs/binder/trusty/binderRpcTest/service/manifest.json b/libs/binder/trusty/binderRpcTest/service/manifest.json index d2a1fc0a48..55ff49c0b8 100644 --- a/libs/binder/trusty/binderRpcTest/service/manifest.json +++ b/libs/binder/trusty/binderRpcTest/service/manifest.json @@ -1,7 +1,7 @@ { "uuid": "87e424e5-69d7-4bbd-8b7c-7e24812cbc94", "app_name": "binderRpcTestService", - "min_heap": 65536, + "min_heap": 4194304, "min_stack": 20480, "mgmt_flags": { "restart_on_exit": true, diff --git a/libs/binder/trusty/rust/binder_rpc_test/binder_rpc_test_session/lib.rs b/libs/binder/trusty/rust/binder_rpc_test/binder_rpc_test_session/lib.rs index 22cba44975..caf3117195 100644 --- a/libs/binder/trusty/rust/binder_rpc_test/binder_rpc_test_session/lib.rs +++ b/libs/binder/trusty/rust/binder_rpc_test/binder_rpc_test_session/lib.rs @@ -82,6 +82,9 @@ impl IBinderRpcTest for MyBinderRpcSession { fn repeatBinder(&self, _binder: Option<&SpIBinder>) -> Result<Option<SpIBinder>, Status> { todo!() } + fn repeatBytes(&self, _bytes: &[u8]) -> Result<Vec<u8>, Status> { + todo!() + } fn holdBinder(&self, _binder: Option<&SpIBinder>) -> Result<(), Status> { todo!() } diff --git a/libs/binder/trusty/rust/binder_rpc_test/service/main.rs b/libs/binder/trusty/rust/binder_rpc_test/service/main.rs index c4a758a214..6f454be2b6 100644 --- a/libs/binder/trusty/rust/binder_rpc_test/service/main.rs +++ b/libs/binder/trusty/rust/binder_rpc_test/service/main.rs @@ -96,6 +96,9 @@ impl IBinderRpcTest for TestService { None => Err(Status::from(StatusCode::BAD_VALUE)), } } + fn repeatBytes(&self, _bytes: &[u8]) -> Result<Vec<u8>, Status> { + todo!() + } fn holdBinder(&self, binder: Option<&SpIBinder>) -> Result<(), Status> { *HOLD_BINDER.lock().unwrap() = binder.cloned(); Ok(()) diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 25e6a52ed1..ba82046388 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -210,8 +210,6 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, bool updateDestinati ComposerServiceAIDL::getComposerService()->getMaxAcquiredBufferCount(&mMaxAcquiredBuffers); mBufferItemConsumer->setMaxAcquiredBufferCount(mMaxAcquiredBuffers); mCurrentMaxAcquiredBufferCount = mMaxAcquiredBuffers; - mNumAcquired = 0; - mNumFrameAvailable = 0; TransactionCompletedListener::getInstance()->addQueueStallListener( [&](const std::string& reason) { @@ -436,7 +434,7 @@ void BLASTBufferQueue::transactionCallback(nsecs_t /*latchTime*/, const sp<Fence void BLASTBufferQueue::flushShadowQueue() { BQA_LOGV("flushShadowQueue"); - int numFramesToFlush = mNumFrameAvailable; + int32_t numFramesToFlush = mNumFrameAvailable; while (numFramesToFlush > 0) { acquireNextBufferLocked(std::nullopt); numFramesToFlush--; diff --git a/libs/gui/include/gui/InputTransferToken.h b/libs/gui/include/gui/InputTransferToken.h index 6530b5069a..fb4aaa73ae 100644 --- a/libs/gui/include/gui/InputTransferToken.h +++ b/libs/gui/include/gui/InputTransferToken.h @@ -25,7 +25,7 @@ namespace android { struct InputTransferToken : public RefBase, Parcelable { public: - InputTransferToken() { mToken = new BBinder(); } + InputTransferToken() { mToken = sp<BBinder>::make(); } InputTransferToken(const sp<IBinder>& token) { mToken = token; } @@ -50,4 +50,4 @@ static inline bool operator==(const sp<InputTransferToken>& token1, return token1->mToken == token2->mToken; } -} // namespace android
\ No newline at end of file +} // namespace android diff --git a/libs/renderengine/OWNERS b/libs/renderengine/OWNERS index 17ab29fd29..e296283f26 100644 --- a/libs/renderengine/OWNERS +++ b/libs/renderengine/OWNERS @@ -7,4 +7,3 @@ jreck@google.com lpy@google.com nscobie@google.com sallyqi@google.com -scroggo@google.com diff --git a/libs/renderengine/skia/Cache.cpp b/libs/renderengine/skia/Cache.cpp index 57041ee6a1..3b0f03671b 100644 --- a/libs/renderengine/skia/Cache.cpp +++ b/libs/renderengine/skia/Cache.cpp @@ -337,17 +337,17 @@ static void drawImageDimmedLayers(SkiaRenderEngine* renderengine, const DisplayS LayerSettings layer{ .geometry = Geometry{ + .boundaries = rect, // The position transform doesn't matter when the reduced shader mode // in in effect. A matrix transform stage is always included. .positionTransform = mat4(), - .boundaries = rect, - .roundedCornersCrop = rect, .roundedCornersRadius = {0.f, 0.f}, + .roundedCornersCrop = rect, }, .source = PixelSource{.buffer = Buffer{.buffer = srcTexture, - .maxLuminanceNits = 1000.f, .usePremultipliedAlpha = true, - .isOpaque = true}}, + .isOpaque = true, + .maxLuminanceNits = 1000.f}}, .alpha = 1.f, .sourceDataspace = kDestDataSpace, }; @@ -370,16 +370,16 @@ static void drawTransparentImageDimmedLayers(SkiaRenderEngine* renderengine, LayerSettings layer{ .geometry = Geometry{ - .positionTransform = mat4(), .boundaries = rect, + .positionTransform = mat4(), .roundedCornersCrop = rect, }, .source = PixelSource{.buffer = Buffer{ .buffer = srcTexture, - .maxLuminanceNits = 1000.f, .usePremultipliedAlpha = true, .isOpaque = false, + .maxLuminanceNits = 1000.f, }}, .sourceDataspace = kDestDataSpace, }; @@ -421,17 +421,17 @@ static void drawClippedDimmedImageLayers(SkiaRenderEngine* renderengine, LayerSettings layer{ .geometry = Geometry{ - .positionTransform = mat4(), .boundaries = boundary, - .roundedCornersCrop = rect, + .positionTransform = mat4(), .roundedCornersRadius = {27.f, 27.f}, + .roundedCornersCrop = rect, }, .source = PixelSource{.buffer = Buffer{ .buffer = srcTexture, - .maxLuminanceNits = 1000.f, .usePremultipliedAlpha = true, .isOpaque = false, + .maxLuminanceNits = 1000.f, }}, .alpha = 1.f, .sourceDataspace = kDestDataSpace, @@ -489,17 +489,17 @@ static void drawBT2020ImageLayers(SkiaRenderEngine* renderengine, const DisplayS LayerSettings layer{ .geometry = Geometry{ + .boundaries = rect, // The position transform doesn't matter when the reduced shader mode // in in effect. A matrix transform stage is always included. .positionTransform = mat4(), - .boundaries = rect, - .roundedCornersCrop = rect, .roundedCornersRadius = {0.f, 0.f}, + .roundedCornersCrop = rect, }, .source = PixelSource{.buffer = Buffer{.buffer = srcTexture, - .maxLuminanceNits = 1000.f, .usePremultipliedAlpha = true, - .isOpaque = true}}, + .isOpaque = true, + .maxLuminanceNits = 1000.f}}, .alpha = 1.f, .sourceDataspace = kBT2020DataSpace, }; @@ -527,17 +527,17 @@ static void drawBT2020ClippedImageLayers(SkiaRenderEngine* renderengine, LayerSettings layer{ .geometry = Geometry{ - .positionTransform = kScaleAsymmetric, .boundaries = boundary, - .roundedCornersCrop = rect, + .positionTransform = kScaleAsymmetric, .roundedCornersRadius = {64.1f, 64.1f}, + .roundedCornersCrop = rect, }, .source = PixelSource{.buffer = Buffer{ .buffer = srcTexture, - .maxLuminanceNits = 1000.f, .usePremultipliedAlpha = true, .isOpaque = true, + .maxLuminanceNits = 1000.f, }}, .alpha = 0.5f, .sourceDataspace = kBT2020DataSpace, @@ -556,17 +556,17 @@ static void drawExtendedHDRImageLayers(SkiaRenderEngine* renderengine, LayerSettings layer{ .geometry = Geometry{ + .boundaries = rect, // The position transform doesn't matter when the reduced shader mode // in in effect. A matrix transform stage is always included. .positionTransform = mat4(), - .boundaries = rect, - .roundedCornersCrop = rect, .roundedCornersRadius = {50.f, 50.f}, + .roundedCornersCrop = rect, }, .source = PixelSource{.buffer = Buffer{.buffer = srcTexture, - .maxLuminanceNits = 1000.f, .usePremultipliedAlpha = true, - .isOpaque = true}}, + .isOpaque = true, + .maxLuminanceNits = 1000.f}}, .alpha = 0.5f, .sourceDataspace = kExtendedHdrDataSpce, }; @@ -594,17 +594,17 @@ static void drawP3ImageLayers(SkiaRenderEngine* renderengine, const DisplaySetti LayerSettings layer{ .geometry = Geometry{ + .boundaries = rect, // The position transform doesn't matter when the reduced shader mode // in in effect. A matrix transform stage is always included. .positionTransform = mat4(), - .boundaries = rect, - .roundedCornersCrop = rect, .roundedCornersRadius = {50.f, 50.f}, + .roundedCornersCrop = rect, }, .source = PixelSource{.buffer = Buffer{.buffer = srcTexture, - .maxLuminanceNits = 1000.f, .usePremultipliedAlpha = true, - .isOpaque = false}}, + .isOpaque = false, + .maxLuminanceNits = 1000.f}}, .alpha = 0.5f, .sourceDataspace = kOtherDataSpace, }; diff --git a/libs/renderengine/skia/debug/CaptureTimer.cpp b/libs/renderengine/skia/debug/CaptureTimer.cpp index 11bcdb8996..1c1ee0abb3 100644 --- a/libs/renderengine/skia/debug/CaptureTimer.cpp +++ b/libs/renderengine/skia/debug/CaptureTimer.cpp @@ -30,7 +30,7 @@ namespace skia { void CaptureTimer::setTimeout(TimeoutCallback function, std::chrono::milliseconds delay) { this->clear = false; - CommonPool::post([=]() { + CommonPool::post([=,this]() { if (this->clear) return; std::this_thread::sleep_for(delay); if (this->clear) return; diff --git a/libs/shaders/OWNERS b/libs/shaders/OWNERS index 6d91da3bd2..6977a49cc9 100644 --- a/libs/shaders/OWNERS +++ b/libs/shaders/OWNERS @@ -1,4 +1,3 @@ alecmouri@google.com jreck@google.com sallyqi@google.com -scroggo@google.com
\ No newline at end of file diff --git a/libs/tonemap/OWNERS b/libs/tonemap/OWNERS index 6d91da3bd2..6977a49cc9 100644 --- a/libs/tonemap/OWNERS +++ b/libs/tonemap/OWNERS @@ -1,4 +1,3 @@ alecmouri@google.com jreck@google.com sallyqi@google.com -scroggo@google.com
\ No newline at end of file diff --git a/libs/ui/OWNERS b/libs/ui/OWNERS index a0b5fe7119..2a85a4b493 100644 --- a/libs/ui/OWNERS +++ b/libs/ui/OWNERS @@ -2,6 +2,5 @@ adyabr@google.com alecmouri@google.com chrisforbes@google.com jreck@google.com -lpy@google.com mathias@google.com romainguy@google.com diff --git a/opengl/tools/glgen/stubs/egl/eglCreateWindowSurface.cpp b/opengl/tools/glgen/stubs/egl/eglCreateWindowSurface.cpp index 7c255ed106..5ba72affc0 100644 --- a/opengl/tools/glgen/stubs/egl/eglCreateWindowSurface.cpp +++ b/opengl/tools/glgen/stubs/egl/eglCreateWindowSurface.cpp @@ -113,7 +113,7 @@ not_valid_surface: if (producer == NULL) goto not_valid_surface; - window = new android::Surface(producer, true); + window = android::sp<android::Surface>::make(producer, true); if (window == NULL) goto not_valid_surface; diff --git a/services/displayservice/OWNERS b/services/displayservice/OWNERS index 7a3e4c266f..40164aae0c 100644 --- a/services/displayservice/OWNERS +++ b/services/displayservice/OWNERS @@ -1,2 +1 @@ smoreland@google.com -lpy@google.com diff --git a/services/inputflinger/tests/InputDispatcher_test.cpp b/services/inputflinger/tests/InputDispatcher_test.cpp index 41131fc49b..6ff510677a 100644 --- a/services/inputflinger/tests/InputDispatcher_test.cpp +++ b/services/inputflinger/tests/InputDispatcher_test.cpp @@ -8132,6 +8132,17 @@ TEST_F(InputDispatcherTest, VerifyInputEvent_KeyEvent) { ASSERT_EQ(keyArgs.scanCode, verifiedKey.scanCode); ASSERT_EQ(keyArgs.metaState, verifiedKey.metaState); ASSERT_EQ(0, verifiedKey.repeatCount); + + // InputEvent and subclasses don't have a virtual destructor and only + // InputEvent's destructor gets called when `verified` goes out of scope, + // even if `verifyInputEvent` returns an object of a subclass. To fix this, + // we should either consider using std::variant in some way, or introduce an + // intermediate POD data structure that we will put the data into just prior + // to signing. Adding virtual functions to these classes is undesirable as + // the bytes in these objects are getting signed. As a temporary fix, cast + // the pointer to the correct class (which is statically known) before + // destruction. + delete (VerifiedKeyEvent*)verified.release(); } TEST_F(InputDispatcherTest, VerifyInputEvent_MotionEvent) { @@ -8179,6 +8190,10 @@ TEST_F(InputDispatcherTest, VerifyInputEvent_MotionEvent) { EXPECT_EQ(motionArgs.downTime, verifiedMotion.downTimeNanos); EXPECT_EQ(motionArgs.metaState, verifiedMotion.metaState); EXPECT_EQ(motionArgs.buttonState, verifiedMotion.buttonState); + + // Cast to the correct type before destruction. See explanation at the end + // of the VerifyInputEvent_KeyEvent test. + delete (VerifiedMotionEvent*)verified.release(); } /** diff --git a/services/surfaceflinger/OWNERS b/services/surfaceflinger/OWNERS index fa0ecee6b5..13edd1668e 100644 --- a/services/surfaceflinger/OWNERS +++ b/services/surfaceflinger/OWNERS @@ -12,6 +12,5 @@ racarr@google.com ramindani@google.com rnlee@google.com sallyqi@google.com -scroggo@google.com vishnun@google.com xwxw@google.com diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 6cce334ab7..326bf57f0a 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -4354,27 +4354,6 @@ void SurfaceFlinger::invalidateLayerStack(const ui::LayerFilter& layerFilter, co status_t SurfaceFlinger::addClientLayer(LayerCreationArgs& args, const sp<IBinder>& handle, const sp<Layer>& layer, const wp<Layer>& parent, uint32_t* outTransformHint) { - if (mNumLayers >= MAX_LAYERS) { - static std::atomic<nsecs_t> lasttime{0}; - nsecs_t now = systemTime(); - if (lasttime != 0 && ns2s(now - lasttime.load()) < 10) { - ALOGE("AddClientLayer already dumped 10s before"); - return NO_MEMORY; - } else { - lasttime = now; - } - - ALOGE("AddClientLayer failed, mNumLayers (%zu) >= MAX_LAYERS (%zu)", mNumLayers.load(), - MAX_LAYERS); - static_cast<void>(mScheduler->schedule([&]() FTL_FAKE_GUARD(kMainThreadContext) { - ALOGE("Dumping on-screen layers."); - mLayerHierarchyBuilder.dumpLayerSample(mLayerHierarchyBuilder.getHierarchy()); - ALOGE("Dumping off-screen layers."); - mLayerHierarchyBuilder.dumpLayerSample(mLayerHierarchyBuilder.getOffscreenHierarchy()); - })); - return NO_MEMORY; - } - if (outTransformHint) { *outTransformHint = mActiveDisplayTransformHint; } @@ -5145,16 +5124,15 @@ status_t SurfaceFlinger::mirrorDisplay(DisplayId displayId, const LayerCreationA mirrorArgs.addToRoot = true; mirrorArgs.layerStackToMirror = layerStack; result = createEffectLayer(mirrorArgs, &outResult.handle, &rootMirrorLayer); + if (result != NO_ERROR) { + return result; + } outResult.layerId = rootMirrorLayer->sequence; outResult.layerName = String16(rootMirrorLayer->getDebugName()); - result |= addClientLayer(mirrorArgs, outResult.handle, rootMirrorLayer /* layer */, + addClientLayer(mirrorArgs, outResult.handle, rootMirrorLayer /* layer */, nullptr /* parent */, nullptr /* outTransformHint */); } - if (result != NO_ERROR) { - return result; - } - setTransactionFlags(eTransactionFlushNeeded); return NO_ERROR; } @@ -5172,6 +5150,9 @@ status_t SurfaceFlinger::createLayer(LayerCreationArgs& args, gui::CreateSurface [[fallthrough]]; case ISurfaceComposerClient::eFXSurfaceEffect: { result = createBufferStateLayer(args, &outResult.handle, &layer); + if (result != NO_ERROR) { + return result; + } std::atomic<int32_t>* pendingBufferCounter = layer->getPendingBufferCounter(); if (pendingBufferCounter) { std::string counterName = layer->getPendingBufferCounterName(); @@ -5211,6 +5192,9 @@ status_t SurfaceFlinger::createLayer(LayerCreationArgs& args, gui::CreateSurface status_t SurfaceFlinger::createBufferStateLayer(LayerCreationArgs& args, sp<IBinder>* handle, sp<Layer>* outLayer) { + if (checkLayerLeaks() != NO_ERROR) { + return NO_MEMORY; + } *outLayer = getFactory().createBufferStateLayer(args); *handle = (*outLayer)->getHandle(); return NO_ERROR; @@ -5218,11 +5202,38 @@ status_t SurfaceFlinger::createBufferStateLayer(LayerCreationArgs& args, sp<IBin status_t SurfaceFlinger::createEffectLayer(const LayerCreationArgs& args, sp<IBinder>* handle, sp<Layer>* outLayer) { + if (checkLayerLeaks() != NO_ERROR) { + return NO_MEMORY; + } *outLayer = getFactory().createEffectLayer(args); *handle = (*outLayer)->getHandle(); return NO_ERROR; } +status_t SurfaceFlinger::checkLayerLeaks() { + if (mNumLayers >= MAX_LAYERS) { + static std::atomic<nsecs_t> lasttime{0}; + nsecs_t now = systemTime(); + if (lasttime != 0 && ns2s(now - lasttime.load()) < 10) { + ALOGE("CreateLayer already dumped 10s before"); + return NO_MEMORY; + } else { + lasttime = now; + } + + ALOGE("CreateLayer failed, mNumLayers (%zu) >= MAX_LAYERS (%zu)", mNumLayers.load(), + MAX_LAYERS); + static_cast<void>(mScheduler->schedule([&]() FTL_FAKE_GUARD(kMainThreadContext) { + ALOGE("Dumping on-screen layers."); + mLayerHierarchyBuilder.dumpLayerSample(mLayerHierarchyBuilder.getHierarchy()); + ALOGE("Dumping off-screen layers."); + mLayerHierarchyBuilder.dumpLayerSample(mLayerHierarchyBuilder.getOffscreenHierarchy()); + })); + return NO_MEMORY; + } + return NO_ERROR; +} + void SurfaceFlinger::onHandleDestroyed(BBinder* handle, sp<Layer>& layer, uint32_t layerId) { { // Used to remove stalled transactions which uses an internal lock. diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 0808c12d07..24194b18ac 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -835,6 +835,9 @@ private: status_t createEffectLayer(const LayerCreationArgs& args, sp<IBinder>* outHandle, sp<Layer>* outLayer); + // Checks if there are layer leaks before creating layer + status_t checkLayerLeaks(); + status_t mirrorLayer(const LayerCreationArgs& args, const sp<IBinder>& mirrorFromHandle, gui::CreateSurfaceResult& outResult); |