Effect config parser: fix use after free on file path
ParsingResult::configPath is the path of the configuration file
used for the factory config parsing.
This path is used for an error log if the configuration file has errors.
The paths used to be a static string literals stored as char*
without lifecycle management.
When it was changed to dynamic strings, the code was not updated.
This patch changes it to a std::string.
Bug: 111261328
Bug: 117118292
Test: flash and check effect works
Change-Id: Ia2022c794936f3f75793371cdde86c3047bb6c0a
Signed-off-by: Kevin Rocard <krocard@google.com>
(cherry picked from commit a82fd60004272acc0ea25914cac429035082be21)
(cherry picked from commit 2fae5b7c8d457856f0896aafc61845ce5b209d4d)
diff --git a/media/libeffects/config/include/media/EffectsConfig.h b/media/libeffects/config/include/media/EffectsConfig.h
index 55b946f..fa0415b 100644
--- a/media/libeffects/config/include/media/EffectsConfig.h
+++ b/media/libeffects/config/include/media/EffectsConfig.h
@@ -96,7 +96,7 @@
/** Parsed config, nullptr if the xml lib could not load the file */
std::unique_ptr<Config> parsedConfig;
size_t nbSkippedElement; //< Number of skipped invalid library, effect or processing chain
- const char* configPath; //< Path to the loaded configuration
+ const std::string configPath; //< Path to the loaded configuration
};
/** Parses the provided effect configuration.
diff --git a/media/libeffects/config/src/EffectsConfig.cpp b/media/libeffects/config/src/EffectsConfig.cpp
index d79501f..351b1ee 100644
--- a/media/libeffects/config/src/EffectsConfig.cpp
+++ b/media/libeffects/config/src/EffectsConfig.cpp
@@ -250,14 +250,14 @@
return true;
}
-/** Internal version of the public parse(const char* path) with precondition `path != nullptr`. */
-ParsingResult parseWithPath(const char* path) {
+/** Internal version of the public parse(const char* path) where path always exist. */
+ParsingResult parseWithPath(std::string&& path) {
XMLDocument doc;
- doc.LoadFile(path);
+ doc.LoadFile(path.c_str());
if (doc.Error()) {
- ALOGE("Failed to parse %s: Tinyxml2 error (%d): %s", path,
+ ALOGE("Failed to parse %s: Tinyxml2 error (%d): %s", path.c_str(),
doc.ErrorID(), doc.ErrorStr());
- return {nullptr, 0, path};
+ return {nullptr, 0, std::move(path)};
}
auto config = std::make_unique<Config>();
@@ -295,7 +295,7 @@
}
}
}
- return {std::move(config), nbSkippedElements, path};
+ return {std::move(config), nbSkippedElements, std::move(path)};
}
}; // namespace
@@ -310,14 +310,14 @@
if (access(defaultPath.c_str(), R_OK) != 0) {
continue;
}
- auto result = parseWithPath(defaultPath.c_str());
+ auto result = parseWithPath(std::move(defaultPath));
if (result.parsedConfig != nullptr) {
return result;
}
}
ALOGE("Could not parse effect configuration in any of the default locations.");
- return {nullptr, 0, nullptr};
+ return {nullptr, 0, ""};
}
} // namespace effectsConfig
diff --git a/media/libeffects/factory/EffectsXmlConfigLoader.cpp b/media/libeffects/factory/EffectsXmlConfigLoader.cpp
index 7a7d431..052a88b 100644
--- a/media/libeffects/factory/EffectsXmlConfigLoader.cpp
+++ b/media/libeffects/factory/EffectsXmlConfigLoader.cpp
@@ -327,7 +327,8 @@
&gSkippedEffects, &gSubEffectList);
ALOGE_IF(result.nbSkippedElement != 0, "%zu errors during loading of configuration: %s",
- result.nbSkippedElement, result.configPath ?: "No config file found");
+ result.nbSkippedElement,
+ result.configPath.empty() ? "No config file found" : result.configPath.c_str());
return result.nbSkippedElement;
}
