This is an automated email from the ASF dual-hosted git repository. bmahler pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 682f6a743acb9cde4063e8e2d8d200c8531a21b3 Author: Benjamin Mahler <[email protected]> AuthorDate: Wed Jul 24 00:33:55 2024 -0400 [cgroups2] Fix unsafe Process usage in DeviceManager. Calls to the DeviceManager wrapper were directly accessing the state of DeviceManagerProcess. This patch uses the dispatch mechanism instead, and adjusts the tests accordingly. --- .../device_manager/device_manager.cpp | 17 ++++++-- .../device_manager/device_manager.hpp | 4 +- src/tests/device_manager_tests.cpp | 47 +++++++++++++--------- 3 files changed, 44 insertions(+), 24 deletions(-) diff --git a/src/slave/containerizer/device_manager/device_manager.cpp b/src/slave/containerizer/device_manager/device_manager.cpp index d28b2c35b..4c9b86393 100644 --- a/src/slave/containerizer/device_manager/device_manager.cpp +++ b/src/slave/containerizer/device_manager/device_manager.cpp @@ -224,16 +224,25 @@ Future<Nothing> DeviceManager::configure( } -hashmap<string, DeviceManager::CgroupDeviceAccess> DeviceManager::state() const +Future<hashmap<string, DeviceManager::CgroupDeviceAccess>> + DeviceManager::state() const { - return process->state(); + // Necessary due to overloading of state(). + auto process_copy = process; + return dispatch(*process, [process_copy]() { + return process_copy->state(); + }); } -DeviceManager::CgroupDeviceAccess DeviceManager::state( +Future<DeviceManager::CgroupDeviceAccess> DeviceManager::state( const string& cgroup) const { - return process->state(cgroup); + // Necessary due to overloading of state(). + auto process_copy = process; + return dispatch(*process, [process_copy, cgroup]() { + return process_copy->state(cgroup); + }); } diff --git a/src/slave/containerizer/device_manager/device_manager.hpp b/src/slave/containerizer/device_manager/device_manager.hpp index eafa4e6f6..7c8523d8b 100644 --- a/src/slave/containerizer/device_manager/device_manager.hpp +++ b/src/slave/containerizer/device_manager/device_manager.hpp @@ -97,10 +97,10 @@ public: // Return the cgroup's device access state, which can be // used to query if a device access would be granted. - CgroupDeviceAccess state(const std::string& cgroup) const; + process::Future<CgroupDeviceAccess> state(const std::string& cgroup) const; // Return the cgroup's device access state for all cgroups tracked. - hashmap<std::string, CgroupDeviceAccess> state() const; + process::Future<hashmap<std::string, CgroupDeviceAccess>> state() const; // Returns cgroup device state with additions and removals applied to it. // Exposed for unit testing. diff --git a/src/tests/device_manager_tests.cpp b/src/tests/device_manager_tests.cpp index 19ca0af4c..54d464e97 100644 --- a/src/tests/device_manager_tests.cpp +++ b/src/tests/device_manager_tests.cpp @@ -126,10 +126,12 @@ TEST_F(DeviceManagerTest, ROOT_DeviceManagerConfigure_Normal) allow_list, CHECK_NOTERROR(convert_to_non_wildcards(deny_list)))); - DeviceManager::CgroupDeviceAccess cgroup_state = dm->state(TEST_CGROUP); + Future<DeviceManager::CgroupDeviceAccess> cgroup_state = + dm->state(TEST_CGROUP); - EXPECT_EQ(cgroup_state.allow_list, allow_list); - EXPECT_EQ(cgroup_state.deny_list, deny_list); + AWAIT_ASSERT_READY(cgroup_state); + EXPECT_EQ(allow_list, cgroup_state->allow_list); + EXPECT_EQ(deny_list, cgroup_state->deny_list); pid_t pid = ::fork(); ASSERT_NE(-1, pid); @@ -171,10 +173,13 @@ TEST_F(DeviceManagerTest, ROOT_DeviceManagerReconfigure_Normal) TEST_CGROUP, allow_list, CHECK_NOTERROR(convert_to_non_wildcards(deny_list)))); - DeviceManager::CgroupDeviceAccess cgroup_state = dm->state(TEST_CGROUP); - EXPECT_EQ(cgroup_state.allow_list, allow_list); - EXPECT_EQ(cgroup_state.deny_list, deny_list); + Future<DeviceManager::CgroupDeviceAccess> cgroup_state = + dm->state(TEST_CGROUP); + + AWAIT_ASSERT_READY(cgroup_state); + EXPECT_EQ(allow_list, cgroup_state->allow_list); + EXPECT_EQ(deny_list, cgroup_state->deny_list); vector<devices::Entry> additions = {*devices::Entry::parse("c 1:3 r")}; vector<devices::Entry> removals = allow_list; @@ -183,10 +188,12 @@ TEST_F(DeviceManagerTest, ROOT_DeviceManagerReconfigure_Normal) TEST_CGROUP, CHECK_NOTERROR(convert_to_non_wildcards(additions)), CHECK_NOTERROR(convert_to_non_wildcards(removals)))); + cgroup_state = dm->state(TEST_CGROUP); - EXPECT_EQ(cgroup_state.allow_list, additions); - EXPECT_EQ(cgroup_state.deny_list, deny_list); + AWAIT_ASSERT_READY(cgroup_state); + EXPECT_EQ(additions, cgroup_state->allow_list); + EXPECT_EQ(deny_list, cgroup_state->deny_list); pid_t pid = ::fork(); ASSERT_NE(-1, pid); @@ -250,10 +257,12 @@ TEST_F(DeviceManagerTest, ROOT_DeviceManagerConfigure_AllowWildcard) allow_list, CHECK_NOTERROR(convert_to_non_wildcards(deny_list)))); - DeviceManager::CgroupDeviceAccess dm_state = dm->state(TEST_CGROUP); + Future<DeviceManager::CgroupDeviceAccess> cgroup_state = + dm->state(TEST_CGROUP); - EXPECT_EQ(dm_state.allow_list, allow_list); - EXPECT_EQ(dm_state.deny_list, deny_list); + AWAIT_ASSERT_READY(cgroup_state); + EXPECT_EQ(allow_list, cgroup_state->allow_list); + EXPECT_EQ(deny_list, cgroup_state->deny_list); } @@ -315,18 +324,20 @@ TEST_P(DeviceManagerGetDiffStateTestFixture, ROOT_DeviceManagerGetDiffState) setup_allow, CHECK_NOTERROR(convert_to_non_wildcards(setup_deny)))); - DeviceManager::CgroupDeviceAccess dm_state = dm->state(TEST_CGROUP); + Future<DeviceManager::CgroupDeviceAccess> cgroup_state = + dm->state(TEST_CGROUP); - EXPECT_EQ(dm_state.allow_list, setup_allow); - EXPECT_EQ(dm_state.deny_list, setup_deny); + AWAIT_ASSERT_READY(cgroup_state); + EXPECT_EQ(setup_allow, cgroup_state->allow_list); + EXPECT_EQ(setup_deny, cgroup_state->deny_list); - dm_state = dm->apply_diff( - dm->state(TEST_CGROUP), + cgroup_state = dm->apply_diff( + cgroup_state.get(), CHECK_NOTERROR(convert_to_non_wildcards(additions)), CHECK_NOTERROR(convert_to_non_wildcards(removals))); - EXPECT_EQ(dm_state.allow_list, reconfigured_allow); - EXPECT_EQ(dm_state.deny_list, reconfigured_deny); + EXPECT_EQ(reconfigured_allow, cgroup_state->allow_list); + EXPECT_EQ(reconfigured_deny, cgroup_state->deny_list); }
