Skip to content

Protect erase_resource and set_resource_health against cyclic sub-resources #510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ZayanKhan-12 wants to merge 1 commit into from
Closed

Protect erase_resource and set_resource_health against cyclic sub-resources #510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Development/cmake/NmosCppTest.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
109 changes: 69 additions & 40 deletions Development/nmos/resources.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<nmos::id>& 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<nmos::id> erasing;
return details::erase_resource(resources, id, forget_now, erasing);
}

// forget all erased resources which expired *before* the specified time from the specified resources
Expand Down Expand Up @@ -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<nmos::id>& 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<nmos::id> setting;
details::set_resource_health(resources, id, health, setting);
}

static inline std::pair<id, type> no_resource() { return{}; }

// get the super-resource id and type, according to the guidelines on referential integrity
Expand Down
78 changes: 78 additions & 0 deletions Development/nmos/test/resources_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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")));
}