From 45cf5baa51963409c477fb85720b285c3fcb51d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Ag=C3=BCero?= Date: Fri, 12 Jul 2024 19:29:12 +0200 Subject: [PATCH 1/3] Fix potential unsafe initialization in the Graph class (#606) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Avoid constructor/destructor fiascos in graph class * Deprecations and migration guide. Signed-off-by: Carlos Agüero Signed-off-by: Carlos Agüero Co-authored-by: Steve Peters --- Migration.md | 14 ++++++++++++++ include/gz/math/graph/Edge.hh | 18 ++++++++++++++++-- include/gz/math/graph/Graph.hh | 20 ++++++++++---------- include/gz/math/graph/Vertex.hh | 15 +++++++++++++-- 4 files changed, 53 insertions(+), 14 deletions(-) diff --git a/Migration.md b/Migration.md index e5444ebd..8bd35f88 100644 --- a/Migration.md +++ b/Migration.md @@ -5,6 +5,20 @@ Deprecated code produces compile-time warnings. These warning serve as notification to users that their code should be upgraded. The next major release will remove the deprecated code. +## Gazebo Math 8.X to 9.X + +### Deprecations + +1. **graph/Vertex.hh** + + The `Vertex::NullVertex` static member is deprecated. Please use + `Vertex::NullVertex()` instead. + E.g.: https://github.com/gazebosim/gz-math/pull/606/files#diff-0c0220a7e72be70337975433eeddc3f5e072ade5cd80dfb1ac03da233c39c983L153-R153 + +1. **graph/Edge.hh** + + The `Edge::NullEdge` static member is deprecated. Please use + `Vertex::NullEdge()` instead. + E.g.: https://github.com/gazebosim/gz-math/pull/606/files#diff-0c0220a7e72be70337975433eeddc3f5e072ade5cd80dfb1ac03da233c39c983L222-R222 + ## Gazebo Math 7.X to 8.X ### Breaking Changes diff --git a/include/gz/math/graph/Edge.hh b/include/gz/math/graph/Edge.hh index ecdf973f..900ece02 100644 --- a/include/gz/math/graph/Edge.hh +++ b/include/gz/math/graph/Edge.hh @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -203,7 +204,8 @@ namespace graph class UndirectedEdge : public Edge { /// \brief An invalid undirected edge. - public: static UndirectedEdge NullEdge; + // Deprecated in favor of NullEdge(). + public: static UndirectedEdge GZ_DEPRECATED(8) NullEdge; /// \brief Constructor. /// \param[in] _vertices The vertices of the edge. @@ -255,6 +257,7 @@ namespace graph }; /// \brief An invalid undirected edge. + // Deprecated in favor of NullEdge(). template UndirectedEdge UndirectedEdge::NullEdge( VertexId_P(kNullId, kNullId), E(), 1.0, kNullId); @@ -266,7 +269,8 @@ namespace graph class DirectedEdge : public Edge { /// \brief An invalid directed edge. - public: static DirectedEdge NullEdge; + // Deprecated in favor of NullEdge(). + public: static DirectedEdge GZ_DEPRECATED(8) NullEdge; /// \brief Constructor. /// \param[in] _vertices The vertices of the edge. @@ -330,9 +334,19 @@ namespace graph }; /// \brief An invalid directed edge. + // Deprecated in favor of NullEdge(). template DirectedEdge DirectedEdge::NullEdge( VertexId_P(kNullId, kNullId), E(), 1.0, kNullId); + + /// \brief An invalid edge. + template + EdgeType &NullEdge() + { + static auto e = std::make_unique( + VertexId_P(kNullId, kNullId), E(), 1.0, kNullId); + return *e; + } } // namespace graph } // namespace GZ_MATH_VERSION_NAMESPACE } // namespace gz::math diff --git a/include/gz/math/graph/Graph.hh b/include/gz/math/graph/Graph.hh index d3b23726..1469e9e5 100644 --- a/include/gz/math/graph/Graph.hh +++ b/include/gz/math/graph/Graph.hh @@ -148,7 +148,7 @@ namespace graph { std::cerr << "[Graph::AddVertex()] The limit of vertices has been " << "reached. Ignoring vertex." << std::endl; - return Vertex::NullVertex; + return NullVertex(); } } @@ -161,7 +161,7 @@ namespace graph { std::cerr << "[Graph::AddVertex()] Repeated vertex [" << id << "]" << std::endl; - return Vertex::NullVertex; + return NullVertex(); } // Link the vertex with an empty list of edges. @@ -217,7 +217,7 @@ namespace graph { std::cerr << "[Graph::AddEdge()] The limit of edges has been reached. " << "Ignoring edge." << std::endl; - return EdgeType::NullEdge; + return NullEdge(); } EdgeType newEdge(_vertices, _data, _weight, id); @@ -238,7 +238,7 @@ namespace graph for (auto const &v : {edgeVertices.first, edgeVertices.second}) { if (this->vertices.find(v) == this->vertices.end()) - return EdgeType::NullEdge; + return NullEdge(); } // Link the new edge. @@ -609,7 +609,7 @@ namespace graph { auto iter = this->vertices.find(_id); if (iter == this->vertices.end()) - return Vertex::NullVertex; + return NullVertex(); return iter->second; } @@ -622,7 +622,7 @@ namespace graph { auto iter = this->vertices.find(_id); if (iter == this->vertices.end()) - return Vertex::NullVertex; + return NullVertex(); return iter->second; } @@ -644,7 +644,7 @@ namespace graph // Quit early if there is no adjacency entry if (adjIt == this->adjList.end()) - return EdgeType::NullEdge; + return NullEdge(); // Loop over the edges in the source vertex's adjacency list for (std::set::const_iterator edgIt = adjIt->second.begin(); @@ -663,7 +663,7 @@ namespace graph } } - return EdgeType::NullEdge; + return NullEdge(); } /// \brief Get a reference to an edge using its Id. @@ -674,7 +674,7 @@ namespace graph { auto iter = this->edges.find(_id); if (iter == this->edges.end()) - return EdgeType::NullEdge; + return NullEdge(); return iter->second; } @@ -687,7 +687,7 @@ namespace graph { auto iter = this->edges.find(_id); if (iter == this->edges.end()) - return EdgeType::NullEdge; + return NullEdge(); return iter->second; } diff --git a/include/gz/math/graph/Vertex.hh b/include/gz/math/graph/Vertex.hh index 7d99c373..bfeef6c3 100644 --- a/include/gz/math/graph/Vertex.hh +++ b/include/gz/math/graph/Vertex.hh @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -43,7 +44,7 @@ namespace graph using VertexId_P = std::pair; /// \brief Represents an invalid Id. - static const VertexId kNullId = MAX_UI64; + constexpr VertexId kNullId = MAX_UI64; /// \brief A vertex of a graph. It stores user information, an optional name, /// and keeps an internal unique Id. This class does not enforce to choose a @@ -52,7 +53,8 @@ namespace graph class Vertex { /// \brief An invalid vertex. - public: static Vertex NullVertex; + // Deprecated in favor of NullVertex(). + public: static Vertex GZ_DEPRECATED(8) NullVertex; /// \brief Constructor. /// \param[in] _name Non-unique vertex name. @@ -134,9 +136,18 @@ namespace graph }; /// \brief An invalid vertex. + // Deprecated in favor of NullVertex(). template Vertex Vertex::NullVertex("__null__", V(), kNullId); + /// \brief An invalid vertex. + template + Vertex &NullVertex() + { + static auto v = std::make_unique>("__null__", V(), kNullId); + return *v; + } + /// \def VertexRef_M /// \brief Map of vertices. The key is the vertex Id. The value is a /// reference to the vertex. From e74d61216341532717680d6536c72e9e8634cb27 Mon Sep 17 00:00:00 2001 From: Steve Peters Date: Thu, 18 Jul 2024 17:03:47 -0700 Subject: [PATCH 2/3] Use local static instead of dynamic allocation Signed-off-by: Steve Peters --- include/gz/math/graph/Edge.hh | 5 ++--- include/gz/math/graph/Vertex.hh | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/include/gz/math/graph/Edge.hh b/include/gz/math/graph/Edge.hh index 900ece02..f1a4c128 100644 --- a/include/gz/math/graph/Edge.hh +++ b/include/gz/math/graph/Edge.hh @@ -21,7 +21,6 @@ #include #include #include -#include #include #include @@ -343,9 +342,9 @@ namespace graph template EdgeType &NullEdge() { - static auto e = std::make_unique( + static EdgeType e( VertexId_P(kNullId, kNullId), E(), 1.0, kNullId); - return *e; + return e; } } // namespace graph } // namespace GZ_MATH_VERSION_NAMESPACE diff --git a/include/gz/math/graph/Vertex.hh b/include/gz/math/graph/Vertex.hh index bfeef6c3..883528c1 100644 --- a/include/gz/math/graph/Vertex.hh +++ b/include/gz/math/graph/Vertex.hh @@ -21,7 +21,6 @@ #include #include #include -#include #include #include #include @@ -144,8 +143,8 @@ namespace graph template Vertex &NullVertex() { - static auto v = std::make_unique>("__null__", V(), kNullId); - return *v; + static Vertex v("__null__", V(), kNullId); + return v; } /// \def VertexRef_M From 3fa98549b1e781aca2f17909538c4891548963ad Mon Sep 17 00:00:00 2001 From: Steve Peters Date: Tue, 30 Jul 2024 17:01:09 -0700 Subject: [PATCH 3/3] Use gz::utils::NeverDestroyed Signed-off-by: Steve Peters --- include/gz/math/graph/Edge.hh | 7 ++++--- include/gz/math/graph/Vertex.hh | 5 +++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/include/gz/math/graph/Edge.hh b/include/gz/math/graph/Edge.hh index f1a4c128..f02f4ba4 100644 --- a/include/gz/math/graph/Edge.hh +++ b/include/gz/math/graph/Edge.hh @@ -25,7 +25,8 @@ #include #include -#include "gz/math/graph/Vertex.hh" +#include +#include namespace gz::math { @@ -342,9 +343,9 @@ namespace graph template EdgeType &NullEdge() { - static EdgeType e( + static gz::utils::NeverDestroyed e( VertexId_P(kNullId, kNullId), E(), 1.0, kNullId); - return e; + return e.Access(); } } // namespace graph } // namespace GZ_MATH_VERSION_NAMESPACE diff --git a/include/gz/math/graph/Vertex.hh b/include/gz/math/graph/Vertex.hh index 883528c1..f6d2a71e 100644 --- a/include/gz/math/graph/Vertex.hh +++ b/include/gz/math/graph/Vertex.hh @@ -27,6 +27,7 @@ #include #include +#include namespace gz::math { @@ -143,8 +144,8 @@ namespace graph template Vertex &NullVertex() { - static Vertex v("__null__", V(), kNullId); - return v; + static gz::utils::NeverDestroyed> v("__null__", V(), kNullId); + return v.Access(); } /// \def VertexRef_M