Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 7598f6bf authored by Tim Van Patten's avatar Tim Van Patten
Browse files

gpuservice: Fix FeatureOverrides std::map crash 

The std::map copy constructor invoked by
FeatureOverrideParser::getFeatureOverrides() is crashing. The most
likely cause appears to be multiple threads attempting to update update
FeatureOverrideParser::mFeatureOverrides simultaneously while servicing
Binder requests during device boot. This hasn't been proven empirically
with a recreate though.

Remove this possibility by populating
FeatureOverrideParser::mFeatureOverrides as part of the
FeatureOverrideParser constructor. This is performed as part of the
GpuService constructor, before any threads are forked for Binder.

Additionally:
* FeatureOverrideParser::getFeatureOverrides() is renamed to
  getCachedFeatureOverrides() to make it clear it only returns the
  cached values.
* FeatureOverrideParser::forceFileRead() is removed, since it's no
  longer possible to re-read the file at run-time and repopulate
  FeatureOverrideParser::mFeatureOverrides.

While these changes should eliminate the crash, they also remove the
ability to update the file at run-time without restarting gpuservice.
The alternative is to acquire a mutex in
GpuService::getFeatureOverrides() (and
GpuService::cmdFeatureOverrides()) which could impact performance, since
it's called as part of every app launch. This will need to be evaluated
if reloading the file at run-time is necessary and added in the future.

Bug: b/425292768
Test: atest gpuservice_unittest
Flag: EXEMPT bugfix
Change-Id: I98f26bad040286a1d4eb0d59460f5f10601d3d38
parent 5e116051

services/gpuservice/GpuService.cpp

+6 −8
Original line number Diff line number Diff line
@@ -60,11 +60,14 @@ const std::string sAngleGlesDriverSuffix = "angle"; 


const char* const GpuService::SERVICE_NAME = "gpu";



const std::string kConfigFilePath = "/system/etc/angle/feature_config_vk.binarypb";



