diff --git a/media/libmediatranscoding/TranscodingClientManager.cpp b/media/libmediatranscoding/TranscodingClientManager.cpp index 5013a51526..e26dbaa3f8 100644 --- a/media/libmediatranscoding/TranscodingClientManager.cpp +++ b/media/libmediatranscoding/TranscodingClientManager.cpp @@ -17,12 +17,12 @@ // #define LOG_NDEBUG 0 #define LOG_TAG "TranscodingClientManager" +#include #include #include namespace android { -class DeathNotifier; using Status = ::ndk::ScopedAStatus; // static @@ -31,9 +31,17 @@ sp TranscodingClientManager::getInstance() { return sInstance; } +// static +void TranscodingClientManager::BinderDiedCallback(void* cookie) { + int32_t clientId = static_cast(reinterpret_cast(cookie)); + ALOGD("Client %" PRId32 " is dead", clientId); + // Don't check for pid validity since we know it's already dead. + sp manager = TranscodingClientManager::getInstance(); + manager->removeClient(clientId); +} + TranscodingClientManager::TranscodingClientManager() - : mDeathRecipient(AIBinder_DeathRecipient_new( - TranscodingClientManager::DeathNotifier::BinderDiedCallback)) { + : mDeathRecipient(AIBinder_DeathRecipient_new(BinderDiedCallback)) { ALOGD("TranscodingClientManager started"); } @@ -77,14 +85,13 @@ void TranscodingClientManager::dumpAllClients(int fd, const Vector& ar status_t TranscodingClientManager::addClient(std::unique_ptr client) { // Validate the client. - if (client == nullptr || client->mClientId <= 0 || client->mClientPid <= 0 || - client->mClientUid <= 0 || client->mClientOpPackageName.empty() || + if (client == nullptr || client->mClientId < 0 || client->mClientPid < 0 || + client->mClientUid < 0 || client->mClientOpPackageName.empty() || client->mClientOpPackageName == "") { ALOGE("Invalid client"); return BAD_VALUE; } - ALOGD("Adding client id %d %s", client->mClientId, client->mClientOpPackageName.c_str()); std::scoped_lock lock{mLock}; // Check if the client already exists. @@ -93,10 +100,11 @@ status_t TranscodingClientManager::addClient(std::unique_ptr client) return ALREADY_EXISTS; } - // Listen to the death of the client. - client->mDeathNotifier = new DeathNotifier(); + ALOGD("Adding client id %d pid: %d uid: %d %s", client->mClientId, client->mClientPid, + client->mClientUid, client->mClientOpPackageName.c_str()); + AIBinder_linkToDeath(client->mClient->asBinder().get(), mDeathRecipient.get(), - client->mDeathNotifier.get()); + reinterpret_cast(client->mClientId)); // Adds the new client to the map. mClientIdToClientInfoMap[client->mClientId] = std::move(client); @@ -120,7 +128,7 @@ status_t TranscodingClientManager::removeClient(int32_t clientId) { // Check if the client still live. If alive, unlink the death. if (client) { AIBinder_unlinkToDeath(client->asBinder().get(), mDeathRecipient.get(), - it->second->mDeathNotifier.get()); + reinterpret_cast(clientId)); } // Erase the entry. @@ -134,13 +142,4 @@ size_t TranscodingClientManager::getNumOfClients() const { return mClientIdToClientInfoMap.size(); } -// static -void TranscodingClientManager::DeathNotifier::BinderDiedCallback(void* cookie) { - int32_t* pClientId = static_cast(cookie); - ALOGD("Client %d is dead", *pClientId); - // Don't check for pid validity since we know it's already dead. - sp manager = TranscodingClientManager::getInstance(); - manager->removeClient(*pClientId); -} - } // namespace android diff --git a/media/libmediatranscoding/aidl/android/media/IMediaTranscodingService.aidl b/media/libmediatranscoding/aidl/android/media/IMediaTranscodingService.aidl index 798300afc9..07b6c1a8a2 100644 --- a/media/libmediatranscoding/aidl/android/media/IMediaTranscodingService.aidl +++ b/media/libmediatranscoding/aidl/android/media/IMediaTranscodingService.aidl @@ -74,6 +74,11 @@ interface IMediaTranscodingService { */ boolean unregisterClient(in int clientId); + /** + * Returns the number of clients. This is used for debugging. + */ + int getNumOfClients(); + /** * Submits a transcoding request to MediaTranscodingService. * diff --git a/media/libmediatranscoding/include/media/TranscodingClientManager.h b/media/libmediatranscoding/include/media/TranscodingClientManager.h index dbf837c2fe..74a000ff84 100644 --- a/media/libmediatranscoding/include/media/TranscodingClientManager.h +++ b/media/libmediatranscoding/include/media/TranscodingClientManager.h @@ -46,10 +46,6 @@ class MediaTranscodingService; * TODO(hkuang): Hook up with MediaMetrics to log all the transactions. */ class TranscodingClientManager : public RefBase { - private: - // Forward declare it as it will be used in ClientInfo below. - class DeathNotifier; - public: virtual ~TranscodingClientManager(); @@ -67,8 +63,6 @@ class TranscodingClientManager : public RefBase { int32_t mClientUid; /* Package name of the client. */ std::string mClientOpPackageName; - /* Listener for the death of the client. */ - sp mDeathNotifier; ClientInfo(const std::shared_ptr& client, int64_t clientId, int32_t pid, int32_t uid, const std::string& opPackageName) @@ -76,8 +70,7 @@ class TranscodingClientManager : public RefBase { mClientId(clientId), mClientPid(pid), mClientUid(uid), - mClientOpPackageName(opPackageName), - mDeathNotifier(nullptr) {} + mClientOpPackageName(opPackageName) {} }; /** @@ -121,26 +114,17 @@ class TranscodingClientManager : public RefBase { friend class MediaTranscodingService; friend class TranscodingClientManagerTest; - class DeathNotifier : public RefBase { - public: - DeathNotifier() = default; - - ~DeathNotifier() = default; - - // Implement death recipient - static void BinderDiedCallback(void* cookie); - }; - /** Get the singleton instance of the TranscodingClientManager. */ static sp getInstance(); TranscodingClientManager(); + static void BinderDiedCallback(void* cookie); + mutable std::mutex mLock; std::unordered_map> mClientIdToClientInfoMap GUARDED_BY(mLock); - std::vector> mDeathNotifiers GUARDED_BY(mLock); ::ndk::ScopedAIBinder_DeathRecipient mDeathRecipient; }; diff --git a/services/mediatranscoding/Android.bp b/services/mediatranscoding/Android.bp index 3dc43f132c..17347a98ef 100644 --- a/services/mediatranscoding/Android.bp +++ b/services/mediatranscoding/Android.bp @@ -5,8 +5,11 @@ cc_library_shared { srcs: ["MediaTranscodingService.cpp"], shared_libs: [ + "libbase", "libbinder_ndk", "liblog", + "libmediatranscoding", + "libutils", ], static_libs: [ @@ -27,11 +30,13 @@ cc_binary { ], shared_libs: [ + "libbase", // TODO(hkuang): Use libbinder_ndk "libbinder", "libutils", "liblog", "libbase", + "libmediatranscoding", "libmediatranscodingservice", ], diff --git a/services/mediatranscoding/MediaTranscodingService.cpp b/services/mediatranscoding/MediaTranscodingService.cpp index 0269896c14..480844e508 100644 --- a/services/mediatranscoding/MediaTranscodingService.cpp +++ b/services/mediatranscoding/MediaTranscodingService.cpp @@ -16,26 +16,55 @@ //#define LOG_NDEBUG 0 #define LOG_TAG "MediaTranscodingService" -#include "MediaTranscodingService.h" - +#include #include #include +#include #include #include namespace android { +// Convenience methods for constructing binder::Status objects for error returns +#define STATUS_ERROR_FMT(errorCode, errorString, ...) \ + Status::fromServiceSpecificErrorWithMessage( \ + errorCode, \ + String8::format("%s:%d: " errorString, __FUNCTION__, __LINE__, ##__VA_ARGS__)) + +// Can MediaTranscoding service trust the caller based on the calling UID? +// TODO(hkuang): Add MediaProvider's UID. +static bool isTrustedCallingUid(uid_t uid) { + switch (uid) { + case AID_ROOT: // root user + case AID_SYSTEM: + case AID_SHELL: + case AID_MEDIA: // mediaserver + return true; + default: + return false; + } +} + MediaTranscodingService::MediaTranscodingService() { ALOGV("MediaTranscodingService is created"); + mTranscodingClientManager = TranscodingClientManager::getInstance(); } MediaTranscodingService::~MediaTranscodingService() { ALOGE("Should not be in ~MediaTranscodingService"); } -binder_status_t MediaTranscodingService::dump(int /* fd */, const char** /*args*/, - uint32_t /*numArgs*/) { - // TODO(hkuang): Add implementation. +binder_status_t MediaTranscodingService::dump(int fd, const char** /*args*/, uint32_t /*numArgs*/) { + String8 result; + const size_t SIZE = 256; + char buffer[SIZE]; + + snprintf(buffer, SIZE, "MediaTranscodingService: %p\n", this); + result.append(buffer); + write(fd, result.string(), result.size()); + + Vector args; + mTranscodingClientManager->dumpAllClients(fd, args); return OK; } @@ -51,15 +80,97 @@ void MediaTranscodingService::instantiate() { } Status MediaTranscodingService::registerClient( - const std::shared_ptr& /*in_client*/, - const std::string& /* in_opPackageName */, int32_t /* in_clientUid */, - int32_t /* in_clientPid */, int32_t* /*_aidl_return*/) { - // TODO(hkuang): Add implementation. + const std::shared_ptr& in_client, + const std::string& in_opPackageName, int32_t in_clientUid, int32_t in_clientPid, + int32_t* _aidl_return) { + if (in_client == nullptr) { + ALOGE("Client can not be null"); + *_aidl_return = kInvalidJobId; + return Status::fromServiceSpecificError(ERROR_ILLEGAL_ARGUMENT); + } + + int32_t callingPid = AIBinder_getCallingPid(); + int32_t callingUid = AIBinder_getCallingUid(); + + // Check if we can trust clientUid. Only privilege caller could forward the uid on app client's behalf. + if (in_clientUid == USE_CALLING_UID) { + in_clientUid = callingUid; + } else if (!isTrustedCallingUid(callingUid)) { + ALOGE("MediaTranscodingService::registerClient failed (calling PID %d, calling UID %d) " + "rejected " + "(don't trust clientUid %d)", + in_clientPid, in_clientUid, in_clientUid); + return STATUS_ERROR_FMT(ERROR_PERMISSION_DENIED, + "Untrusted caller (calling PID %d, UID %d) trying to " + "register client", + in_clientPid, in_clientUid); + } + + // Check if we can trust clientPid. Only privilege caller could forward the pid on app client's behalf. + if (in_clientPid == USE_CALLING_PID) { + in_clientPid = callingPid; + } else if (!isTrustedCallingUid(callingUid)) { + ALOGE("MediaTranscodingService::registerClient client failed (calling PID %d, calling UID " + "%d) rejected " + "(don't trust clientPid %d)", + in_clientPid, in_clientUid, in_clientPid); + return STATUS_ERROR_FMT(ERROR_PERMISSION_DENIED, + "Untrusted caller (calling PID %d, UID %d) trying to " + "register client", + in_clientPid, in_clientUid); + } + + // We know the clientId must be equal to its pid as we assigned client's pid as its clientId. + int32_t clientId = in_clientPid; + + // Checks if the client already registers. + if (mTranscodingClientManager->isClientIdRegistered(clientId)) { + return Status::fromServiceSpecificError(ERROR_ALREADY_EXISTS); + } + + // Creates the client and uses its process id as client id. + std::unique_ptr newClient = + std::make_unique( + in_client, clientId, in_clientPid, in_clientUid, in_opPackageName); + status_t err = mTranscodingClientManager->addClient(std::move(newClient)); + if (err != OK) { + *_aidl_return = kInvalidClientId; + return STATUS_ERROR_FMT(err, "Failed to add client to TranscodingClientManager"); + } + + ALOGD("Assign client: %s pid: %d, uid: %d with id: %d", in_opPackageName.c_str(), in_clientPid, + in_clientUid, clientId); + + *_aidl_return = clientId; return Status::ok(); } -Status MediaTranscodingService::unregisterClient(int32_t /*clientId*/, bool* /*_aidl_return*/) { - // TODO(hkuang): Add implementation. +Status MediaTranscodingService::unregisterClient(int32_t clientId, bool* _aidl_return) { + ALOGD("unregisterClient id: %d", clientId); + int32_t callingUid = AIBinder_getCallingUid(); + int32_t callingPid = AIBinder_getCallingPid(); + + // Only the client with clientId or the trusted caller could unregister the client. + if (callingPid != clientId) { + if (!isTrustedCallingUid(callingUid)) { + ALOGE("Untrusted caller (calling PID %d, UID %d) trying to " + "unregister client with id: %d", + callingUid, callingPid, clientId); + *_aidl_return = true; + return STATUS_ERROR_FMT(ERROR_PERMISSION_DENIED, + "Untrusted caller (calling PID %d, UID %d) trying to " + "unregister client with id: %d", + callingUid, callingPid, clientId); + } + } + + *_aidl_return = (mTranscodingClientManager->removeClient(clientId) == OK); + return Status::ok(); +} + +Status MediaTranscodingService::getNumOfClients(int32_t* _aidl_return) { + ALOGD("MediaTranscodingService::getNumOfClients"); + *_aidl_return = mTranscodingClientManager->getNumOfClients(); return Status::ok(); } diff --git a/services/mediatranscoding/MediaTranscodingService.h b/services/mediatranscoding/MediaTranscodingService.h index d225f9a83c..4bdb0788ee 100644 --- a/services/mediatranscoding/MediaTranscodingService.h +++ b/services/mediatranscoding/MediaTranscodingService.h @@ -19,6 +19,7 @@ #include #include +#include namespace android { @@ -30,6 +31,9 @@ using ::aidl::android::media::TranscodingRequestParcel; class MediaTranscodingService : public BnMediaTranscodingService { public: + static constexpr int32_t kInvalidJobId = -1; + static constexpr int32_t kInvalidClientId = -1; + MediaTranscodingService(); virtual ~MediaTranscodingService(); @@ -43,6 +47,8 @@ public: Status unregisterClient(int32_t clientId, bool* _aidl_return) override; + Status getNumOfClients(int32_t* _aidl_return) override; + Status submitRequest(int32_t in_clientId, const TranscodingRequestParcel& in_request, TranscodingJobParcel* out_job, int32_t* _aidl_return) override; @@ -54,6 +60,11 @@ public: virtual inline binder_status_t dump(int /*fd*/, const char** /*args*/, uint32_t /*numArgs*/); private: + friend class MediaTranscodingServiceTest; + + mutable std::mutex mServiceLock; + + sp mTranscodingClientManager; }; } // namespace android diff --git a/services/mediatranscoding/tests/Android.bp b/services/mediatranscoding/tests/Android.bp new file mode 100644 index 0000000000..e0e040c316 --- /dev/null +++ b/services/mediatranscoding/tests/Android.bp @@ -0,0 +1,35 @@ +// Build the unit tests for MediaTranscodingService + +cc_defaults { + name: "mediatranscodingservice_test_defaults", + + cflags: [ + "-Wall", + "-Werror", + "-Wextra", + ], + + include_dirs: [ + "frameworks/av/services/mediatranscoding", + ], + + shared_libs: [ + "libbinder", + "libbinder_ndk", + "liblog", + "libutils", + "libmediatranscodingservice", + ], + + static_libs: [ + "mediatranscoding_aidl_interface-ndk_platform", + ], +} + +// MediaTranscodingService unit test +cc_test { + name: "mediatranscodingservice_tests", + defaults: ["mediatranscodingservice_test_defaults"], + + srcs: ["mediatranscodingservice_tests.cpp"], +} \ No newline at end of file diff --git a/services/mediatranscoding/tests/build_and_run_all_unit_tests.sh b/services/mediatranscoding/tests/build_and_run_all_unit_tests.sh new file mode 100644 index 0000000000..bcdc7f7a80 --- /dev/null +++ b/services/mediatranscoding/tests/build_and_run_all_unit_tests.sh @@ -0,0 +1,23 @@ +#!/bin/bash +# +# Run tests in this directory. +# + +if [ -z "$ANDROID_BUILD_TOP" ]; then + echo "Android build environment not set" + exit -1 +fi + +# ensure we have mm +. $ANDROID_BUILD_TOP/build/envsetup.sh + +mm + +echo "waiting for device" + +adb root && adb wait-for-device remount && adb sync + +echo "========================================" + +echo "testing mediatranscodingservice" +adb shell /data/nativetest64/mediatranscodingservice_tests/mediatranscodingservice_tests diff --git a/services/mediatranscoding/tests/mediatranscodingservice_tests.cpp b/services/mediatranscoding/tests/mediatranscodingservice_tests.cpp new file mode 100644 index 0000000000..5a791fe9ee --- /dev/null +++ b/services/mediatranscoding/tests/mediatranscodingservice_tests.cpp @@ -0,0 +1,239 @@ +/* + * Copyright (C) 2019 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. + */ + +// Unit Test for MediaTranscoding Service. + +//#define LOG_NDEBUG 0 +#define LOG_TAG "MediaTranscodingServiceTest" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace android { + +namespace media { + +using Status = ::ndk::ScopedAStatus; +using aidl::android::media::BnTranscodingServiceClient; +using aidl::android::media::IMediaTranscodingService; +using aidl::android::media::ITranscodingServiceClient; + +constexpr int32_t kInvalidClientId = -5; + +// Note that -1 is valid and means using calling pid/uid for the service. But only privilege caller could +// use them. This test is not a privilege caller. +constexpr int32_t kInvalidClientPid = -5; +constexpr int32_t kInvalidClientUid = -5; +constexpr const char* kInvalidClientOpPackageName = ""; + +constexpr int32_t kClientUseCallingPid = -1; +constexpr int32_t kClientUseCallingUid = -1; +constexpr const char* kClientOpPackageName = "TestClient"; + +class MediaTranscodingServiceTest : public ::testing::Test { +public: + MediaTranscodingServiceTest() { ALOGD("MediaTranscodingServiceTest created"); } + + void SetUp() override { + ::ndk::SpAIBinder binder(AServiceManager_getService("media.transcoding")); + mService = IMediaTranscodingService::fromBinder(binder); + if (mService == nullptr) { + ALOGE("Failed to connect to the media.trascoding service."); + return; + } + } + + ~MediaTranscodingServiceTest() { ALOGD("MediaTranscodingingServiceTest destroyed"); } + + std::shared_ptr mService = nullptr; +}; + +struct TestClient : public BnTranscodingServiceClient { + TestClient(const std::shared_ptr& service) : mService(service) { + ALOGD("TestClient Created"); + } + + Status getName(std::string* _aidl_return) override { + *_aidl_return = "test_client"; + return Status::ok(); + } + + Status onTranscodingFinished( + int32_t /* in_jobId */, + const ::aidl::android::media::TranscodingResultParcel& /* in_result */) override { + return Status::ok(); + } + + Status onTranscodingFailed( + int32_t /* in_jobId */, + ::aidl::android::media::TranscodingErrorCode /*in_errorCode */) override { + return Status::ok(); + } + + Status onAwaitNumberOfJobsChanged(int32_t /* in_jobId */, int32_t /* in_oldAwaitNumber */, + int32_t /* in_newAwaitNumber */) override { + return Status::ok(); + } + + Status onProgressUpdate(int32_t /* in_jobId */, int32_t /* in_progress */) override { + return Status::ok(); + } + + virtual ~TestClient() { ALOGI("TestClient destroyed"); }; + +private: + std::shared_ptr mService; +}; + +TEST_F(MediaTranscodingServiceTest, TestRegisterNullClient) { + std::shared_ptr client = nullptr; + int32_t clientId = 0; + Status status = mService->registerClient(client, kClientOpPackageName, kClientUseCallingUid, + kClientUseCallingPid, &clientId); + EXPECT_FALSE(status.isOk()); +} + +TEST_F(MediaTranscodingServiceTest, TestRegisterClientWithInvalidClientPid) { + std::shared_ptr client = + ::ndk::SharedRefBase::make(mService); + EXPECT_TRUE(client != nullptr); + + // Register the client with the service. + int32_t clientId = 0; + Status status = mService->registerClient(client, kClientOpPackageName, kClientUseCallingUid, + kInvalidClientPid, &clientId); + EXPECT_FALSE(status.isOk()); +} + +TEST_F(MediaTranscodingServiceTest, TestRegisterClientWithInvalidClientUid) { + std::shared_ptr client = + ::ndk::SharedRefBase::make(mService); + EXPECT_TRUE(client != nullptr); + + // Register the client with the service. + int32_t clientId = 0; + Status status = mService->registerClient(client, kClientOpPackageName, kInvalidClientUid, + kClientUseCallingPid, &clientId); + EXPECT_FALSE(status.isOk()); +} + +TEST_F(MediaTranscodingServiceTest, TestRegisterClientWithInvalidClientPackageName) { + std::shared_ptr client = + ::ndk::SharedRefBase::make(mService); + EXPECT_TRUE(client != nullptr); + + // Register the client with the service. + int32_t clientId = 0; + Status status = mService->registerClient(client, kInvalidClientOpPackageName, + kClientUseCallingUid, kClientUseCallingPid, &clientId); + EXPECT_FALSE(status.isOk()); +} + +TEST_F(MediaTranscodingServiceTest, TestRegisterOneClient) { + std::shared_ptr client = + ::ndk::SharedRefBase::make(mService); + EXPECT_TRUE(client != nullptr); + + // Register the client with the service. + int32_t clientId = 0; + Status status = mService->registerClient(client, kClientOpPackageName, kClientUseCallingPid, + kClientUseCallingUid, &clientId); + ALOGD("client id is %d", clientId); + EXPECT_TRUE(status.isOk()); + + // Validate the clientId. + EXPECT_TRUE(clientId > 0); + + // Check the number of Clients. + int32_t numOfClients; + status = mService->getNumOfClients(&numOfClients); + EXPECT_TRUE(status.isOk()); + EXPECT_EQ(1, numOfClients); + + // Unregister the client. + bool res; + status = mService->unregisterClient(clientId, &res); + EXPECT_TRUE(status.isOk()); + EXPECT_TRUE(res); +} + +TEST_F(MediaTranscodingServiceTest, TestUnRegisterClientWithInvalidClientId) { + std::shared_ptr client = + ::ndk::SharedRefBase::make(mService); + EXPECT_TRUE(client != nullptr); + + // Register the client with the service. + int32_t clientId = 0; + Status status = mService->registerClient(client, kClientOpPackageName, kClientUseCallingUid, + kClientUseCallingPid, &clientId); + ALOGD("client id is %d", clientId); + EXPECT_TRUE(status.isOk()); + + // Validate the clientId. + EXPECT_TRUE(clientId > 0); + + // Check the number of Clients. + int32_t numOfClients; + status = mService->getNumOfClients(&numOfClients); + EXPECT_TRUE(status.isOk()); + EXPECT_EQ(1, numOfClients); + + // Unregister the client with invalid ID + bool res; + mService->unregisterClient(kInvalidClientId, &res); + EXPECT_FALSE(res); + + // Unregister the valid client. + mService->unregisterClient(clientId, &res); +} + +TEST_F(MediaTranscodingServiceTest, TestRegisterClientTwice) { + std::shared_ptr client = + ::ndk::SharedRefBase::make(mService); + EXPECT_TRUE(client != nullptr); + + // Register the client with the service. + int32_t clientId = 0; + Status status = mService->registerClient(client, kClientOpPackageName, kClientUseCallingUid, + kClientUseCallingPid, &clientId); + EXPECT_TRUE(status.isOk()); + + // Validate the clientId. + EXPECT_TRUE(clientId > 0); + + // Register the client again and expects failure. + status = mService->registerClient(client, kClientOpPackageName, kClientUseCallingUid, + kClientUseCallingPid, &clientId); + EXPECT_FALSE(status.isOk()); + + // Unregister the valid client. + bool res; + mService->unregisterClient(clientId, &res); +} + +} // namespace media +} // namespace android