From e5e2ec461b156845358815787cdb527a3e4d5ff8 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 12 Sep 2024 13:20:44 -0400 Subject: [PATCH 1/5] [Daemon] fix symlinks not resolving at mount time --- include/multipass/vm_mount.h | 2 ++ src/daemon/daemon.cpp | 8 +++++--- src/daemon/daemon.h | 2 +- src/utils/vm_mount.cpp | 33 +++++++++++++++++++++++++++++++++ 4 files changed, 41 insertions(+), 4 deletions(-) diff --git a/include/multipass/vm_mount.h b/include/multipass/vm_mount.h index b3ab98b7d1..25fedf8877 100644 --- a/include/multipass/vm_mount.h +++ b/include/multipass/vm_mount.h @@ -39,6 +39,8 @@ class VMMount explicit VMMount(const QJsonObject& json); VMMount(const std::string& sourcePath, id_mappings gidMappings, id_mappings uidMappings, MountType mountType); + void resolve_source_path(); + QJsonObject serialize() const; const std::string& get_source_path() const noexcept; diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index a38552498e..957a6dd0bf 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -3270,11 +3270,13 @@ bool mp::Daemon::create_missing_mounts(std::unordered_map& return mount_specs.size() != initial_mount_count; } -mp::MountHandler::UPtr mp::Daemon::make_mount(VirtualMachine* vm, const std::string& target, const VMMount& mount) +mp::MountHandler::UPtr mp::Daemon::make_mount(VirtualMachine* vm, const std::string& target, VMMount mount) { + mount.resolve_source_path(); + return mount.get_mount_type() == VMMount::MountType::Classic - ? std::make_unique(vm, config->ssh_key_provider.get(), target, mount) - : vm->make_native_mount_handler(target, mount); + ? std::make_unique(vm, config->ssh_key_provider.get(), target, std::move(mount)) + : vm->make_native_mount_handler(target, std::move(mount)); } QFutureWatcher* diff --git a/src/daemon/daemon.h b/src/daemon/daemon.h index 6556052f63..e82ee6bffa 100644 --- a/src/daemon/daemon.h +++ b/src/daemon/daemon.h @@ -173,7 +173,7 @@ public slots: std::unordered_map& vm_mounts, VirtualMachine* vm); - MountHandler::UPtr make_mount(VirtualMachine* vm, const std::string& target, const VMMount& mount); + MountHandler::UPtr make_mount(VirtualMachine* vm, const std::string& target, VMMount mount); struct AsyncOperationStatus { diff --git a/src/utils/vm_mount.cpp b/src/utils/vm_mount.cpp index e95598830b..a293296dc7 100644 --- a/src/utils/vm_mount.cpp +++ b/src/utils/vm_mount.cpp @@ -17,6 +17,8 @@ #include +#include + #include namespace mp = multipass; @@ -111,6 +113,37 @@ mp::VMMount::VMMount(const QJsonObject& json) : VMMount{parse_json(json)} // del { } +void mp::VMMount::resolve_source_path() +{ + std::error_code err; + fs::path path{source_path}; + + auto status = MP_FILEOPS.symlink_status(path, err); + if (err) + throw std::runtime_error( + fmt::format("Mount source path \"{}\" is not accessible: {}.", source_path, err.message())); + + if (status.type() == fs::file_type::symlink) + { + auto symlink_path = MP_FILEOPS.read_symlink(path, err); + + if (err) + throw std::runtime_error( + fmt::format("Mount symlink source path \"{}\" could not be read: {}.", source_path, err.message())); + + if (symlink_path.is_relative()) + symlink_path = path.parent_path() / symlink_path; + + path = fs::weakly_canonical(symlink_path, err); + + if (err) + throw std::runtime_error( + fmt::format("Mount symlink source path \"{}\" could not be made weakly canonical: {}.", source_path, err.message())); + + source_path = path.string(); + } +} + QJsonObject mp::VMMount::serialize() const { QJsonObject ret; From 4bb0fc13e70579f991a99ef802c171291c51d9d8 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 12 Sep 2024 16:17:05 -0400 Subject: [PATCH 2/5] [daemon][unit test] Add mount symlink resolution tests --- src/utils/vm_mount.cpp | 3 --- tests/test_daemon_mount.cpp | 46 +++++++++++++++++++++++++++++++++++++ tests/test_vm_mount.cpp | 46 +++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 3 deletions(-) diff --git a/src/utils/vm_mount.cpp b/src/utils/vm_mount.cpp index a293296dc7..0f71891df2 100644 --- a/src/utils/vm_mount.cpp +++ b/src/utils/vm_mount.cpp @@ -119,9 +119,6 @@ void mp::VMMount::resolve_source_path() fs::path path{source_path}; auto status = MP_FILEOPS.symlink_status(path, err); - if (err) - throw std::runtime_error( - fmt::format("Mount source path \"{}\" is not accessible: {}.", source_path, err.message())); if (status.type() == fs::file_type::symlink) { diff --git a/tests/test_daemon_mount.cpp b/tests/test_daemon_mount.cpp index 75d447b390..134540a5c9 100644 --- a/tests/test_daemon_mount.cpp +++ b/tests/test_daemon_mount.cpp @@ -17,6 +17,7 @@ #include "common.h" #include "daemon_test_fixture.h" +#include "mock_file_ops.h" #include "mock_logger.h" #include "mock_mount_handler.h" #include "mock_platform.h" @@ -267,3 +268,48 @@ TEST_F(TestDaemonMount, performanceMountsNotImplementedHasErrorFails) EXPECT_EQ(status.error_code(), grpc::StatusCode::FAILED_PRECONDITION); EXPECT_THAT(status.error_message(), StrEq("The native mounts feature is not implemented on this backend.")); } + +TEST_F(TestDaemonMount, symlinkSourceGetsResolved) +{ + const auto [temp_dir, filename] = plant_instance_json(fake_json_contents(mac_addr, extra_interfaces)); + config_builder.data_directory = temp_dir->path(); + + const auto [mock_file_ops, _] = mpt::MockFileOps::inject(); + EXPECT_CALL(*mock_file_ops, symlink_status) + .WillOnce(Return(mp::fs::file_status{mp::fs::file_type::symlink})); + EXPECT_CALL(*mock_file_ops, read_symlink) + .WillOnce(Return(mp::fs::path{config_builder.data_directory.toStdString()})); + + + auto original_implementation_of_mkpath = [](const QDir& dir, const QString& dirName) -> bool { + return MP_FILEOPS.FileOps::mkpath(dir, dirName); + }; + EXPECT_CALL(*mock_file_ops, mkpath) + .WillRepeatedly(original_implementation_of_mkpath); + + auto original_implementation_of_open = [](QFileDevice& dev, QIODevice::OpenMode mode) -> bool { + return MP_FILEOPS.FileOps::open(dev, mode); + }; + EXPECT_CALL(*mock_file_ops, open(A(), A())) + .WillRepeatedly(original_implementation_of_open); + + auto original_implementation_of_commit = [](QSaveFile& file) -> bool { + return MP_FILEOPS.FileOps::commit(file); + }; + EXPECT_CALL(*mock_file_ops, commit) + .WillRepeatedly(original_implementation_of_commit); + + mp::Daemon daemon{config_builder.build()}; + + mp::MountRequest request; + request.set_source_path(mount_dir.path().toStdString()); + request.set_mount_type(mp::MountRequest::NATIVE); + auto entry = request.add_target_paths(); + entry->set_instance_name(mock_instance_name); + entry->set_target_path(fake_target_path); + + auto status = call_daemon_slot(daemon, &mp::Daemon::mount, request, + StrictMock>{}); + + EXPECT_TRUE(status.ok()); +} diff --git a/tests/test_vm_mount.cpp b/tests/test_vm_mount.cpp index 5f0a61d9b2..8945e2909a 100644 --- a/tests/test_vm_mount.cpp +++ b/tests/test_vm_mount.cpp @@ -16,6 +16,7 @@ */ #include "common.h" +#include "mock_file_ops.h" #include @@ -139,4 +140,49 @@ TEST_F(TestVMMount, duplicateGidsThrowsWithDuplicateTargetID) HasSubstr("1002:1001"), HasSubstr("1000:1001")))); } + +TEST_F(TestVMMount, notSymlinkSourcePathUnchanged) +{ + const auto [mock_file_ops, _] = mpt::MockFileOps::inject(); + EXPECT_CALL(*mock_file_ops, symlink_status) + .WillOnce(Return(mp::fs::file_status{mp::fs::file_type::regular})); + EXPECT_CALL(*mock_file_ops, read_symlink).Times(0); + + mp::VMMount mount{"src", {}, {}, mp::VMMount::MountType::Classic}; + + mount.resolve_source_path(); + + EXPECT_EQ(mount.get_source_path(), "src"); +} + +TEST_F(TestVMMount, absoluteSymlinkSourcePathResolved) +{ + const auto [mock_file_ops, _] = mpt::MockFileOps::inject(); + EXPECT_CALL(*mock_file_ops, symlink_status) + .WillOnce(Return(mp::fs::file_status{mp::fs::file_type::symlink})); + EXPECT_CALL(*mock_file_ops, read_symlink) + .WillOnce(Return(mp::fs::path{"/home/dest"})); + + mp::VMMount mount{"/tmp/src", {}, {}, mp::VMMount::MountType::Classic}; + + mount.resolve_source_path(); + + EXPECT_EQ(mount.get_source_path(), "/home/dest"); +} + +TEST_F(TestVMMount, relativeSymlinkSourcePathResolved) +{ + const auto [mock_file_ops, _] = mpt::MockFileOps::inject(); + EXPECT_CALL(*mock_file_ops, symlink_status) + .WillOnce(Return(mp::fs::file_status{mp::fs::file_type::symlink})); + EXPECT_CALL(*mock_file_ops, read_symlink) + .WillOnce(Return(mp::fs::path{"./dest"})); + + mp::VMMount mount{"/tmp/src", {}, {}, mp::VMMount::MountType::Classic}; + + mount.resolve_source_path(); + + EXPECT_EQ(mount.get_source_path(), "/tmp/dest"); +} + } // namespace From e35205659755dab70e94c6ef65c3fd0ec3185c57 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 12 Sep 2024 17:00:30 -0400 Subject: [PATCH 3/5] Format changes --- src/utils/vm_mount.cpp | 8 +++++--- tests/test_daemon_mount.cpp | 18 +++++++----------- tests/test_vm_mount.cpp | 15 +++++---------- 3 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/utils/vm_mount.cpp b/src/utils/vm_mount.cpp index 0f71891df2..ad03d20245 100644 --- a/src/utils/vm_mount.cpp +++ b/src/utils/vm_mount.cpp @@ -129,14 +129,16 @@ void mp::VMMount::resolve_source_path() fmt::format("Mount symlink source path \"{}\" could not be read: {}.", source_path, err.message())); if (symlink_path.is_relative()) - symlink_path = path.parent_path() / symlink_path; + symlink_path = path.parent_path() / symlink_path; path = fs::weakly_canonical(symlink_path, err); if (err) throw std::runtime_error( - fmt::format("Mount symlink source path \"{}\" could not be made weakly canonical: {}.", source_path, err.message())); - + fmt::format("Mount symlink source path \"{}\" could not be made weakly canonical: {}.", + source_path, + err.message())); + source_path = path.string(); } } diff --git a/tests/test_daemon_mount.cpp b/tests/test_daemon_mount.cpp index 134540a5c9..85a8d5f75c 100644 --- a/tests/test_daemon_mount.cpp +++ b/tests/test_daemon_mount.cpp @@ -275,17 +275,14 @@ TEST_F(TestDaemonMount, symlinkSourceGetsResolved) config_builder.data_directory = temp_dir->path(); const auto [mock_file_ops, _] = mpt::MockFileOps::inject(); - EXPECT_CALL(*mock_file_ops, symlink_status) - .WillOnce(Return(mp::fs::file_status{mp::fs::file_type::symlink})); + EXPECT_CALL(*mock_file_ops, symlink_status).WillOnce(Return(mp::fs::file_status{mp::fs::file_type::symlink})); EXPECT_CALL(*mock_file_ops, read_symlink) .WillOnce(Return(mp::fs::path{config_builder.data_directory.toStdString()})); - auto original_implementation_of_mkpath = [](const QDir& dir, const QString& dirName) -> bool { return MP_FILEOPS.FileOps::mkpath(dir, dirName); }; - EXPECT_CALL(*mock_file_ops, mkpath) - .WillRepeatedly(original_implementation_of_mkpath); + EXPECT_CALL(*mock_file_ops, mkpath).WillRepeatedly(original_implementation_of_mkpath); auto original_implementation_of_open = [](QFileDevice& dev, QIODevice::OpenMode mode) -> bool { return MP_FILEOPS.FileOps::open(dev, mode); @@ -293,11 +290,8 @@ TEST_F(TestDaemonMount, symlinkSourceGetsResolved) EXPECT_CALL(*mock_file_ops, open(A(), A())) .WillRepeatedly(original_implementation_of_open); - auto original_implementation_of_commit = [](QSaveFile& file) -> bool { - return MP_FILEOPS.FileOps::commit(file); - }; - EXPECT_CALL(*mock_file_ops, commit) - .WillRepeatedly(original_implementation_of_commit); + auto original_implementation_of_commit = [](QSaveFile& file) -> bool { return MP_FILEOPS.FileOps::commit(file); }; + EXPECT_CALL(*mock_file_ops, commit).WillRepeatedly(original_implementation_of_commit); mp::Daemon daemon{config_builder.build()}; @@ -308,7 +302,9 @@ TEST_F(TestDaemonMount, symlinkSourceGetsResolved) entry->set_instance_name(mock_instance_name); entry->set_target_path(fake_target_path); - auto status = call_daemon_slot(daemon, &mp::Daemon::mount, request, + auto status = call_daemon_slot(daemon, + &mp::Daemon::mount, + request, StrictMock>{}); EXPECT_TRUE(status.ok()); diff --git a/tests/test_vm_mount.cpp b/tests/test_vm_mount.cpp index 8945e2909a..d21b4b2cff 100644 --- a/tests/test_vm_mount.cpp +++ b/tests/test_vm_mount.cpp @@ -144,8 +144,7 @@ TEST_F(TestVMMount, duplicateGidsThrowsWithDuplicateTargetID) TEST_F(TestVMMount, notSymlinkSourcePathUnchanged) { const auto [mock_file_ops, _] = mpt::MockFileOps::inject(); - EXPECT_CALL(*mock_file_ops, symlink_status) - .WillOnce(Return(mp::fs::file_status{mp::fs::file_type::regular})); + EXPECT_CALL(*mock_file_ops, symlink_status).WillOnce(Return(mp::fs::file_status{mp::fs::file_type::regular})); EXPECT_CALL(*mock_file_ops, read_symlink).Times(0); mp::VMMount mount{"src", {}, {}, mp::VMMount::MountType::Classic}; @@ -158,10 +157,8 @@ TEST_F(TestVMMount, notSymlinkSourcePathUnchanged) TEST_F(TestVMMount, absoluteSymlinkSourcePathResolved) { const auto [mock_file_ops, _] = mpt::MockFileOps::inject(); - EXPECT_CALL(*mock_file_ops, symlink_status) - .WillOnce(Return(mp::fs::file_status{mp::fs::file_type::symlink})); - EXPECT_CALL(*mock_file_ops, read_symlink) - .WillOnce(Return(mp::fs::path{"/home/dest"})); + EXPECT_CALL(*mock_file_ops, symlink_status).WillOnce(Return(mp::fs::file_status{mp::fs::file_type::symlink})); + EXPECT_CALL(*mock_file_ops, read_symlink).WillOnce(Return(mp::fs::path{"/home/dest"})); mp::VMMount mount{"/tmp/src", {}, {}, mp::VMMount::MountType::Classic}; @@ -173,10 +170,8 @@ TEST_F(TestVMMount, absoluteSymlinkSourcePathResolved) TEST_F(TestVMMount, relativeSymlinkSourcePathResolved) { const auto [mock_file_ops, _] = mpt::MockFileOps::inject(); - EXPECT_CALL(*mock_file_ops, symlink_status) - .WillOnce(Return(mp::fs::file_status{mp::fs::file_type::symlink})); - EXPECT_CALL(*mock_file_ops, read_symlink) - .WillOnce(Return(mp::fs::path{"./dest"})); + EXPECT_CALL(*mock_file_ops, symlink_status).WillOnce(Return(mp::fs::file_status{mp::fs::file_type::symlink})); + EXPECT_CALL(*mock_file_ops, read_symlink).WillOnce(Return(mp::fs::path{"./dest"})); mp::VMMount mount{"/tmp/src", {}, {}, mp::VMMount::MountType::Classic}; From 607afba8332b95e0343be769fdb5e2e0ee2bccb9 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Fri, 13 Sep 2024 21:20:20 -0400 Subject: [PATCH 4/5] [daemon][unit test] Fix tests expected path on Windows and MacOS --- tests/test_vm_mount.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/test_vm_mount.cpp b/tests/test_vm_mount.cpp index d21b4b2cff..fefaec7ed0 100644 --- a/tests/test_vm_mount.cpp +++ b/tests/test_vm_mount.cpp @@ -156,28 +156,33 @@ TEST_F(TestVMMount, notSymlinkSourcePathUnchanged) TEST_F(TestVMMount, absoluteSymlinkSourcePathResolved) { + auto source_path = mp::fs::weakly_canonical("/tmp/src"); + auto symlink_path = mp::fs::weakly_canonical("/home/dest"); + const auto [mock_file_ops, _] = mpt::MockFileOps::inject(); EXPECT_CALL(*mock_file_ops, symlink_status).WillOnce(Return(mp::fs::file_status{mp::fs::file_type::symlink})); - EXPECT_CALL(*mock_file_ops, read_symlink).WillOnce(Return(mp::fs::path{"/home/dest"})); + EXPECT_CALL(*mock_file_ops, read_symlink).WillOnce(Return(symlink_path)); - mp::VMMount mount{"/tmp/src", {}, {}, mp::VMMount::MountType::Classic}; + mp::VMMount mount{source_path.string(), {}, {}, mp::VMMount::MountType::Classic}; mount.resolve_source_path(); - EXPECT_EQ(mount.get_source_path(), "/home/dest"); + EXPECT_EQ(mount.get_source_path(), symlink_path.string()); } TEST_F(TestVMMount, relativeSymlinkSourcePathResolved) { + auto source_path = mp::fs::weakly_canonical("/tmp/src"); + const auto [mock_file_ops, _] = mpt::MockFileOps::inject(); EXPECT_CALL(*mock_file_ops, symlink_status).WillOnce(Return(mp::fs::file_status{mp::fs::file_type::symlink})); EXPECT_CALL(*mock_file_ops, read_symlink).WillOnce(Return(mp::fs::path{"./dest"})); - mp::VMMount mount{"/tmp/src", {}, {}, mp::VMMount::MountType::Classic}; + mp::VMMount mount{source_path.string(), {}, {}, mp::VMMount::MountType::Classic}; mount.resolve_source_path(); - EXPECT_EQ(mount.get_source_path(), "/tmp/dest"); + EXPECT_EQ(mount.get_source_path(), source_path.replace_filename("dest").string()); } } // namespace From 06347b015bf1f64bd8cbcadd03b59fea66676cad Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Mon, 16 Sep 2024 12:25:26 -0400 Subject: [PATCH 5/5] [daemon] Move symlink resolution to VMMount ctor --- include/multipass/vm_mount.h | 4 ++-- src/daemon/daemon.cpp | 8 +++----- src/daemon/daemon.h | 2 +- src/utils/vm_mount.cpp | 2 ++ tests/test_vm_mount.cpp | 6 ------ 5 files changed, 8 insertions(+), 14 deletions(-) diff --git a/include/multipass/vm_mount.h b/include/multipass/vm_mount.h index 25fedf8877..1572948f1c 100644 --- a/include/multipass/vm_mount.h +++ b/include/multipass/vm_mount.h @@ -39,8 +39,6 @@ class VMMount explicit VMMount(const QJsonObject& json); VMMount(const std::string& sourcePath, id_mappings gidMappings, id_mappings uidMappings, MountType mountType); - void resolve_source_path(); - QJsonObject serialize() const; const std::string& get_source_path() const noexcept; @@ -52,6 +50,8 @@ class VMMount friend bool operator!=(const VMMount& a, const VMMount& b) noexcept; private: + void resolve_source_path(); + std::string source_path; id_mappings gid_mappings; id_mappings uid_mappings; diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index 957a6dd0bf..a38552498e 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -3270,13 +3270,11 @@ bool mp::Daemon::create_missing_mounts(std::unordered_map& return mount_specs.size() != initial_mount_count; } -mp::MountHandler::UPtr mp::Daemon::make_mount(VirtualMachine* vm, const std::string& target, VMMount mount) +mp::MountHandler::UPtr mp::Daemon::make_mount(VirtualMachine* vm, const std::string& target, const VMMount& mount) { - mount.resolve_source_path(); - return mount.get_mount_type() == VMMount::MountType::Classic - ? std::make_unique(vm, config->ssh_key_provider.get(), target, std::move(mount)) - : vm->make_native_mount_handler(target, std::move(mount)); + ? std::make_unique(vm, config->ssh_key_provider.get(), target, mount) + : vm->make_native_mount_handler(target, mount); } QFutureWatcher* diff --git a/src/daemon/daemon.h b/src/daemon/daemon.h index e82ee6bffa..6556052f63 100644 --- a/src/daemon/daemon.h +++ b/src/daemon/daemon.h @@ -173,7 +173,7 @@ public slots: std::unordered_map& vm_mounts, VirtualMachine* vm); - MountHandler::UPtr make_mount(VirtualMachine* vm, const std::string& target, VMMount mount); + MountHandler::UPtr make_mount(VirtualMachine* vm, const std::string& target, const VMMount& mount); struct AsyncOperationStatus { diff --git a/src/utils/vm_mount.cpp b/src/utils/vm_mount.cpp index ad03d20245..5cffd80f05 100644 --- a/src/utils/vm_mount.cpp +++ b/src/utils/vm_mount.cpp @@ -107,6 +107,8 @@ mp::VMMount::VMMount(const std::string& sourcePath, if (errors.size()) throw std::runtime_error( fmt::format("Mount cannot apply mapping with duplicate ids:{}", fmt::to_string(errors))); + + resolve_source_path(); } mp::VMMount::VMMount(const QJsonObject& json) : VMMount{parse_json(json)} // delegate on copy ctor diff --git a/tests/test_vm_mount.cpp b/tests/test_vm_mount.cpp index fefaec7ed0..e33465fea0 100644 --- a/tests/test_vm_mount.cpp +++ b/tests/test_vm_mount.cpp @@ -149,8 +149,6 @@ TEST_F(TestVMMount, notSymlinkSourcePathUnchanged) mp::VMMount mount{"src", {}, {}, mp::VMMount::MountType::Classic}; - mount.resolve_source_path(); - EXPECT_EQ(mount.get_source_path(), "src"); } @@ -165,8 +163,6 @@ TEST_F(TestVMMount, absoluteSymlinkSourcePathResolved) mp::VMMount mount{source_path.string(), {}, {}, mp::VMMount::MountType::Classic}; - mount.resolve_source_path(); - EXPECT_EQ(mount.get_source_path(), symlink_path.string()); } @@ -180,8 +176,6 @@ TEST_F(TestVMMount, relativeSymlinkSourcePathResolved) mp::VMMount mount{source_path.string(), {}, {}, mp::VMMount::MountType::Classic}; - mount.resolve_source_path(); - EXPECT_EQ(mount.get_source_path(), source_path.replace_filename("dest").string()); }