Skip to content
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

Jump to bottom

Fix potential unsafe initialization in the Graph class #606

Merged
merged 7 commits into from
Jul 12, 2024
Merged

Fix potential unsafe initialization in the Graph class #606

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
be6caa9
Avoid constructor/destructor fiascos in graph class
caguero Jul 2, 2024
05b847d
Merge branch 'main' into caguero/issue_269_graph
caguero Jul 2, 2024
a805fcd
Merge branch 'main' into caguero/issue_269_graph
caguero Jul 10, 2024
15b8975
Deprecations and migration guide.
caguero Jul 11, 2024
5f606f0
Breaking changes.
caguero Jul 11, 2024
3ea9feb
Update Migration.md
caguero Jul 12, 2024
a08852b
Tweak
caguero Jul 12, 2024
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
24 changes: 9 additions & 15 deletions include/gz/math/graph/Edge.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <cstdint>
#include <functional>
#include <map>
#include <memory>
#include <ostream>
#include <set>

Expand Down Expand Up @@ -204,9 +205,6 @@ namespace graph
template<typename E>
class UndirectedEdge : public Edge<E>
{
/// \brief An invalid undirected edge.
public: static UndirectedEdge<E> NullEdge;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may choose to make a hard API break here, but I'd prefer to not remove these static types in this pull request, but to deprecate them instead, so that we can coordinate fixes for sdformat and any other gz-* packages in separate pull requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if the changes in 15b8975 are sufficient to keep sdformat and friends compiling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like sdformat15 will still compile with this change as of 5f606f0 (tested using the ci_matching_branch/ trick partially documented in gazebosim/docs#377)

Build Status https://build.osrfoundation.org/view/gz-ionic/job/sdformat15-install_bottle-homebrew-amd64/210/


/// \brief Constructor.
/// \param[in] _vertices The vertices of the edge.
/// \param[in] _data The data stored in the edge.
Expand Down Expand Up @@ -256,20 +254,12 @@ namespace graph
}
};

/// \brief An invalid undirected edge.
template<typename E>
UndirectedEdge<E> UndirectedEdge<E>::NullEdge(
VertexId_P(kNullId, kNullId), E(), 1.0, kNullId);

/// \brief A directed edge represents a connection between two vertices.
/// The connection is unidirectional, it's only possible to traverse the edge
/// in one direction (from the tail to the head).
template<typename E>
class DirectedEdge : public Edge<E>
{
/// \brief An invalid directed edge.
public: static DirectedEdge<E> NullEdge;

/// \brief Constructor.
/// \param[in] _vertices The vertices of the edge.
/// \param[in] _data The data stored in the edge.
Expand Down Expand Up @@ -331,10 +321,14 @@ namespace graph
}
};

/// \brief An invalid directed edge.
template<typename E>
DirectedEdge<E> DirectedEdge<E>::NullEdge(
VertexId_P(kNullId, kNullId), E(), 1.0, kNullId);
/// \brief An invalid edge.
template<typename E, typename EdgeType>
EdgeType &NullEdge()
{
static auto e = std::make_unique<EdgeType>(
VertexId_P(kNullId, kNullId), E(), 1.0, kNullId);
return *e;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at Material::Predefined(), I wonder if the unique pointer should be made and released from inside a static lambda function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to use gz::utils::NeverDestroyed (e.g. https://github.com/gazebosim/sdformat/blob/f05f4e7ad1a6784f9ff1a6c1b362191677baa70d/src/Types.cc#L149). However, the fact that this function returns a non-const reference could be problematic since anyone can modify the value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding the non-const reference, I think the behavior in gz-math7 and earlier with static variables has a similar issue right? I tried changing the NullEdge and NullVertex functions to return a const reference, but lots of Graph.hh functions that currently return non-const references like to return references to NullEdge or NullVertex and fail to compile. This seems like a bigger issue with the API that we could open an issue about.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trying again in #612

}
}
}
}
Expand Down
20 changes: 10 additions & 10 deletions include/gz/math/graph/Graph.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ namespace graph
{
std::cerr << "[Graph::AddVertex()] The limit of vertices has been "
<< "reached. Ignoring vertex." << std::endl;
return Vertex<V>::NullVertex;
return NullVertex<V>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like an API change; we may need to do it, but let me check if sdformat will be affected

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this will require changes in sdformat:

https://github.com/gazebosim/sdformat/blob/main/src/FrameSemantics.cc#L89

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I restored the original static members and added deprecations. It should be possible to keep using them.
15b8975

}
}

