From bee6f16d130a3467d74cc3b44ed00b5dd5c158e1 Mon Sep 17 00:00:00 2001 From: Xiang Wang Date: Tue, 18 Jul 2023 17:58:10 -0700 Subject: [PATCH] Fix the setThreads JNI to throw SecurityException Bug: 291830812 Test: atest FrameworksCoreTests:PerformanceHintManagerTest PerformanceHintTest Change-Id: I35b28772c1ad2cf26e9f9f7f019406ef7c15303e --- .../android/os/PerformanceHintManager.java | 2 ++ .../jni/android_os_PerformanceHintManager.cpp | 23 ++++++++++++++++--- .../os/PerformanceHintManagerTest.java | 13 +++++++++-- native/android/performance_hint.cpp | 5 ++-- .../PerformanceHintNativeTest.cpp | 11 +++++++++ 5 files changed, 47 insertions(+), 7 deletions(-) diff --git a/core/java/android/os/PerformanceHintManager.java b/core/java/android/os/PerformanceHintManager.java index f79d6e6da1f8b..977ef60c43b6e 100644 --- a/core/java/android/os/PerformanceHintManager.java +++ b/core/java/android/os/PerformanceHintManager.java @@ -29,6 +29,7 @@ import java.io.Closeable; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.ref.Reference; +import java.util.Objects; /** The PerformanceHintManager allows apps to send performance hint to system. */ @@ -239,6 +240,7 @@ public final class PerformanceHintManager { if (mNativeSessionPtr == 0) { return; } + Objects.requireNonNull(tids, "tids cannot be null"); if (tids.length == 0) { throw new IllegalArgumentException("Thread id list can't be empty."); } diff --git a/core/jni/android_os_PerformanceHintManager.cpp b/core/jni/android_os_PerformanceHintManager.cpp index d8a2497439c2a..ffe844d39f271 100644 --- a/core/jni/android_os_PerformanceHintManager.cpp +++ b/core/jni/android_os_PerformanceHintManager.cpp @@ -41,7 +41,7 @@ typedef void (*APH_updateTargetWorkDuration)(APerformanceHintSession*, int64_t); typedef void (*APH_reportActualWorkDuration)(APerformanceHintSession*, int64_t); typedef void (*APH_closeSession)(APerformanceHintSession* session); typedef void (*APH_sendHint)(APerformanceHintSession*, int32_t); -typedef void (*APH_setThreads)(APerformanceHintSession*, const pid_t*, size_t); +typedef int (*APH_setThreads)(APerformanceHintSession*, const pid_t*, size_t); typedef void (*APH_getThreadIds)(APerformanceHintSession*, int32_t* const, size_t* const); bool gAPerformanceHintBindingInitialized = false; @@ -112,6 +112,20 @@ void ensureAPerformanceHintBindingInitialized() { } // namespace +static void throwExceptionForErrno(JNIEnv* env, int err, const std::string& msg) { + switch (err) { + case EINVAL: + jniThrowExceptionFmt(env, "java/lang/IllegalArgumentException", msg.c_str()); + break; + case EPERM: + jniThrowExceptionFmt(env, "java/lang/SecurityException", msg.c_str()); + break; + default: + jniThrowException(env, "java/lang/RuntimeException", msg.c_str()); + break; + } +} + static jlong nativeAcquireManager(JNIEnv* env, jclass clazz) { ensureAPerformanceHintBindingInitialized(); return reinterpret_cast(gAPH_getManagerFn()); @@ -174,8 +188,11 @@ static void nativeSetThreads(JNIEnv* env, jclass clazz, jlong nativeSessionPtr, for (size_t i = 0; i < tidsArray.size(); ++i) { tidsVector.push_back(static_cast(tidsArray[i])); } - gAPH_setThreadsFn(reinterpret_cast(nativeSessionPtr), - tidsVector.data(), tidsVector.size()); + int err = gAPH_setThreadsFn(reinterpret_cast(nativeSessionPtr), + tidsVector.data(), tidsVector.size()); + if (err != 0) { + throwExceptionForErrno(env, err, "Failed to set threads for hint session"); + } } // This call should only be used for validation in tests only. This call will initiate two IPC diff --git a/core/tests/coretests/src/android/os/PerformanceHintManagerTest.java b/core/tests/coretests/src/android/os/PerformanceHintManagerTest.java index 7eefbbca2d015..2c03fdc2ef0d4 100644 --- a/core/tests/coretests/src/android/os/PerformanceHintManagerTest.java +++ b/core/tests/coretests/src/android/os/PerformanceHintManagerTest.java @@ -139,11 +139,20 @@ public class PerformanceHintManagerTest { } @Test - public void testSetThreadsWithIllegalArgument() { + public void testSetThreads_emptyTids() { Session session = createSession(); assumeNotNull(session); assertThrows(IllegalArgumentException.class, () -> { - session.setThreads(new int[] { }); + session.setThreads(new int[]{}); + }); + } + + @Test + public void testSetThreads_invalidTids() { + Session session = createSession(); + assumeNotNull(session); + assertThrows(SecurityException.class, () -> { + session.setThreads(new int[]{-1}); }); } } diff --git a/native/android/performance_hint.cpp b/native/android/performance_hint.cpp index b3628fa3e5cec..6198f40bfcd1a 100644 --- a/native/android/performance_hint.cpp +++ b/native/android/performance_hint.cpp @@ -274,9 +274,10 @@ int APerformanceHintSession::setThreads(const int32_t* threadIds, size_t size) { binder::Status ret = mHintManager->setHintSessionThreads(mHintSession, tids); if (!ret.isOk()) { ALOGE("%s: failed: %s", __FUNCTION__, ret.exceptionMessage().c_str()); - if (ret.exceptionCode() == binder::Status::Exception::EX_SECURITY || - ret.exceptionCode() == binder::Status::Exception::EX_ILLEGAL_ARGUMENT) { + if (ret.exceptionCode() == binder::Status::Exception::EX_ILLEGAL_ARGUMENT) { return EINVAL; + } else if (ret.exceptionCode() == binder::Status::Exception::EX_SECURITY) { + return EPERM; } return EPIPE; } diff --git a/native/android/tests/performance_hint/PerformanceHintNativeTest.cpp b/native/android/tests/performance_hint/PerformanceHintNativeTest.cpp index 791adfd33fcd2..6f7562b93a30f 100644 --- a/native/android/tests/performance_hint/PerformanceHintNativeTest.cpp +++ b/native/android/tests/performance_hint/PerformanceHintNativeTest.cpp @@ -178,4 +178,15 @@ TEST_F(PerformanceHintTest, SetThreads) { .WillOnce(Return(Status())); result = APerformanceHint_setThreads(session, newTids.data(), newTids.size()); EXPECT_EQ(0, result); + + testing::Mock::VerifyAndClearExpectations(mMockIHintManager); + std::vector invalidTids; + auto status = Status::fromExceptionCode(binder::Status::Exception::EX_SECURITY); + invalidTids.push_back(4); + invalidTids.push_back(6); + EXPECT_CALL(*mMockIHintManager, setHintSessionThreads(_, Eq(invalidTids))) + .Times(Exactly(1)) + .WillOnce(Return(status)); + result = APerformanceHint_setThreads(session, invalidTids.data(), invalidTids.size()); + EXPECT_EQ(EPERM, result); }