Do not allow duplicated major version across <hal> in manifests
<hal> entries has to be grouped by versions. Otherwise, if
<transport> is different in two <hal> entries with the same
<name> and <version>, getTransport always return the first match.
As a side effect, fixes an incorrect behavior in
HalManifest::isCompatible that test for <instance> tags across two
<hal>'s with the same name and version (because it shouldn't occur
at all). This incorrect behavior is hidden under an
EXPECT_FALSE in libvintf_test. Now that this is never allowed,
the test is changed to EXPECT_TRUE.
Test: libvintf_test
Change-Id: I10d4619f3828bf695485938c315c73a924ba6340
Merged-In: I10d4619f3828bf695485938c315c73a924ba6340
Fixes: 38502835
diff --git a/HalManifest.cpp b/HalManifest.cpp
index f6898f3..fbbfa2e 100644
--- a/HalManifest.cpp
+++ b/HalManifest.cpp
@@ -20,7 +20,9 @@
#include "HalManifest.h"
#include <dirent.h>
+
#include <mutex>
+#include <set>
#include "parse_string.h"
#include "parse_xml.h"
@@ -32,11 +34,32 @@
constexpr Version HalManifest::kVersion;
-bool HalManifest::add(ManifestHal &&hal) {
+// Check <version> tag for all <hal> with the same name.
+bool HalManifest::shouldAdd(const ManifestHal& hal) const {
if (!hal.isValid()) {
return false;
}
- mHals.emplace(hal.name, std::move(hal));
+ auto existingHals = mHals.equal_range(hal.name);
+ std::set<size_t> existingMajorVersions;
+ for (auto it = existingHals.first; it != existingHals.second; ++it) {
+ for (const auto& v : it->second.versions) {
+ // Assume integrity on existingHals, so no check on emplace().second
+ existingMajorVersions.insert(v.majorVer);
+ }
+ }
+ for (const auto& v : hal.versions) {
+ if (!existingMajorVersions.emplace(v.majorVer).second /* no insertion */) {
+ return false;
+ }
+ }
+ return true;
+}
+
+bool HalManifest::add(ManifestHal&& hal) {
+ if (!shouldAdd(hal)) {
+ return false;
+ }
+ mHals.emplace(hal.name, std::move(hal)); // always succeed
return true;
}
diff --git a/include/vintf/HalManifest.h b/include/vintf/HalManifest.h
index 8c6e4b0..4a983d1 100644
--- a/include/vintf/HalManifest.h
+++ b/include/vintf/HalManifest.h
@@ -131,6 +131,9 @@
friend std::string dump(const HalManifest &vm);
friend bool operator==(const HalManifest &lft, const HalManifest &rgt);
+ // Check before add()
+ bool shouldAdd(const ManifestHal& toAdd) const;
+
// Return an iterable to all ManifestHal objects. Call it as follows:
// for (const ManifestHal &e : vm.getHals()) { }
ConstMultiMapValueIterable<std::string, ManifestHal> getHals() const;
diff --git a/parse_xml.cpp b/parse_xml.cpp
index f2f3096..0ac87cb 100644
--- a/parse_xml.cpp
+++ b/parse_xml.cpp
@@ -651,8 +651,9 @@
}
}
for (auto &&hal : hals) {
+ std::string description{hal.name};
if (!object->add(std::move(hal))) {
- this->mLastError = "Duplicated manifest.hal entry";
+ this->mLastError = "Duplicated manifest.hal entry " + description;
return false;
}
}
diff --git a/test/main.cpp b/test/main.cpp
index 63d0bf5..343f327 100644
--- a/test/main.cpp
+++ b/test/main.cpp
@@ -274,6 +274,67 @@
"</manifest>"));
}
+TEST_F(LibVintfTest, HalManifestDuplicate) {
+ HalManifest vm;
+ EXPECT_FALSE(gHalManifestConverter(&vm,
+ "<manifest version=\"1.0\" type=\"device\">"
+ " <hal>"
+ " <name>android.hidl.manager</name>"
+ " <transport>hwbinder</transport>"
+ " <version>1.0</version>"
+ " <version>1.1</version>"
+ " </hal>"
+ "</manifest>"))
+ << "Should not allow duplicated major version in <hal>";
+ EXPECT_FALSE(gHalManifestConverter(&vm,
+ "<manifest version=\"1.0\" type=\"device\">"
+ " <hal>"
+ " <name>android.hidl.manager</name>"
+ " <transport>hwbinder</transport>"
+ " <version>1.0</version>"
+ " </hal>"
+ " <hal>"
+ " <name>android.hidl.manager</name>"
+ " <transport arch=\"32+64\">passthrough</transport>"
+ " <version>1.1</version>"
+ " </hal>"
+ "</manifest>"))
+ << "Should not allow duplicated major version across <hal>";
+}
+
+TEST_F(LibVintfTest, HalManifestGetTransport) {
+ HalManifest vm;
+ EXPECT_TRUE(gHalManifestConverter(&vm,
+ "<manifest version=\"1.0\" type=\"device\">"
+ " <hal>"
+ " <name>android.hidl.manager</name>"
+ " <transport>hwbinder</transport>"
+ " <version>1.0</version>"
+ " <interface>"
+ " <name>IServiceManager</name>"
+ " <instance>default</instance>"
+ " </interface>"
+ " </hal>"
+ " <hal>"
+ " <name>android.hidl.manager</name>"
+ " <transport arch=\"32+64\">passthrough</transport>"
+ " <version>2.1</version>"
+ " <interface>"
+ " <name>IServiceManager</name>"
+ " <instance>default</instance>"
+ " </interface>"
+ " </hal>"
+ "</manifest>"));
+ EXPECT_EQ(Transport::PASSTHROUGH,
+ vm.getTransport("android.hidl.manager", {2, 1}, "IServiceManager", "default"));
+ EXPECT_EQ(Transport::PASSTHROUGH,
+ vm.getTransport("android.hidl.manager", {2, 0}, "IServiceManager", "default"));
+ EXPECT_EQ(Transport::EMPTY,
+ vm.getTransport("android.hidl.manager", {2, 2}, "IServiceManager", "default"));
+ EXPECT_EQ(Transport::HWBINDER,
+ vm.getTransport("android.hidl.manager", {1, 0}, "IServiceManager", "default"));
+}
+
TEST_F(LibVintfTest, HalManifestInstances) {
HalManifest vm = testDeviceManifest();
EXPECT_EQ(vm.getInstances("android.hardware.camera", "ICamera"),
@@ -882,14 +943,6 @@
" <interface>\n"
" <name>IFoo</name>\n"
" <instance>default</instance>\n"
- " </interface>\n"
- " </hal>\n"
- " <hal format=\"hidl\">\n"
- " <name>android.hardware.foo</name>\n"
- " <transport>hwbinder</transport>\n"
- " <version>1.0</version>\n"
- " <interface>\n"
- " <name>IFoo</name>\n"
" <instance>specific</instance>\n"
" </interface>\n"
" </hal>\n"
@@ -907,11 +960,9 @@
" </sepolicy>\n"
"</manifest>\n";
HalManifest manifest;
- EXPECT_TRUE(gHalManifestConverter(&manifest, manifestXml));
- EXPECT_FALSE(manifest.checkCompatibility(matrix, &error))
- << "should be compatible even though @1.0::IFoo/default "
- << "and @1.0::IFoo/specific are splitted in two <hal>: "
- << error;
+ EXPECT_TRUE(gHalManifestConverter(&manifest, manifestXml))
+ << gHalManifestConverter.lastError();
+ EXPECT_TRUE(manifest.checkCompatibility(matrix, &error)) << error;
}
}
@@ -936,7 +987,6 @@
" <name>android.hardware.nfc</name>\n"
" <transport>hwbinder</transport>\n"
" <version>1.0</version>\n"
- " <version>2.0</version>\n"
" <interface>\n"
" <name>INfc</name>\n"
" <instance>nfc_nci</instance>\n"
@@ -949,6 +999,7 @@
" <interface>\n"
" <name>INfc</name>\n"
" <instance>default</instance>\n"
+ " <instance>nfc_nci</instance>\n"
" </interface>\n"
" </hal>\n"
" <sepolicy>\n"
