diff --git a/Development/cmake/NmosCppTest.cmake b/Development/cmake/NmosCppTest.cmake index 4979a894..083f680b 100644 --- a/Development/cmake/NmosCppTest.cmake +++ b/Development/cmake/NmosCppTest.cmake @@ -61,6 +61,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_temporal_redundancy_test.cpp nmos/test/sdp_utils_test.cpp diff --git a/Development/nmos/resources.cpp b/Development/nmos/resources.cpp index a4c0b214..830f5d8a 100644 --- a/Development/nmos/resources.cpp +++ b/Development/nmos/resources.cpp @@ -50,15 +50,13 @@ namespace nmos // all types (other than nodes, and subscriptions) must* be a sub-resource of an existing resource // (*assuming not out-of-order insertion by the allow_invalid_resources setting) - auto super_resource = find_resource(resources, get_super_resource(resource)); - if (super_resource != resources.end()) - { - // this isn't modifying the visible data of the super_resouce, so no resource events need to be generated - resources.modify(super_resource, [&](nmos::resource& super_resource) - { - super_resource.sub_resources.insert(resource.id); - }); - } + // evaluate the super-resource id/type before insertion (get_super_resource may throw if + // referential-integrity fields are missing), but only update sub_resources after a successful + // insert or replace — otherwise a rejected insert whose id matches an existing resource + // (e.g. a device reusing a node id) would leave a self-reference and later recurse unboundedly + // in set_resource_health / erase_resource + // see https://github.com/sony/nmos-cpp/issues/403 + const auto super_id_type = get_super_resource(resource); auto result = resources.insert(std::move(resource)); // replacement of a deleted or expired resource is also allowed @@ -72,6 +70,17 @@ namespace nmos if (result.second) { auto& inserted = *result.first; + + auto super_resource = find_resource(resources, super_id_type); + if (super_resource != resources.end()) + { + // this isn't modifying the visible data of the super_resouce, so no resource events need to be generated + resources.modify(super_resource, [&](nmos::resource& super_resource) + { + super_resource.sub_resources.insert(inserted.id); + }); + } + insert_resource_events(resources, inserted.version, inserted.downgrade_version, inserted.type, web::json::value::null(), inserted.data); // set the initial health of this resource from the super-resource (if applicable) diff --git a/Development/nmos/test/resources_test.cpp b/Development/nmos/test/resources_test.cpp new file mode 100644 index 00000000..6bbcfe32 --- /dev/null +++ b/Development/nmos/test/resources_test.cpp @@ -0,0 +1,74 @@ +// 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; + + return{ nmos::is04_versions::v1_3, nmos::types::node, value_of({ + { U("id"), id } + }), false }; + } + + nmos::resource make_test_device(const nmos::id& id, const nmos::id& node_id) + { + using web::json::value_of; + + return{ nmos::is04_versions::v1_3, nmos::types::device, value_of({ + { U("id"), id }, + { U("node_id"), node_id } + }), false }; + } +} + +//////////////////////////////////////////////////////////////////////////////////////////// +// A resource id is unique in the model, so a device that reuses an existing node id cannot +// be inserted. Previously, insert_resource still recorded that id in the node's sub_resources +// before the insert was rejected, leaving a self-reference that made set_resource_health / +// erase_resource recurse without bound. +// See https://github.com/sony/nmos-cpp/issues/403 +BST_TEST_CASE(testInsertResourceRejectsDuplicateIdWithoutCorruptingSubResources) +{ + const nmos::id id{ U("11111111-1111-1111-1111-111111111111") }; + + nmos::resources resources; + BST_REQUIRE(nmos::insert_resource(resources, make_test_node(id)).second); + + // same id as the node, and node_id also equal to that id (the mistake from issue #403) + BST_REQUIRE(!nmos::insert_resource(resources, make_test_device(id, id)).second); + + BST_REQUIRE_EQUAL(1u, resources.size()); + const auto node = nmos::find_resource(resources, { id, nmos::types::node }); + BST_REQUIRE(resources.end() != node); + BST_REQUIRE(node->sub_resources.empty()); + + // would previously stack-overflow / crash + nmos::set_resource_health(resources, id); + BST_REQUIRE_EQUAL(1u, nmos::erase_resource(resources, id)); +} + +//////////////////////////////////////////////////////////////////////////////////////////// +BST_TEST_CASE(testInsertResourceRecordsSubResources) +{ + const nmos::id node_id{ U("22222222-2222-2222-2222-222222222222") }; + const nmos::id device_id{ U("33333333-3333-3333-3333-333333333333") }; + + nmos::resources resources; + BST_REQUIRE(nmos::insert_resource(resources, make_test_node(node_id)).second); + BST_REQUIRE(nmos::insert_resource(resources, make_test_device(device_id, node_id)).second); + + const auto node = nmos::find_resource(resources, { node_id, nmos::types::node }); + BST_REQUIRE(resources.end() != node); + BST_REQUIRE_EQUAL(1u, node->sub_resources.count(device_id)); + + const auto device = nmos::find_resource(resources, { device_id, nmos::types::device }); + BST_REQUIRE(resources.end() != device); + BST_REQUIRE_EQUAL(node->health.load(), device->health.load()); + + BST_REQUIRE_EQUAL(2u, nmos::erase_resource(resources, node_id)); +}