From 0be351886e8b9a34ff31e461daa9a59c64fe8fc9 Mon Sep 17 00:00:00 2001 From: Ajay-26 Date: Mon, 6 May 2024 12:28:30 -0500 Subject: [PATCH 1/6] optional should first check whether the node indeed exists --- include/CXXGraph/Graph/Graph_impl.hpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/include/CXXGraph/Graph/Graph_impl.hpp b/include/CXXGraph/Graph/Graph_impl.hpp index e6ca0221..3825ebf7 100644 --- a/include/CXXGraph/Graph/Graph_impl.hpp +++ b/include/CXXGraph/Graph/Graph_impl.hpp @@ -232,8 +232,11 @@ void Graph::removeEdge(const CXXGraph::id_t edgeId) { template void Graph::removeNode(const std::string &nodeUserId) { auto nodeOpt = getNode(nodeUserId); - auto isolatedNodeIt = isolatedNodesSet.find(nodeOpt.value()); - + auto isolatedNodeIt = isolatedNodesSet.end(); + if (nodeOpt) { + isolatedNodeIt = isolatedNodeIt.find(nodeOpt.value()); + } + if (nodeOpt.has_value() && isolatedNodeIt != isolatedNodesSet.end()) { // The node is isolated isolatedNodesSet.erase(isolatedNodeIt); From 5ced82919a80b46f88ad41ca55e6155e6d56f426 Mon Sep 17 00:00:00 2001 From: Ajay-26 Date: Mon, 6 May 2024 12:46:27 -0500 Subject: [PATCH 2/6] fixed typo in impl and also added test for removing node that was never added --- include/CXXGraph/Graph/Graph_impl.hpp | 2 +- test/GraphTest.cpp | 39 +++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/include/CXXGraph/Graph/Graph_impl.hpp b/include/CXXGraph/Graph/Graph_impl.hpp index 3825ebf7..17f315a4 100644 --- a/include/CXXGraph/Graph/Graph_impl.hpp +++ b/include/CXXGraph/Graph/Graph_impl.hpp @@ -234,7 +234,7 @@ void Graph::removeNode(const std::string &nodeUserId) { auto nodeOpt = getNode(nodeUserId); auto isolatedNodeIt = isolatedNodesSet.end(); if (nodeOpt) { - isolatedNodeIt = isolatedNodeIt.find(nodeOpt.value()); + isolatedNodeIt = isolatedNodesSet.find(nodeOpt.value()); } if (nodeOpt.has_value() && isolatedNodeIt != isolatedNodesSet.end()) { diff --git a/test/GraphTest.cpp b/test/GraphTest.cpp index 3976be2b..be271599 100644 --- a/test/GraphTest.cpp +++ b/test/GraphTest.cpp @@ -1340,6 +1340,45 @@ TEST(TestRemoveNode, Test_connectedNode) { ASSERT_EQ(graph.getEdgeSet().size(), 1); } +TEST(TestRemoveNode, Test_removeInvalidNode) { + /** Test to call the remove_node function on a node that was never added. In this case getNode will return an optional that is nullptr*/ + // Create a graph with 3 nodes and 3 edges. + CXXGraph::Node node1("1", 1); + CXXGraph::Node node2("2", 2); + CXXGraph::Node node3("3", 3); + CXXGraph::DirectedEdge edge1(1, node1, node2); + CXXGraph::DirectedEdge edge2(2, node2, node1); + CXXGraph::DirectedEdge edge3(3, node1, node3); + CXXGraph::T_EdgeSet edgeSet; + // Add the 3 edges into the graph. + edgeSet.insert(make_shared>(edge1)); + edgeSet.insert(make_shared>(edge2)); + edgeSet.insert(make_shared>(edge3)); + // Initialise the graph + CXXGraph::Graph graph(edgeSet); + + // Check the initial number of edges and nodes. Everything should be okay so far + ASSERT_EQ(graph.getNodeSet().size(), 3); + ASSERT_EQ(graph.getEdgeSet().size(), 3); + + // Remove a node that was never in the graph + graph.removeNode("4"); + + // Number of nodes and edges in the graph should remain the same + ASSERT_EQ(graph.getNodeSet().size(), 3); + ASSERT_EQ(graph.getEdgeSet().size(), 3); + + // Remove an existing node, the edge associated with that node should also be removed. Node "3" had just outgoing edge, so there should now be 2 nodes and 2 edges. + graph.removeNode("3"); + ASSERT_EQ(graph.getNodeSet().size(), 2); + ASSERT_EQ(graph.getEdgeSet().size(), 2); + + // Remove the node that had already been removed. Should not change anything about the graph now, similar to when "4" was removed above + graph.removeNode("3"); + ASSERT_EQ(graph.getNodeSet().size(), 2); + ASSERT_EQ(graph.getEdgeSet().size(), 2); +} + TEST(TestGetNode, Test_1) { CXXGraph::Node node1("1", 1); CXXGraph::Node node2("2", 2); From d8d07c1b19f134a852309c9649a900ed23e54585 Mon Sep 17 00:00:00 2001 From: Ajay-26 Date: Mon, 6 May 2024 13:53:19 -0500 Subject: [PATCH 3/6] incorporated review into PR --- include/CXXGraph/Graph/Graph_impl.hpp | 30 +++++++++++++-------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/include/CXXGraph/Graph/Graph_impl.hpp b/include/CXXGraph/Graph/Graph_impl.hpp index 17f315a4..eb47aa6a 100644 --- a/include/CXXGraph/Graph/Graph_impl.hpp +++ b/include/CXXGraph/Graph/Graph_impl.hpp @@ -232,25 +232,23 @@ void Graph::removeEdge(const CXXGraph::id_t edgeId) { template void Graph::removeNode(const std::string &nodeUserId) { auto nodeOpt = getNode(nodeUserId); - auto isolatedNodeIt = isolatedNodesSet.end(); if (nodeOpt) { - isolatedNodeIt = isolatedNodesSet.find(nodeOpt.value()); - } - - if (nodeOpt.has_value() && isolatedNodeIt != isolatedNodesSet.end()) { - // The node is isolated - isolatedNodesSet.erase(isolatedNodeIt); - } else if (nodeOpt.has_value()) { - // The node is not isolated - // Remove the edges containing the node - auto edgeset = edgeSet; - for (auto edgeIt : edgeset) { - if (edgeIt->getNodePair().first->getUserId() == nodeUserId || - edgeIt->getNodePair().second->getUserId() == nodeUserId) { - this->removeEdge(edgeIt->getId()); + auto isolatedNodeIt = isolatedNodesSet.find(nodeOpt.value()); + if (isolatedNodeIt != isolatedNodesSet.end()) { + // The node is isolated + isolatedNodesSet.erase(isolatedNodeIt); + } else { + // The node is not isolated + // Remove the edges containing the node + auto edgeset = edgeSet; + for (auto edgeIt : edgeset) { + if (edgeIt->getNodePair().first->getUserId() == nodeUserId || + edgeIt->getNodePair().second->getUserId() == nodeUserId) { + this->removeEdge(edgeIt->getId()); + } } } - } + } } template From 7f1efe66fbce48edf999de2db2aee9e81d28d31f Mon Sep 17 00:00:00 2001 From: Ajay-26 Date: Tue, 7 May 2024 09:35:58 -0500 Subject: [PATCH 4/6] resolved use after free for map iterator --- include/CXXGraph/Graph/Graph_impl.hpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/include/CXXGraph/Graph/Graph_impl.hpp b/include/CXXGraph/Graph/Graph_impl.hpp index eb47aa6a..94551758 100644 --- a/include/CXXGraph/Graph/Graph_impl.hpp +++ b/include/CXXGraph/Graph/Graph_impl.hpp @@ -232,7 +232,7 @@ void Graph::removeEdge(const CXXGraph::id_t edgeId) { template void Graph::removeNode(const std::string &nodeUserId) { auto nodeOpt = getNode(nodeUserId); - if (nodeOpt) { + if (nodeOpt.has_value()) { auto isolatedNodeIt = isolatedNodesSet.find(nodeOpt.value()); if (isolatedNodeIt != isolatedNodesSet.end()) { // The node is isolated @@ -240,12 +240,16 @@ void Graph::removeNode(const std::string &nodeUserId) { } else { // The node is not isolated // Remove the edges containing the node - auto edgeset = edgeSet; - for (auto edgeIt : edgeset) { - if (edgeIt->getNodePair().first->getUserId() == nodeUserId || - edgeIt->getNodePair().second->getUserId() == nodeUserId) { - this->removeEdge(edgeIt->getId()); + auto edgeIt = edgeSet.begin(); + auto nextIt = edgeIt; + while(edgeIt != edgeSet.end()) { + nextIt = std::next(edgeIt); + if ((*edgeIt)->getNodePair().first->getUserId() == nodeUserId || + (*edgeIt)->getNodePair().second->getUserId() == nodeUserId) { + + this->removeEdge((*edgeIt)->getId()); } + edgeIt = nextIt; } } } From 4489104f2190b788472871270e31eb26549d03a4 Mon Sep 17 00:00:00 2001 From: Ajay-26 Date: Tue, 7 May 2024 09:49:35 -0500 Subject: [PATCH 5/6] reformatted code as per PR feedback --- include/CXXGraph/Graph/Graph_impl.hpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/include/CXXGraph/Graph/Graph_impl.hpp b/include/CXXGraph/Graph/Graph_impl.hpp index 94551758..58eb16a3 100644 --- a/include/CXXGraph/Graph/Graph_impl.hpp +++ b/include/CXXGraph/Graph/Graph_impl.hpp @@ -233,7 +233,7 @@ template void Graph::removeNode(const std::string &nodeUserId) { auto nodeOpt = getNode(nodeUserId); if (nodeOpt.has_value()) { - auto isolatedNodeIt = isolatedNodesSet.find(nodeOpt.value()); + auto isolatedNodeIt = isolatedNodesSet.find(nodeOpt.value()); if (isolatedNodeIt != isolatedNodesSet.end()) { // The node is isolated isolatedNodesSet.erase(isolatedNodeIt); @@ -242,17 +242,16 @@ void Graph::removeNode(const std::string &nodeUserId) { // Remove the edges containing the node auto edgeIt = edgeSet.begin(); auto nextIt = edgeIt; - while(edgeIt != edgeSet.end()) { + while (edgeIt != edgeSet.end()) { nextIt = std::next(edgeIt); if ((*edgeIt)->getNodePair().first->getUserId() == nodeUserId || (*edgeIt)->getNodePair().second->getUserId() == nodeUserId) { - this->removeEdge((*edgeIt)->getId()); } edgeIt = nextIt; } } - } + } } template From 6f4e6540838837af09f97769ed77615001cfefef Mon Sep 17 00:00:00 2001 From: Ajay-26 Date: Tue, 7 May 2024 09:49:54 -0500 Subject: [PATCH 6/6] reformatted code as per PR feedback --- test/GraphTest.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/test/GraphTest.cpp b/test/GraphTest.cpp index be271599..b272f344 100644 --- a/test/GraphTest.cpp +++ b/test/GraphTest.cpp @@ -1341,7 +1341,8 @@ TEST(TestRemoveNode, Test_connectedNode) { } TEST(TestRemoveNode, Test_removeInvalidNode) { - /** Test to call the remove_node function on a node that was never added. In this case getNode will return an optional that is nullptr*/ + /** Test to call the remove_node function on a node that was never added. In + * this case getNode will return an optional that is nullptr*/ // Create a graph with 3 nodes and 3 edges. CXXGraph::Node node1("1", 1); CXXGraph::Node node2("2", 2); @@ -1357,7 +1358,8 @@ TEST(TestRemoveNode, Test_removeInvalidNode) { // Initialise the graph CXXGraph::Graph graph(edgeSet); - // Check the initial number of edges and nodes. Everything should be okay so far + // Check the initial number of edges and nodes. Everything should be okay so + // far ASSERT_EQ(graph.getNodeSet().size(), 3); ASSERT_EQ(graph.getEdgeSet().size(), 3); @@ -1368,12 +1370,15 @@ TEST(TestRemoveNode, Test_removeInvalidNode) { ASSERT_EQ(graph.getNodeSet().size(), 3); ASSERT_EQ(graph.getEdgeSet().size(), 3); - // Remove an existing node, the edge associated with that node should also be removed. Node "3" had just outgoing edge, so there should now be 2 nodes and 2 edges. + // Remove an existing node, the edge associated with that node should also be + // removed. Node "3" had just outgoing edge, so there should now be 2 nodes + // and 2 edges. graph.removeNode("3"); ASSERT_EQ(graph.getNodeSet().size(), 2); ASSERT_EQ(graph.getEdgeSet().size(), 2); - // Remove the node that had already been removed. Should not change anything about the graph now, similar to when "4" was removed above + // Remove the node that had already been removed. Should not change anything + // about the graph now, similar to when "4" was removed above graph.removeNode("3"); ASSERT_EQ(graph.getNodeSet().size(), 2); ASSERT_EQ(graph.getEdgeSet().size(), 2);