Expand All @@ -163,7 +163,7 @@ namespace graph
{
std::cerr << "[Graph::AddVertex()] Repeated vertex [" << id << "]"
<< std::endl;
return Vertex<V>::NullVertex;
return NullVertex<V>();
}

// Link the vertex with an empty list of edges.
Expand Down Expand Up @@ -219,7 +219,7 @@ namespace graph
{
std::cerr << "[Graph::AddEdge()] The limit of edges has been reached. "
<< "Ignoring edge." << std::endl;
return EdgeType::NullEdge;
return NullEdge<E, EdgeType>();
}

EdgeType newEdge(_vertices, _data, _weight, id);
Expand All @@ -240,7 +240,7 @@ namespace graph
for (auto const &v : {edgeVertices.first, edgeVertices.second})
{
if (this->vertices.find(v) == this->vertices.end())
return EdgeType::NullEdge;
return NullEdge<E, EdgeType>();
}

// Link the new edge.
Expand Down Expand Up @@ -611,7 +611,7 @@ namespace graph
{
auto iter = this->vertices.find(_id);
if (iter == this->vertices.end())
return Vertex<V>::NullVertex;
return NullVertex<V>();

return iter->second;
}
Expand All @@ -624,7 +624,7 @@ namespace graph
{
auto iter = this->vertices.find(_id);
if (iter == this->vertices.end())
return Vertex<V>::NullVertex;
return NullVertex<V>();

return iter->second;
}
Expand All @@ -646,7 +646,7 @@ namespace graph

// Quit early if there is no adjacency entry
if (adjIt == this->adjList.end())
return EdgeType::NullEdge;
return NullEdge<E, EdgeType>();

// Loop over the edges in the source vertex's adjacency list
for (std::set<EdgeId>::const_iterator edgIt = adjIt->second.begin();
Expand All @@ -665,7 +665,7 @@ namespace graph
}
}

return EdgeType::NullEdge;
return NullEdge<E, EdgeType>();
}

/// \brief Get a reference to an edge using its Id.
Expand All @@ -676,7 +676,7 @@ namespace graph
{
auto iter = this->edges.find(_id);
if (iter == this->edges.end())
return EdgeType::NullEdge;
return NullEdge<E, EdgeType>();

return iter->second;
}
Expand All @@ -689,7 +689,7 @@ namespace graph
{
auto iter = this->edges.find(_id);
if (iter == this->edges.end())
return EdgeType::NullEdge;
return NullEdge<E, EdgeType>();

return iter->second;
}
Expand Down
12 changes: 7 additions & 5 deletions include/gz/math/graph/Vertex.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <cstdint>
#include <functional>
#include <map>
#include <memory>
#include <ostream>
#include <string>
#include <utility>
Expand All @@ -45,17 +46,14 @@ namespace graph
using VertexId_P = std::pair<VertexId, VertexId>;

/// \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
/// unique name.
template<typename V>
class Vertex
{
/// \brief An invalid vertex.
public: static Vertex<V> NullVertex;

/// \brief Constructor.
/// \param[in] _name Non-unique vertex name.
/// \param[in] _data User information.
Expand Down Expand Up @@ -137,7 +135,11 @@ namespace graph

/// \brief An invalid vertex.
template<typename V>
Vertex<V> Vertex<V>::NullVertex("__null__", V(), kNullId);
Vertex<V> &NullVertex()
{
static auto v = std::make_unique<Vertex<V>>("__null__", V(), kNullId);
return *v;
}

/// \def VertexRef_M
/// \brief Map of vertices. The key is the vertex Id. The value is a
Expand Down