HalManifest rejects names invalid to FqInstance
With the legacy package name + version + interface name
+ instance name format, if the names can't fit into
an FqInstance (e.g. interface name does not start with I),
forEachInstance silently drops the instance.
Detect this early so that such HAL manifests are considered invalid.
If a device relies on this behavior and contains invalid characters in
the names, it will encounter a build / runtime error.
Test: libvintf_test
Bug: 175821740
Change-Id: I2a7e3e9a5d2784a245560bcbf2f8abb512fdb0e1
diff --git a/Android.bp b/Android.bp
index f148541..1eb0c8c 100644
--- a/Android.bp
+++ b/Android.bp
@@ -68,6 +68,7 @@
"TransportArch.cpp",
"VintfObject.cpp",
"XmlFile.cpp",
+ "utils.cpp",
],
shared_libs: libvintf_private_deps,
header_libs: [
diff --git a/ManifestHal.cpp b/ManifestHal.cpp
index cb60331..6bc86d3 100644
--- a/ManifestHal.cpp
+++ b/ManifestHal.cpp
@@ -15,15 +15,19 @@
*/
#include "ManifestHal.h"
+
#include <unordered_set>
#include "MapValueIterator.h"
#include "constants-private.h"
#include "parse_string.h"
+#include "utils.h"
namespace android {
namespace vintf {
+using details::canConvertToFqInstance;
+
bool ManifestHal::isValid(std::string* error) const {
if (error) {
error->clear();
@@ -38,13 +42,29 @@
}
success = false;
if (error) {
- *error += "Duplicated major version: " + to_string(v) + " vs. " + to_string(it->second);
+ *error += "Duplicated major version: " + to_string(v) + " vs. " +
+ to_string(it->second) + ".\n";
}
}
+
+ // Check legacy instances (i.e. <version> + <interface> + <instance>) can be
+ // converted into FqInstance because forEachInstance relies on FqInstance.
+ for (const auto& v : versions) {
+ for (const auto& intf : iterateValues(interfaces)) {
+ intf.forEachInstance(
+ [&](const auto& interface, const auto& instance, bool /* isRegex */) {
+ if (!canConvertToFqInstance(getName(), v, interface, instance, format, error)) {
+ success = false;
+ }
+ return true; // continue
+ });
+ }
+ }
+
std::string transportArchError;
if (!transportArch.isValid(&transportArchError)) {
success = false;
- if (error) *error += transportArchError;
+ if (error) *error += transportArchError + "\n";
}
return success;
}
diff --git a/include/vintf/ManifestHal.h b/include/vintf/ManifestHal.h
index a2837fd..020a962 100644
--- a/include/vintf/ManifestHal.h
+++ b/include/vintf/ManifestHal.h
@@ -65,6 +65,8 @@
inline Arch arch() const { return transportArch.arch; }
inline const std::string& getName() const { return name; }
+
+ // Assume isValid().
bool forEachInstance(const std::function<bool(const ManifestInstance&)>& func) const;
bool isOverride() const { return mIsOverride; }
diff --git a/test/LibVintfTest.cpp b/test/LibVintfTest.cpp
index 7df0907..0794607 100644
--- a/test/LibVintfTest.cpp
+++ b/test/LibVintfTest.cpp
@@ -35,6 +35,7 @@
using ::testing::ElementsAre;
using ::testing::Eq;
+using ::testing::HasSubstr;
using ::testing::Property;
using ::testing::SizeIs;
@@ -4283,6 +4284,44 @@
"5.4.42-android12-0-something", nullptr, &level));
}
+TEST_F(LibVintfTest, HalManifestMissingI) {
+ // If package name, interface or instance contains characters invalid to FqInstance,
+ // it must be rejected because forEachInstance requires them to fit into FqInstance.
+ std::string xml = "<manifest " + kMetaVersionStr + R"( type="framework">
+ <hal format="aidl">
+ <name>android.frameworks.foo</name>
+ <version>1</version>
+ <interface>
+ <name>MyFoo</name>
+ <instance>default</instance>
+ </interface>
+ </hal>
+ </manifest>)";
+ HalManifest manifest;
+ std::string error;
+ ASSERT_FALSE(gHalManifestConverter(&manifest, xml, &error)) << "Should not be valid:\n" << xml;
+ EXPECT_THAT(error, HasSubstr("Interface 'MyFoo' should have the format I[a-zA-Z0-9_]*"));
+}
+
+TEST_F(LibVintfTest, HalManifestInvalidPackage) {
+ // If package name, interface or instance contains characters invalid to FqInstance,
+ // it must be rejected because forEachInstance requires them to fit into FqInstance.
+ std::string xml = "<manifest " + kMetaVersionStr + R"( type="framework">
+ <hal format="aidl">
+ <name>not_a_valid_package!</name>
+ <version>1</version>
+ <interface>
+ <name>MyFoo</name>
+ <instance>default</instance>
+ </interface>
+ </hal>
+ </manifest>)";
+ HalManifest manifest;
+ std::string error;
+ ASSERT_FALSE(gHalManifestConverter(&manifest, xml, &error)) << "Should not be valid:\n" << xml;
+ EXPECT_THAT(error, HasSubstr("not_a_valid_package!"));
+}
+
// clang-format off
struct FrameworkCompatibilityMatrixCombineTest : public LibVintfTest {
diff --git a/utils.cpp b/utils.cpp
new file mode 100644
index 0000000..db208ec
--- /dev/null
+++ b/utils.cpp
@@ -0,0 +1,68 @@
+/*
+ * Copyright (C) 2021 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.
+ */
+#include "utils.h"
+
+#include <sstream>
+
+#include "parse_string.h"
+
+namespace android::vintf::details {
+
+bool canConvertToFqInstance(const std::string& package, const Version& version,
+ const std::string& interface, const std::string& instance,
+ HalFormat format, std::string* appendedError) {
+ if (FqInstance::from(package, version.majorVer, version.minorVer, interface, instance)
+ .has_value()) {
+ return true;
+ }
+ if (appendedError == nullptr) {
+ return false;
+ }
+
+ // Attempt to construct a good error message.
+ std::stringstream ss;
+ ss << "Invalid instance: '";
+ if (format == HalFormat::AIDL) {
+ ss << toAidlFqnameString(package, interface, instance) << " (@" << version.minorVer << ")";
+ } else {
+ ss << toFQNameString(package, version, interface, instance);
+ }
+ ss << "'. ";
+
+ // Attempt to guess the source of error.
+ bool foundError = false;
+ if (!FqInstance::from(package).has_value()) {
+ ss << "Package '" << package
+ << "' should have the format [a-zA-Z_][a-zA-Z0-9_]*(\\.[a-zA-Z_][a-zA-Z0-9_]*)*";
+ foundError = true;
+ }
+
+ std::optional<FqInstance> convertedInterface = FqInstance::from(interface);
+ if (!convertedInterface.has_value() || !convertedInterface->hasInterface()) {
+ ss << "Interface '" << interface << "' should have the format I[a-zA-Z0-9_]*";
+ foundError = true;
+ }
+
+ if (!foundError) {
+ ss << "Unknown error.";
+ }
+ ss << "\n";
+
+ *appendedError += ss.str();
+ return false;
+}
+
+} // namespace android::vintf::details
diff --git a/utils.h b/utils.h
index 83cb874..d207546 100644
--- a/utils.h
+++ b/utils.h
@@ -106,6 +106,13 @@
return false;
}
+// Check legacy instances (i.e. <version> + <interface> + <instance>) can be
+// converted into FqInstance because forEachInstance relies on FqInstance.
+// If error and appendedError is not null, error message is appended to appendedError.
+[[nodiscard]] bool canConvertToFqInstance(const std::string& package, const Version& version,
+ const std::string& interface, const std::string& instance,
+ HalFormat format, std::string* appendedError);
+
} // namespace details
} // namespace vintf
} // namespace android