From 4f5ede6434f9085881964abb18dbfc4d8e7d93f5 Mon Sep 17 00:00:00 2001 From: Yu Shan Date: Thu, 21 Dec 2023 17:25:57 -0800 Subject: [PATCH] Add ScheduleInfo validity check. Define the max task data size. Requires remote access HAL to return invalid arg if ScheduleInfo is not valid. Updated the reference impl to add the checks. Test: atest RemoteAccessServiceUnitTest Bug: 317405128 Change-Id: Ia17dda2683c3bcc861542cb2fbd812ce8bd368aa --- .../automotive/remoteaccess/ScheduleInfo.aidl | 1 + .../remoteaccess/IRemoteAccess.aidl | 3 + .../automotive/remoteaccess/ScheduleInfo.aidl | 3 + .../hal/default/src/RemoteAccessService.cpp | 21 +++++- .../test/RemoteAccessServiceUnitTest.cpp | 66 ++++++++++++++++++- 5 files changed, 92 insertions(+), 2 deletions(-) diff --git a/automotive/remoteaccess/aidl_api/android.hardware.automotive.remoteaccess/current/android/hardware/automotive/remoteaccess/ScheduleInfo.aidl b/automotive/remoteaccess/aidl_api/android.hardware.automotive.remoteaccess/current/android/hardware/automotive/remoteaccess/ScheduleInfo.aidl index a5d81cf9fc..ec8733a1bc 100644 --- a/automotive/remoteaccess/aidl_api/android.hardware.automotive.remoteaccess/current/android/hardware/automotive/remoteaccess/ScheduleInfo.aidl +++ b/automotive/remoteaccess/aidl_api/android.hardware.automotive.remoteaccess/current/android/hardware/automotive/remoteaccess/ScheduleInfo.aidl @@ -41,4 +41,5 @@ parcelable ScheduleInfo { int count; long startTimeInEpochSeconds; long periodicInSeconds; + const int MAX_TASK_DATA_SIZE_IN_BYTES = 10240; } diff --git a/automotive/remoteaccess/android/hardware/automotive/remoteaccess/IRemoteAccess.aidl b/automotive/remoteaccess/android/hardware/automotive/remoteaccess/IRemoteAccess.aidl index 705cdbdd08..f0468c42b4 100644 --- a/automotive/remoteaccess/android/hardware/automotive/remoteaccess/IRemoteAccess.aidl +++ b/automotive/remoteaccess/android/hardware/automotive/remoteaccess/IRemoteAccess.aidl @@ -167,6 +167,9 @@ interface IRemoteAccess { * {@code scheduleId} for this client exists. * *

Must return {@code EX_ILLEGAL_ARGUMENT} if the task type is not supported. + * + *

Must return {@code EX_ILLEGLA_ARGUMENT} if the scheduleInfo is not valid (e.g. count is + * a negative number). */ void scheduleTask(in ScheduleInfo scheduleInfo); diff --git a/automotive/remoteaccess/android/hardware/automotive/remoteaccess/ScheduleInfo.aidl b/automotive/remoteaccess/android/hardware/automotive/remoteaccess/ScheduleInfo.aidl index 40fba6f2a3..4f2537c773 100644 --- a/automotive/remoteaccess/android/hardware/automotive/remoteaccess/ScheduleInfo.aidl +++ b/automotive/remoteaccess/android/hardware/automotive/remoteaccess/ScheduleInfo.aidl @@ -21,6 +21,7 @@ import android.hardware.automotive.remoteaccess.TaskType; @VintfStability @JavaDerive(equals=true, toString=true) parcelable ScheduleInfo { + const int MAX_TASK_DATA_SIZE_IN_BYTES = 10240; /** * The ID used to identify the client this schedule is for. This must be one of the * preconfigured remote access serverless client ID defined in car service resource @@ -41,6 +42,8 @@ parcelable ScheduleInfo { * executed. It is not interpreted/parsed by the Android system. * *

This is only used for {@code TaskType.CUSTOM}. + * + *

The data size must be less than {@link MAX_TASK_DATA_SIZE_IN_BYTES}. */ byte[] taskData; /** diff --git a/automotive/remoteaccess/hal/default/src/RemoteAccessService.cpp b/automotive/remoteaccess/hal/default/src/RemoteAccessService.cpp index 2a7f2091e4..1b42a1f091 100644 --- a/automotive/remoteaccess/hal/default/src/RemoteAccessService.cpp +++ b/automotive/remoteaccess/hal/default/src/RemoteAccessService.cpp @@ -331,6 +331,24 @@ ScopedAStatus RemoteAccessService::scheduleTask(const ScheduleInfo& scheduleInfo ClientContext context; ScheduleTaskRequest request = {}; ScheduleTaskResponse response = {}; + + if (scheduleInfo.count < 0) { + return ScopedAStatus::fromExceptionCodeWithMessage(EX_ILLEGAL_ARGUMENT, + "count must be >= 0"); + } + if (scheduleInfo.startTimeInEpochSeconds < 0) { + return ScopedAStatus::fromExceptionCodeWithMessage(EX_ILLEGAL_ARGUMENT, + "startTimeInEpochSeconds must be >= 0"); + } + if (scheduleInfo.periodicInSeconds < 0) { + return ScopedAStatus::fromExceptionCodeWithMessage(EX_ILLEGAL_ARGUMENT, + "periodicInSeconds must be >= 0"); + } + if (scheduleInfo.taskData.size() > scheduleInfo.MAX_TASK_DATA_SIZE_IN_BYTES) { + return ScopedAStatus::fromExceptionCodeWithMessage(EX_ILLEGAL_ARGUMENT, + "task data too big"); + } + request.mutable_scheduleinfo()->set_clientid(scheduleInfo.clientId); request.mutable_scheduleinfo()->set_scheduleid(scheduleInfo.scheduleId); request.mutable_scheduleinfo()->set_data(scheduleInfo.taskData.data(), @@ -348,7 +366,8 @@ ScopedAStatus RemoteAccessService::scheduleTask(const ScheduleInfo& scheduleInfo case ErrorCode::OK: return ScopedAStatus::ok(); case ErrorCode::INVALID_ARG: - return ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); + return ScopedAStatus::fromExceptionCodeWithMessage( + EX_ILLEGAL_ARGUMENT, "received invalid_arg from grpc server"); default: // Should not happen. return ScopedAStatus::fromServiceSpecificErrorWithMessage( diff --git a/automotive/remoteaccess/hal/default/test/RemoteAccessServiceUnitTest.cpp b/automotive/remoteaccess/hal/default/test/RemoteAccessServiceUnitTest.cpp index a46a983a9c..7992a50f5e 100644 --- a/automotive/remoteaccess/hal/default/test/RemoteAccessServiceUnitTest.cpp +++ b/automotive/remoteaccess/hal/default/test/RemoteAccessServiceUnitTest.cpp @@ -473,7 +473,71 @@ TEST_F(RemoteAccessServiceUnitTest, TestScheduleTask) { EXPECT_EQ(grpcRequest.scheduleinfo().periodicinseconds(), kTestPeriodicInSeconds); } -TEST_F(RemoteAccessServiceUnitTest, TestScheduleTask_InvalidArg) { +TEST_F(RemoteAccessServiceUnitTest, TestScheduleTask_InvalidCount) { + ScheduleInfo scheduleInfo = { + .clientId = kTestClientId, + .scheduleId = kTestScheduleId, + .taskData = kTestData, + .count = -1, + .startTimeInEpochSeconds = kTestStartTimeInEpochSeconds, + .periodicInSeconds = kTestPeriodicInSeconds, + }; + + ScopedAStatus status = getService()->scheduleTask(scheduleInfo); + + ASSERT_FALSE(status.isOk()); + ASSERT_EQ(status.getExceptionCode(), EX_ILLEGAL_ARGUMENT); +} + +TEST_F(RemoteAccessServiceUnitTest, TestScheduleTask_InvalidStartTimeInEpochSeconds) { + ScheduleInfo scheduleInfo = { + .clientId = kTestClientId, + .scheduleId = kTestScheduleId, + .taskData = kTestData, + .count = kTestCount, + .startTimeInEpochSeconds = -1, + .periodicInSeconds = kTestPeriodicInSeconds, + }; + + ScopedAStatus status = getService()->scheduleTask(scheduleInfo); + + ASSERT_FALSE(status.isOk()); + ASSERT_EQ(status.getExceptionCode(), EX_ILLEGAL_ARGUMENT); +} + +TEST_F(RemoteAccessServiceUnitTest, TestScheduleTask_InvalidPeriodicInSeconds) { + ScheduleInfo scheduleInfo = { + .clientId = kTestClientId, + .scheduleId = kTestScheduleId, + .taskData = kTestData, + .count = kTestCount, + .startTimeInEpochSeconds = kTestStartTimeInEpochSeconds, + .periodicInSeconds = -1, + }; + + ScopedAStatus status = getService()->scheduleTask(scheduleInfo); + + ASSERT_FALSE(status.isOk()); + ASSERT_EQ(status.getExceptionCode(), EX_ILLEGAL_ARGUMENT); +} + +TEST_F(RemoteAccessServiceUnitTest, TestScheduleTask_TaskDataTooLarge) { + ScheduleInfo scheduleInfo = { + .clientId = kTestClientId, + .scheduleId = kTestScheduleId, + .taskData = std::vector(ScheduleInfo::MAX_TASK_DATA_SIZE_IN_BYTES + 1), + .count = kTestCount, + .startTimeInEpochSeconds = kTestStartTimeInEpochSeconds, + .periodicInSeconds = kTestPeriodicInSeconds, + }; + + ScopedAStatus status = getService()->scheduleTask(scheduleInfo); + + ASSERT_FALSE(status.isOk()); + ASSERT_EQ(status.getExceptionCode(), EX_ILLEGAL_ARGUMENT); +} + +TEST_F(RemoteAccessServiceUnitTest, TestScheduleTask_InvalidArgFromGrpcServer) { EXPECT_CALL(*getGrpcWakeupClientStub(), ScheduleTask) .WillOnce([]([[maybe_unused]] ClientContext* context, [[maybe_unused]] const ScheduleTaskRequest& request,