From 3123d5905c8ae2c1c8a20159e36f189ea901fdb9 Mon Sep 17 00:00:00 2001 From: sergiuferentz Date: Mon, 26 Jun 2023 18:01:47 +0000 Subject: Fix for heap-use-after-free in GPUService.cpp This adds a unit test and fix for the bug reported by libfuzzer. Changes made: * Expose GPUService as testable code. * Update main_gpuservice.cpp to use the new GpuService now located at gpuservice/GpuService.h * Make initializer threads members of GpuService * Join the threads in destructor to prevent heap-use-after-free. * Add unit test that waits 3 seconds after deallocation to ensure no wrong access is made. Merged-In: I4d1d2d4658b575bf2c8f425f91f68f03114ad029 Bug: 282919145 Test: Added unit test and ran on device with ASAN Change-Id: I4d1d2d4658b575bf2c8f425f91f68f03114ad029 (cherry picked from commit 3c00cbc0f119c3f59325aa6d5061529feb58462b) --- services/gpuservice/Android.bp | 1 + services/gpuservice/GpuService.cpp | 14 ++-- services/gpuservice/GpuService.h | 93 --------------------- .../gpuservice/include/gpuservice/GpuService.h | 97 ++++++++++++++++++++++ services/gpuservice/main_gpuservice.cpp | 2 +- .../gpuservice/tests/fuzzers/GpuServiceFuzzer.cpp | 2 +- services/gpuservice/tests/unittests/Android.bp | 2 + .../gpuservice/tests/unittests/GpuServiceTest.cpp | 52 ++++++++++++ 8 files changed, 163 insertions(+), 100 deletions(-) delete mode 100644 services/gpuservice/GpuService.h create mode 100644 services/gpuservice/include/gpuservice/GpuService.h create mode 100644 services/gpuservice/tests/unittests/GpuServiceTest.cpp diff --git a/services/gpuservice/Android.bp b/services/gpuservice/Android.bp index fba64c7569..052efb6bbb 100644 --- a/services/gpuservice/Android.bp +++ b/services/gpuservice/Android.bp @@ -71,6 +71,7 @@ filegroup { cc_library_shared { name: "libgpuservice", defaults: ["libgpuservice_production_defaults"], + export_include_dirs: ["include"], srcs: [ ":libgpuservice_sources", ], diff --git a/services/gpuservice/GpuService.cpp b/services/gpuservice/GpuService.cpp index 7b9782f4e8..5643940a6e 100644 --- a/services/gpuservice/GpuService.cpp +++ b/services/gpuservice/GpuService.cpp @@ -16,7 +16,7 @@ #define ATRACE_TAG ATRACE_TAG_GRAPHICS -#include "GpuService.h" +#include "gpuservice/GpuService.h" #include #include @@ -34,6 +34,7 @@ #include #include +#include namespace android { @@ -55,18 +56,21 @@ GpuService::GpuService() mGpuStats(std::make_unique()), mGpuMemTracer(std::make_unique()) { - std::thread gpuMemAsyncInitThread([this]() { + mGpuMemAsyncInitThread = std::make_unique([this] (){ mGpuMem->initialize(); mGpuMemTracer->initialize(mGpuMem); }); - gpuMemAsyncInitThread.detach(); - std::thread gpuWorkAsyncInitThread([this]() { + mGpuWorkAsyncInitThread = std::make_unique([this]() { mGpuWork->initialize(); }); - gpuWorkAsyncInitThread.detach(); }; +GpuService::~GpuService() { + mGpuWorkAsyncInitThread->join(); + mGpuMemAsyncInitThread->join(); +} + void GpuService::setGpuStats(const std::string& driverPackageName, const std::string& driverVersionName, uint64_t driverVersionCode, int64_t driverBuildTime, const std::string& appPackageName, diff --git a/services/gpuservice/GpuService.h b/services/gpuservice/GpuService.h deleted file mode 100644 index d7313d165e..0000000000 --- a/services/gpuservice/GpuService.h +++ /dev/null @@ -1,93 +0,0 @@ -/* - * Copyright 2016 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. - */ - -#ifndef ANDROID_GPUSERVICE_H -#define ANDROID_GPUSERVICE_H - -#include -#include -#include -#include -#include - -#include -#include - -namespace android { - -namespace gpuwork { -class GpuWork; -} - -class GpuMem; -class GpuStats; -class GpuMemTracer; - -class GpuService : public BnGpuService, public PriorityDumper { -public: - static const char* const SERVICE_NAME ANDROID_API; - - GpuService() ANDROID_API; - -protected: - status_t shellCommand(int in, int out, int err, std::vector& args) override; - -private: - /* - * IGpuService interface - */ - void setGpuStats(const std::string& driverPackageName, const std::string& driverVersionName, - uint64_t driverVersionCode, int64_t driverBuildTime, - const std::string& appPackageName, const int32_t vulkanVersion, - GpuStatsInfo::Driver driver, bool isDriverLoaded, - int64_t driverLoadingTime) override; - void setTargetStats(const std::string& appPackageName, const uint64_t driverVersionCode, - const GpuStatsInfo::Stats stats, const uint64_t value) override; - void setUpdatableDriverPath(const std::string& driverPath) override; - std::string getUpdatableDriverPath() override; - - /* - * IBinder interface - */ - status_t dump(int fd, const Vector& args) override { return priorityDump(fd, args); } - - /* - * Debugging & dumpsys - */ - status_t dumpCritical(int fd, const Vector& /*args*/, bool asProto) override { - return doDump(fd, Vector(), asProto); - } - - status_t dumpAll(int fd, const Vector& args, bool asProto) override { - return doDump(fd, args, asProto); - } - - status_t doDump(int fd, const Vector& args, bool asProto); - - /* - * Attributes - */ - std::shared_ptr mGpuMem; - std::shared_ptr mGpuWork; - std::unique_ptr mGpuStats; - std::unique_ptr mGpuMemTracer; - std::mutex mLock; - std::string mDeveloperDriverPath; -}; - -} // namespace android - -#endif // ANDROID_GPUSERVICE_H diff --git a/services/gpuservice/include/gpuservice/GpuService.h b/services/gpuservice/include/gpuservice/GpuService.h new file mode 100644 index 0000000000..3e0ae66f39 --- /dev/null +++ b/services/gpuservice/include/gpuservice/GpuService.h @@ -0,0 +1,97 @@ +/* + * Copyright 2016 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. + */ + +#ifndef ANDROID_GPUSERVICE_H +#define ANDROID_GPUSERVICE_H + +#include +#include +#include +#include +#include + +#include +#include +#include + +namespace android { + +namespace gpuwork { +class GpuWork; +} + +class GpuMem; +class GpuStats; +class GpuMemTracer; + +class GpuService : public BnGpuService, public PriorityDumper { +public: + static const char* const SERVICE_NAME ANDROID_API; + + GpuService() ANDROID_API; + ~GpuService(); + +protected: + status_t shellCommand(int in, int out, int err, std::vector& args) override; + +private: + /* + * IGpuService interface + */ + void setGpuStats(const std::string& driverPackageName, const std::string& driverVersionName, + uint64_t driverVersionCode, int64_t driverBuildTime, + const std::string& appPackageName, const int32_t vulkanVersion, + GpuStatsInfo::Driver driver, bool isDriverLoaded, + int64_t driverLoadingTime) override; + void setTargetStats(const std::string& appPackageName, const uint64_t driverVersionCode, + const GpuStatsInfo::Stats stats, const uint64_t value) override; + void setUpdatableDriverPath(const std::string& driverPath) override; + std::string getUpdatableDriverPath() override; + + /* + * IBinder interface + */ + status_t dump(int fd, const Vector& args) override { return priorityDump(fd, args); } + + /* + * Debugging & dumpsys + */ + status_t dumpCritical(int fd, const Vector& /*args*/, bool asProto) override { + return doDump(fd, Vector(), asProto); + } + + status_t dumpAll(int fd, const Vector& args, bool asProto) override { + return doDump(fd, args, asProto); + } + + status_t doDump(int fd, const Vector& args, bool asProto); + + /* + * Attributes + */ + std::shared_ptr mGpuMem; + std::shared_ptr mGpuWork; + std::unique_ptr mGpuStats; + std::unique_ptr mGpuMemTracer; + std::mutex mLock; + std::string mDeveloperDriverPath; + std::unique_ptr mGpuMemAsyncInitThread; + std::unique_ptr mGpuWorkAsyncInitThread; +}; + +} // namespace android + +#endif // ANDROID_GPUSERVICE_H diff --git a/services/gpuservice/main_gpuservice.cpp b/services/gpuservice/main_gpuservice.cpp index 64aafcab6a..200237219e 100644 --- a/services/gpuservice/main_gpuservice.cpp +++ b/services/gpuservice/main_gpuservice.cpp @@ -18,7 +18,7 @@ #include #include #include -#include "GpuService.h" +#include "gpuservice/GpuService.h" using namespace android; diff --git a/services/gpuservice/tests/fuzzers/GpuServiceFuzzer.cpp b/services/gpuservice/tests/fuzzers/GpuServiceFuzzer.cpp index c2574a3fd3..241b8646e1 100644 --- a/services/gpuservice/tests/fuzzers/GpuServiceFuzzer.cpp +++ b/services/gpuservice/tests/fuzzers/GpuServiceFuzzer.cpp @@ -16,7 +16,7 @@ #include -#include "GpuService.h" +#include "gpuservice/GpuService.h" using ::android::fuzzService; using ::android::GpuService; diff --git a/services/gpuservice/tests/unittests/Android.bp b/services/gpuservice/tests/unittests/Android.bp index 51642f9472..c870b17b79 100644 --- a/services/gpuservice/tests/unittests/Android.bp +++ b/services/gpuservice/tests/unittests/Android.bp @@ -28,6 +28,7 @@ cc_test { "GpuMemTest.cpp", "GpuMemTracerTest.cpp", "GpuStatsTest.cpp", + "GpuServiceTest.cpp", ], header_libs: ["bpf_headers"], shared_libs: [ @@ -45,6 +46,7 @@ cc_test { "libstatslog", "libstatspull", "libutils", + "libgpuservice", ], static_libs: [ "libgmock", diff --git a/services/gpuservice/tests/unittests/GpuServiceTest.cpp b/services/gpuservice/tests/unittests/GpuServiceTest.cpp new file mode 100644 index 0000000000..62b3e53f53 --- /dev/null +++ b/services/gpuservice/tests/unittests/GpuServiceTest.cpp @@ -0,0 +1,52 @@ +#undef LOG_TAG +#define LOG_TAG "gpuservice_unittest" + +#include "gpuservice/GpuService.h" + +#include +#include + +#include +#include + +namespace android { +namespace { + +class GpuServiceTest : public testing::Test { +public: + GpuServiceTest() { + const ::testing::TestInfo* const test_info = + ::testing::UnitTest::GetInstance()->current_test_info(); + ALOGD("**** Setting up for %s.%s\n", test_info->test_case_name(), test_info->name()); + } + + ~GpuServiceTest() { + const ::testing::TestInfo* const test_info = + ::testing::UnitTest::GetInstance()->current_test_info(); + ALOGD("**** Tearing down after %s.%s\n", test_info->test_case_name(), test_info->name()); + } + +}; + + +/* +* The behaviour before this test + fixes was UB caused by threads accessing deallocated memory. +* +* This test creates the service (which initializes the culprit threads), +* deallocates it immediately and sleeps. +* +* GpuService's destructor gets called and joins the threads. +* If we haven't crashed by the time the sleep time has elapsed, we're good +* Let the test pass. +*/ +TEST_F(GpuServiceTest, onInitializeShouldNotCauseUseAfterFree) { + sp service = new GpuService(); + service.clear(); + std::this_thread::sleep_for(std::chrono::seconds(3)); + + // If we haven't crashed yet due to threads accessing freed up memory, let the test pass + EXPECT_TRUE(true); +} + +} // namespace +} // namespace android -- cgit v1.2.3-59-g8ed1b