GpuService::GpuService()

      : mGpuMem(std::make_shared<GpuMem>()),

        mGpuWork(std::make_shared<gpuwork::GpuWork>()),

        mGpuStats(std::make_unique<GpuStats>()),

        mGpuMemTracer(std::make_unique<GpuMemTracer>()) {

        mGpuMemTracer(std::make_unique<GpuMemTracer>()),

        mFeatureOverrideParser(kConfigFilePath) {



    mGpuMemAsyncInitThread = std::make_unique<std::thread>([this] (){

        mGpuMem->initialize();
@@ -141,12 +144,7 @@ std::string GpuService::getPersistGraphicsEgl() { 
}



FeatureOverrides GpuService::getFeatureOverrides() {

    if (!graphicsenv_flags::angle_feature_overrides()) {

        FeatureOverrides featureOverrides;

        return featureOverrides;

    }



    return mFeatureOverrideParser.getFeatureOverrides();

    return mFeatureOverrideParser.getCachedFeatureOverrides();

}



void GpuService::setUpdatableDriverPath(const std::string& driverPath) {
@@ -245,7 +243,7 @@ status_t GpuService::doDump(int fd, const Vector<String16>& args, bool /*asProto 
}



status_t GpuService::cmdFeatureOverrides(int out, int /*err*/) {

    dprintf(out, "%s\n", mFeatureOverrideParser.getFeatureOverrides().toString().c_str());

    dprintf(out, "%s\n", mFeatureOverrideParser.getCachedFeatureOverrides().toString().c_str());

    return NO_ERROR;

}

services/gpuservice/feature_override/FeatureOverrideParser.cpp

+10 −37
Original line number Diff line number Diff line
@@ -112,38 +112,18 @@ feature_override::FeatureOverrideProtos readFeatureConfigProtos(const std::strin 


namespace android {



std::string FeatureOverrideParser::getFeatureOverrideFilePath() const {

    const std::string kConfigFilePath = "/system/etc/angle/feature_config_vk.binarypb";



    return kConfigFilePath;

}



bool FeatureOverrideParser::shouldReloadFeatureOverrides() const {

    std::string configFilePath = getFeatureOverrideFilePath();



    std::ifstream configFile(configFilePath);

    if (!configFile.good()) {

        return false;

    }



    struct stat fileStat{};

    if (stat(configFilePath.c_str(), &fileStat) != 0) {

        ALOGE("Error getting file information for '%s': %s", configFilePath.c_str(),

              strerror(errno));

        // stat'ing the file failed, so return false since reading it will also likely fail.

        return false;

FeatureOverrideParser::FeatureOverrideParser(const std::string &configFilePath) {

    // Parse the feature override values from the protobuf file in the ctor, before any gpuservice

    // threads are forked in main() for Binder. Otherwise, a lock would be required to prevent

    // multiple threads from updating mFeatureOverrides simultaneously.

    // Note that this prevents reading the file after the device boots if it's ever updated on the

    // device. That feature may require a lock in the future.

    parseFeatureOverrides(configFilePath);

}



    return fileStat.st_mtime > mLastProtobufReadTime;

}



void FeatureOverrideParser::forceFileRead() {

    mLastProtobufReadTime = 0;

}



void FeatureOverrideParser::parseFeatureOverrides() {

void FeatureOverrideParser::parseFeatureOverrides(const std::string &configFilePath) {

    const feature_override::FeatureOverrideProtos overridesProtos = readFeatureConfigProtos(

            getFeatureOverrideFilePath());

            configFilePath);



    // Clear out the stale values before adding the newly parsed data.

    resetFeatureOverrides(mFeatureOverrides);
@@ -187,16 +167,9 @@ void FeatureOverrideParser::parseFeatureOverrides() { 


        mFeatureOverrides.mPackageFeatures[packageName] = featureConfigs;

    }



    mLastProtobufReadTime = std::chrono::system_clock::to_time_t(

            std::chrono::system_clock::now());

}



FeatureOverrides FeatureOverrideParser::getFeatureOverrides() {

    if (shouldReloadFeatureOverrides()) {

        parseFeatureOverrides();

}



FeatureOverrides FeatureOverrideParser::getCachedFeatureOverrides() {

    return mFeatureOverrides;

}

services/gpuservice/feature_override/include/feature_override/FeatureOverrideParser.h

+3 −8
Original line number Diff line number Diff line
@@ -27,20 +27,15 @@ namespace android { 


class FeatureOverrideParser {

public:

    FeatureOverrideParser() = default;

    FeatureOverrideParser(const std::string &configFilePath);

    FeatureOverrideParser(const FeatureOverrideParser &) = default;

    virtual ~FeatureOverrideParser() = default;



    FeatureOverrides getFeatureOverrides();

    void forceFileRead();

    FeatureOverrides getCachedFeatureOverrides();



private:

    bool shouldReloadFeatureOverrides() const;

    void parseFeatureOverrides();

    // Allow FeatureOverrideParserMock to override with the unit test file's path.

    virtual std::string getFeatureOverrideFilePath() const;

    void parseFeatureOverrides(const std::string &configFilePath);



    std::time_t mLastProtobufReadTime = 0;

    FeatureOverrides mFeatureOverrides;

};

services/gpuservice/tests/unittests/Android.bp

+0 −18
Original line number Diff line number Diff line
@@ -56,23 +56,6 @@ genrule { 
    out: ["gpuservice_unittest_feature_config_vk.binarypb"],

}



// "Updated" protobuf, used to validate forceFileRead().

filegroup {

    name: "gpuservice_unittest_feature_config_vk_force_read_prototext",

    srcs: [

        "data/feature_config_test_force_read.txtpb",

    ],

}



genrule {

    name: "gpuservice_unittest_feature_config_vk_force_read_binarypb",

    defaults: ["gpuservice_unittest_feature_config_pb_defaults"],

    srcs: [

        ":gpuservice_unittest_feature_config_vk_force_read_prototext",

    ],

    out: ["gpuservice_unittest_feature_config_vk_force_read.binarypb"],

}



cc_test {

    name: "gpuservice_unittest",

    test_suites: ["device-tests"],
@@ -110,7 +93,6 @@ cc_test { 
    ],

    data: [

        ":gpuservice_unittest_feature_config_vk_binarypb",

        ":gpuservice_unittest_feature_config_vk_force_read_binarypb",

    ],

    require_root: true,

}

services/gpuservice/tests/unittests/FeatureOverrideParserTest.cpp

+17 −83
Original line number Diff line number Diff line
@@ -39,34 +39,22 @@ std::string getTestBinarypbPath(const std::string &filename) { 
    return path;

}



class FeatureOverrideParserMock : public FeatureOverrideParser {

public:

    MOCK_METHOD(std::string, getFeatureOverrideFilePath, (), (const, override));

};



class FeatureOverrideParserTest : public testing::Test {

public:

    const std::string kFilename = "gpuservice_unittest_feature_config_vk.binarypb";



    FeatureOverrideParserTest() {

        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());

    }



    ~FeatureOverrideParserTest() {

    ~FeatureOverrideParserTest() override {

        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());

    }



    void SetUp() override {

        const std::string filename = "gpuservice_unittest_feature_config_vk.binarypb";



        EXPECT_CALL(mFeatureOverrideParser, getFeatureOverrideFilePath())

            .WillRepeatedly(Return(getTestBinarypbPath(filename)));

    }



    FeatureOverrideParserMock mFeatureOverrideParser;

};



testing::AssertionResult validateFeatureConfigTestTxtpbSizes(FeatureOverrides overrides) {
@@ -87,24 +75,6 @@ testing::AssertionResult validateFeatureConfigTestTxtpbSizes(FeatureOverrides ov 
    return testing::AssertionSuccess();

}



testing::AssertionResult validateFeatureConfigTestForceReadTxtpbSizes(FeatureOverrides overrides) {

    size_t expectedGlobalFeaturesSize = 1;

    if (overrides.mGlobalFeatures.size() != expectedGlobalFeaturesSize) {

        return testing::AssertionFailure()

                << "overrides.mGlobalFeatures.size(): " << overrides.mGlobalFeatures.size()

                << ", expected: " << expectedGlobalFeaturesSize;

    }



    size_t expectedPackageFeaturesSize = 0;

    if (overrides.mPackageFeatures.size() != expectedPackageFeaturesSize) {

        return testing::AssertionFailure()

                << "overrides.mPackageFeatures.size(): " << overrides.mPackageFeatures.size()

                << ", expected: " << expectedPackageFeaturesSize;

    }



    return testing::AssertionSuccess();

}



testing::AssertionResult validateGlobalOverrides1(FeatureOverrides overrides) {

    const int kTestFeatureIndex = 0;

    const std::string expectedFeatureName = "globalOverrides1";
@@ -127,7 +97,8 @@ testing::AssertionResult validateGlobalOverrides1(FeatureOverrides overrides) { 
}



TEST_F(FeatureOverrideParserTest, globalOverrides1) {

    FeatureOverrides overrides = mFeatureOverrideParser.getFeatureOverrides();

    FeatureOverrideParser featureOverrideParser(getTestBinarypbPath(kFilename));

    FeatureOverrides overrides = featureOverrideParser.getCachedFeatureOverrides();



    EXPECT_TRUE(validateFeatureConfigTestTxtpbSizes(overrides));

    EXPECT_TRUE(validateGlobalOverrides1(overrides));
@@ -173,7 +144,8 @@ testing::AssertionResult validateGlobalOverrides2(FeatureOverrides overrides) { 
}



TEST_F(FeatureOverrideParserTest, globalOverrides2) {

    FeatureOverrides overrides = mFeatureOverrideParser.getFeatureOverrides();

    FeatureOverrideParser featureOverrideParser(getTestBinarypbPath(kFilename));

    FeatureOverrides overrides = featureOverrideParser.getCachedFeatureOverrides();



    EXPECT_TRUE(validateGlobalOverrides2(overrides));

}
@@ -218,7 +190,8 @@ testing::AssertionResult validateGlobalOverrides3(FeatureOverrides overrides) { 
}



TEST_F(FeatureOverrideParserTest, globalOverrides3) {

FeatureOverrides overrides = mFeatureOverrideParser.getFeatureOverrides();

    FeatureOverrideParser featureOverrideParser(getTestBinarypbPath(kFilename));

    FeatureOverrides overrides = featureOverrideParser.getCachedFeatureOverrides();



    EXPECT_TRUE(validateGlobalOverrides3(overrides));

}
@@ -262,33 +235,13 @@ testing::AssertionResult validatePackageOverrides1(FeatureOverrides overrides) { 
}



TEST_F(FeatureOverrideParserTest, packageOverrides1) {

    FeatureOverrides overrides = mFeatureOverrideParser.getFeatureOverrides();

    FeatureOverrideParser featureOverrideParser(getTestBinarypbPath(kFilename));

    FeatureOverrides overrides = featureOverrideParser.getCachedFeatureOverrides();



    EXPECT_TRUE(validateFeatureConfigTestTxtpbSizes(overrides));

    EXPECT_TRUE(validatePackageOverrides1(overrides));

}



testing::AssertionResult validateForceFileRead(FeatureOverrides overrides) {

    const int kTestFeatureIndex = 0;

    const std::string expectedFeatureName = "forceFileRead";



    const FeatureConfig &cfg = overrides.mGlobalFeatures[kTestFeatureIndex];

    if (cfg.mFeatureName != expectedFeatureName) {

        return testing::AssertionFailure()

                << "cfg.mFeatureName: " << cfg.mFeatureName

                << ", expected: " << expectedFeatureName;

    }



    bool expectedEnabled = false;

    if (cfg.mEnabled != expectedEnabled) {

        return testing::AssertionFailure()

                << "cfg.mEnabled: " << cfg.mEnabled

                << ", expected: " << expectedEnabled;

    }



    return testing::AssertionSuccess();

}



testing::AssertionResult validatePackageOverrides2(FeatureOverrides overrides) {

    const std::string expectedPackageName = "com.gpuservice_unittest.packageOverrides2";
@@ -344,7 +297,8 @@ testing::AssertionResult validatePackageOverrides2(FeatureOverrides overrides) { 
}



TEST_F(FeatureOverrideParserTest, packageOverrides2) {

    FeatureOverrides overrides = mFeatureOverrideParser.getFeatureOverrides();

    FeatureOverrideParser featureOverrideParser(getTestBinarypbPath(kFilename));

    FeatureOverrides overrides = featureOverrideParser.getCachedFeatureOverrides();



    EXPECT_TRUE(validatePackageOverrides2(overrides));

}
@@ -438,31 +392,11 @@ testing::AssertionResult validatePackageOverrides3(FeatureOverrides overrides) { 
}



TEST_F(FeatureOverrideParserTest, packageOverrides3) {

FeatureOverrides overrides = mFeatureOverrideParser.getFeatureOverrides();

    FeatureOverrideParser featureOverrideParser(getTestBinarypbPath(kFilename));

    FeatureOverrides overrides = featureOverrideParser.getCachedFeatureOverrides();



    EXPECT_TRUE(validatePackageOverrides3(overrides));

}



TEST_F(FeatureOverrideParserTest, forceFileRead) {

    FeatureOverrides overrides = mFeatureOverrideParser.getFeatureOverrides();



    // Validate the "original" contents are present.

    EXPECT_TRUE(validateFeatureConfigTestTxtpbSizes(overrides));

    EXPECT_TRUE(validateGlobalOverrides1(overrides));



    // "Update" the config file.

    const std::string filename = "gpuservice_unittest_feature_config_vk_force_read.binarypb";

    EXPECT_CALL(mFeatureOverrideParser, getFeatureOverrideFilePath())

        .WillRepeatedly(Return(getTestBinarypbPath(filename)));



    mFeatureOverrideParser.forceFileRead();



    overrides = mFeatureOverrideParser.getFeatureOverrides();



    // Validate the new file contents were read and parsed.

    EXPECT_TRUE(validateFeatureConfigTestForceReadTxtpbSizes(overrides));

    EXPECT_TRUE(validateForceFileRead(overrides));

}



} // namespace

} // namespace android