From e5ea0012def5632c83c29e137c028127908805fa Mon Sep 17 00:00:00 2001 From: ZayanKhan-12 <108294002+ZayanKhan-12@users.noreply.github.com> Date: Sat, 25 Jul 2026 05:52:02 -0400 Subject: [PATCH] Protect erase_resource and set_resource_health against cyclic sub-resources A resource that is directly or indirectly listed as a sub-resource of itself caused unbounded recursion and a stack overflow, both in erase_resource (as reported in #403) and in set_resource_health, which is reached via insert_resource when the cyclic relationship already exists in the registry. Track the ids already visited during the recursive descent and skip any id seen before, so cycles of any length are handled, per the discussion in #403. Event order and results for well-formed sub-resource trees are unchanged. Closes #403 Co-Authored-By: Claude Fable 5 --- Development/cmake/NmosCppTest.cmake | 1 + Development/nmos/resources.cpp | 109 ++++++++++++++--------- Development/nmos/test/resources_test.cpp | 78 ++++++++++++++++ 3 files changed, 148 insertions(+), 40 deletions(-) create mode 100644 Development/nmos/test/resources_test.cpp diff --git a/Development/cmake/NmosCppTest.cmake b/Development/cmake/NmosCppTest.cmake index 4a589c51f..1ad6fdaec 100644 --- a/Development/cmake/NmosCppTest.cmake +++ b/Development/cmake/NmosCppTest.cmake @@ -60,6 +60,7 @@ set(NMOS_CPP_TEST_NMOS_TEST_SOURCES nmos/test/node_interfaces_test.cpp nmos/test/paging_utils_test.cpp nmos/test/query_api_test.cpp + nmos/test/resources_test.cpp nmos/test/sdp_test_utils.cpp nmos/test/sdp_utils_test.cpp nmos/test/settings_test.cpp diff --git a/Development/nmos/resources.cpp b/Development/nmos/resources.cpp index a4c0b2144..7a23dffdb 100644 --- a/Development/nmos/resources.cpp +++ b/Development/nmos/resources.cpp @@ -131,47 +131,62 @@ namespace nmos return result; } - // erase the resource with the specified id from the specified resources (if present) - // and return the count of the number of resources erased (including sub-resources) - // resources may optionally be initially "erased" by setting data to null, and remain in this non-extant state until they are explicitly forgotten (or reinserted) - resources::size_type erase_resource(resources& resources, const id& id, bool forget_now) + namespace details { - // also erase all sub-resources of this resource, i.e. - // for a node, all devices with matching node_id - // for a device, all sources, senders and receivers with matching device_id - // for a sender, all flows with matching source_id - // it won't be a very deep recursion... - resources::size_type count = 0; - auto found = resources.find(id); - if (resources.end() != found && found->has_data()) + // as nmos::erase_resource, but with protection against cyclic sub-resource relationships + static resources::size_type erase_resource(nmos::resources& resources, const nmos::id& id, bool forget_now, std::set& erasing) { - for (auto& sub_resource : found->sub_resources) + // also erase all sub-resources of this resource, i.e. + // for a node, all devices with matching node_id + // for a device, all sources, senders and receivers with matching device_id + // for a sender, all flows with matching source_id + // it won't be a very deep recursion... + resources::size_type count = 0; + + // a resource that is directly or indirectly listed as a sub-resource of itself + // would otherwise cause unbounded recursion (see nmos-cpp issue #403) + if (!erasing.insert(id).second) return count; + + auto found = resources.find(id); + if (resources.end() != found && found->has_data()) { - count += erase_resource(resources, sub_resource, forget_now); - } + for (auto& sub_resource : found->sub_resources) + { + count += details::erase_resource(resources, sub_resource, forget_now, erasing); + } - const auto pre = found->data; + const auto pre = found->data; - auto resource_updated = nmos::strictly_increasing_update(resources); - resources.modify(found, [&resource_updated](resource& resource) - { - resource.data = web::json::value::null(); + auto resource_updated = nmos::strictly_increasing_update(resources); + resources.modify(found, [&resource_updated](resource& resource) + { + resource.data = web::json::value::null(); - // set the update timestamp when a resource is deleted - resource.updated = resource_updated; - }); + // set the update timestamp when a resource is deleted + resource.updated = resource_updated; + }); - auto& erased = *found; - insert_resource_events(resources, erased.version, erased.downgrade_version, erased.type, pre, erased.data); + auto& erased = *found; + insert_resource_events(resources, erased.version, erased.downgrade_version, erased.type, pre, erased.data); - if (forget_now) - { - resources.erase(found); - } + if (forget_now) + { + resources.erase(found); + } - ++count; + ++count; + } + return count; } - return count; + } + + // erase the resource with the specified id from the specified resources (if present) + // and return the count of the number of resources erased (including sub-resources) + // resources may optionally be initially "erased" by setting data to null, and remain in this non-extant state until they are explicitly forgotten (or reinserted) + resources::size_type erase_resource(resources& resources, const id& id, bool forget_now) + { + std::set erasing; + return details::erase_resource(resources, id, forget_now, erasing); } // forget all erased resources which expired *before* the specified time from the specified resources @@ -266,22 +281,36 @@ namespace nmos // find the resource with the specified id in the specified resources (if present) and // set the health of the resource and all of its sub-resources, to prevent them expiring // note, since health is mutable, no need for the resources parameter to be non-const - void set_resource_health(const resources& resources, const id& id, health health) + namespace details { - auto found = resources.find(id); - if (resources.end() != found && found->has_data()) + // as nmos::set_resource_health, but with protection against cyclic sub-resource relationships + static void set_resource_health(const nmos::resources& resources, const nmos::id& id, health health, std::set& setting) { - for (auto& sub_resource : found->sub_resources) + // a resource that is directly or indirectly listed as a sub-resource of itself + // would otherwise cause unbounded recursion (see nmos-cpp issue #403) + if (!setting.insert(id).second) return; + + auto found = resources.find(id); + if (resources.end() != found && found->has_data()) { - set_resource_health(resources, sub_resource, health); - } + for (auto& sub_resource : found->sub_resources) + { + details::set_resource_health(resources, sub_resource, health, setting); + } - // since health is mutable, no need for: - // resources.modify(found, [&health](nmos::resource& resource){ resource.health = health; }); - found->health = health; + // since health is mutable, no need for: + // resources.modify(found, [&health](nmos::resource& resource){ resource.health = health; }); + found->health = health; + } } } + void set_resource_health(const resources& resources, const id& id, health health) + { + std::set setting; + details::set_resource_health(resources, id, health, setting); + } + static inline std::pair no_resource() { return{}; } // get the super-resource id and type, according to the guidelines on referential integrity diff --git a/Development/nmos/test/resources_test.cpp b/Development/nmos/test/resources_test.cpp new file mode 100644 index 000000000..fa2b741d8 --- /dev/null +++ b/Development/nmos/test/resources_test.cpp @@ -0,0 +1,78 @@ +// The first "test" is of course whether the header compiles standalone +#include "nmos/resources.h" + +#include "bst/test/test.h" +#include "nmos/is04_versions.h" + +namespace +{ + nmos::resource make_test_node(const nmos::id& id) + { + using web::json::value_of; + + auto data = value_of({ + { U("id"), id } + }); + return{ nmos::is04_versions::v1_3, nmos::types::node, std::move(data), id, false }; + } + + nmos::resource make_test_device(const nmos::id& id, const nmos::id& node_id) + { + using web::json::value_of; + + auto data = value_of({ + { U("id"), id }, + { U("node_id"), node_id } + }); + return{ nmos::is04_versions::v1_3, nmos::types::device, std::move(data), id, false }; + } +} + +//////////////////////////////////////////////////////////////////////////////////////////// +BST_TEST_CASE(testEraseResourceWithCyclicSubResources) +{ + // a resource listed as a sub-resource of itself must not cause unbounded recursion + // see https://github.com/sony/nmos-cpp/issues/403 + { + nmos::resources resources; + auto self = make_test_node(U("self")); + self.sub_resources.insert(U("self")); + nmos::insert_resource(resources, std::move(self)); + + BST_REQUIRE_EQUAL(1u, nmos::erase_resource(resources, U("self"))); + BST_REQUIRE(resources.empty()); + } + + // resources indirectly listed as sub-resources of themselves must not either + { + nmos::resources resources; + auto a = make_test_node(U("a")); + a.sub_resources.insert(U("b")); + auto b = make_test_device(U("b"), U("a")); + b.sub_resources.insert(U("a")); + nmos::insert_resource(resources, std::move(a)); + nmos::insert_resource(resources, std::move(b)); + + BST_REQUIRE_EQUAL(2u, nmos::erase_resource(resources, U("a"))); + BST_REQUIRE(resources.empty()); + } +} + +//////////////////////////////////////////////////////////////////////////////////////////// +BST_TEST_CASE(testEraseResourceWithSubResources) +{ + // erasing a resource also erases its sub-resources, and only those + nmos::resources resources; + auto node = make_test_node(U("node")); + node.sub_resources.insert(U("device")); + auto device = make_test_device(U("device"), U("node")); + auto other = make_test_node(U("other")); + nmos::insert_resource(resources, std::move(node)); + nmos::insert_resource(resources, std::move(device)); + nmos::insert_resource(resources, std::move(other)); + + BST_REQUIRE_EQUAL(2u, nmos::erase_resource(resources, U("node"))); + BST_REQUIRE(resources.end() == resources.find(U("node"))); + BST_REQUIRE(resources.end() == resources.find(U("device"))); + BST_REQUIRE(resources.end() != resources.find(U("other"))); +}