Fix allowlist for unreliable pinning.
When checking IsUnreliablePinningAllowed, the existing code calls
IsTestDir on a value starts with "/data", but IsTestDir expects
metadata_dir_. Fix it.
Also, make subdir checks more robust and add test cases for IsSubdir.
Fixes: 162557082
Test: vts_libsnapshot_test
Test: fiemap_image_test
Change-Id: I495cfae3da11d1e0800b8abf520df10dd9a29dce
diff --git a/fs_mgr/libfiemap/image_manager.cpp b/fs_mgr/libfiemap/image_manager.cpp
index 3ee742f..f32e0eb 100644
--- a/fs_mgr/libfiemap/image_manager.cpp
+++ b/fs_mgr/libfiemap/image_manager.cpp
@@ -136,13 +136,13 @@
return !!FindPartition(*metadata.get(), name);
}
-static bool IsTestDir(const std::string& path) {
- return android::base::StartsWith(path, kTestImageMetadataDir) ||
- android::base::StartsWith(path, kOtaTestImageMetadataDir);
+bool ImageManager::MetadataDirIsTest() const {
+ return IsSubdir(metadata_dir_, kTestImageMetadataDir) ||
+ IsSubdir(metadata_dir_, kOtaTestImageMetadataDir);
}
-static bool IsUnreliablePinningAllowed(const std::string& path) {
- return android::base::StartsWith(path, "/data/gsi/dsu/") || IsTestDir(path);
+bool ImageManager::IsUnreliablePinningAllowed() const {
+ return IsSubdir(data_dir_, "/data/gsi/dsu/") || MetadataDirIsTest();
}
FiemapStatus ImageManager::CreateBackingImage(
@@ -159,7 +159,7 @@
if (!FilesystemHasReliablePinning(data_path, &reliable_pinning)) {
return FiemapStatus::Error();
}
- if (!reliable_pinning && !IsUnreliablePinningAllowed(data_path)) {
+ if (!reliable_pinning && !IsUnreliablePinningAllowed()) {
// For historical reasons, we allow unreliable pinning for certain use
// cases (DSUs, testing) because the ultimate use case is either
// developer-oriented or ephemeral (the intent is to boot immediately
@@ -178,7 +178,7 @@
// if device-mapper is stacked in some complex way not supported by
// FiemapWriter.
auto device_path = GetDevicePathForFile(fw.get());
- if (android::base::StartsWith(device_path, "/dev/block/dm-") && !IsTestDir(metadata_dir_)) {
+ if (android::base::StartsWith(device_path, "/dev/block/dm-") && !MetadataDirIsTest()) {
LOG(ERROR) << "Cannot persist images against device-mapper device: " << device_path;
fw = {};
diff --git a/fs_mgr/libfiemap/image_test.cpp b/fs_mgr/libfiemap/image_test.cpp
index 6663391..6d09751 100644
--- a/fs_mgr/libfiemap/image_test.cpp
+++ b/fs_mgr/libfiemap/image_test.cpp
@@ -34,10 +34,13 @@
#include <libdm/dm.h>
#include <libfiemap/image_manager.h>
+#include "utility.h"
+
using namespace android::dm;
using namespace std::literals;
using android::base::unique_fd;
using android::fiemap::ImageManager;
+using android::fiemap::IsSubdir;
using android::fs_mgr::BlockDeviceInfo;
using android::fs_mgr::PartitionOpener;
using android::fs_mgr::WaitForFile;
@@ -131,6 +134,51 @@
ASSERT_TRUE(manager_->UnmapImageDevice(base_name_));
}
+namespace {
+
+struct IsSubdirTestParam {
+ std::string child;
+ std::string parent;
+ bool result;
+};
+
+class IsSubdirTest : public ::testing::TestWithParam<IsSubdirTestParam> {};
+
+TEST_P(IsSubdirTest, Test) {
+ const auto& param = GetParam();
+ EXPECT_EQ(param.result, IsSubdir(param.child, param.parent))
+ << "IsSubdir(child=\"" << param.child << "\", parent=\"" << param.parent
+ << "\") != " << (param.result ? "true" : "false");
+}
+
+std::vector<IsSubdirTestParam> IsSubdirTestValues() {
+ // clang-format off
+ std::vector<IsSubdirTestParam> base_cases{
+ {"/foo/bar", "/foo", true},
+ {"/foo/bar/baz", "/foo", true},
+ {"/foo", "/foo", true},
+ {"/foo", "/", true},
+ {"/", "/", true},
+ {"/foo", "/foo/bar", false},
+ {"/foo", "/bar", false},
+ {"/foo-bar", "/foo", false},
+ {"/", "/foo", false},
+ };
+ // clang-format on
+ std::vector<IsSubdirTestParam> ret;
+ for (const auto& e : base_cases) {
+ ret.push_back(e);
+ ret.push_back({e.child + "/", e.parent, e.result});
+ ret.push_back({e.child, e.parent + "/", e.result});
+ ret.push_back({e.child + "/", e.parent + "/", e.result});
+ }
+ return ret;
+}
+
+INSTANTIATE_TEST_SUITE_P(IsSubdirTest, IsSubdirTest, ::testing::ValuesIn(IsSubdirTestValues()));
+
+} // namespace
+
bool Mkdir(const std::string& path) {
if (mkdir(path.c_str(), 0700) && errno != EEXIST) {
std::cerr << "Could not mkdir " << path << ": " << strerror(errno) << std::endl;
diff --git a/fs_mgr/libfiemap/include/libfiemap/image_manager.h b/fs_mgr/libfiemap/include/libfiemap/image_manager.h
index 60b98dc..50f4f33 100644
--- a/fs_mgr/libfiemap/include/libfiemap/image_manager.h
+++ b/fs_mgr/libfiemap/include/libfiemap/image_manager.h
@@ -176,6 +176,8 @@
bool MapWithDmLinear(const IPartitionOpener& opener, const std::string& name,
const std::chrono::milliseconds& timeout_ms, std::string* path);
bool UnmapImageDevice(const std::string& name, bool force);
+ bool IsUnreliablePinningAllowed() const;
+ bool MetadataDirIsTest() const;
ImageManager(const ImageManager&) = delete;
ImageManager& operator=(const ImageManager&) = delete;
diff --git a/fs_mgr/libfiemap/utility.cpp b/fs_mgr/libfiemap/utility.cpp
index bbb0510..67c929e 100644
--- a/fs_mgr/libfiemap/utility.cpp
+++ b/fs_mgr/libfiemap/utility.cpp
@@ -168,5 +168,30 @@
return F2fsPinBeforeAllocate(fd, supported);
}
+bool IsSubdir(const std::string& child, const std::string& parent) {
+ // Precondition: both are absolute paths.
+ CHECK(android::base::StartsWith(child, "/")) << "Not an absolute path: " << child;
+ CHECK(android::base::StartsWith(parent, "/")) << "Not an absolute path: " << parent;
+
+ // Remove extraneous "/" at the end.
+ std::string_view child_sv = child;
+ while (child_sv != "/" && android::base::ConsumeSuffix(&child_sv, "/"))
+ ;
+
+ std::string_view parent_sv = parent;
+ while (parent_sv != "/" && android::base::ConsumeSuffix(&parent_sv, "/"))
+ ;
+
+ // IsSubdir(anything, "/") => true
+ if (parent_sv == "/") return true;
+
+ // IsSubdir("/foo", "/foo") => true
+ if (parent_sv == child_sv) return true;
+
+ // IsSubdir("/foo/bar", "/foo") => true
+ // IsSubdir("/foo-bar", "/foo") => false
+ return android::base::StartsWith(child_sv, std::string(parent_sv) + "/");
+}
+
} // namespace fiemap
} // namespace android
diff --git a/fs_mgr/libfiemap/utility.h b/fs_mgr/libfiemap/utility.h
index 4c0bc2b..aa40f79 100644
--- a/fs_mgr/libfiemap/utility.h
+++ b/fs_mgr/libfiemap/utility.h
@@ -51,5 +51,9 @@
// cases (such as snapshots or adb remount).
bool FilesystemHasReliablePinning(const std::string& file, bool* supported);
+// Crude implementation to check if |child| is a subdir of |parent|.
+// Assume both are absolute paths.
+bool IsSubdir(const std::string& child, const std::string& parent);
+
} // namespace fiemap
} // namespace android