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

Commit 900fdf47 authored by Yifan Hong's avatar Yifan Hong
Browse files

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
parent 7c95de75

fs_mgr/libfiemap/image_manager.cpp

+7 −7
Original line number Diff line number Diff line
@@ -136,13 +136,13 @@ bool ImageManager::BackingImageExists(const std::string& name) { 
    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 @@ FiemapStatus ImageManager::CreateBackingImage( 
    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 @@ FiemapStatus ImageManager::CreateBackingImage( 
    // 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 = {};

fs_mgr/libfiemap/image_test.cpp

+48 −0
Original line number Diff line number Diff line
@@ -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 @@ TEST_F(NativeTest, GetMappedImageDevice) { 
    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;

fs_mgr/libfiemap/include/libfiemap/image_manager.h

+2 −0
Original line number Diff line number Diff line
@@ -176,6 +176,8 @@ class ImageManager final : public IImageManager { 
    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;

fs_mgr/libfiemap/utility.cpp

+25 −0
Original line number Diff line number Diff line
@@ -168,5 +168,30 @@ bool FilesystemHasReliablePinning(const std::string& file, bool* supported) { 
    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

fs_mgr/libfiemap/utility.h

+4 −0
Original line number Diff line number Diff line
@@ -51,5 +51,9 @@ bool BlockDeviceToName(uint32_t major, uint32_t minor, std::string* bdev_name); 
// 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