-
Notifications
You must be signed in to change notification settings - Fork 67
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.
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
Fix potential unsafe initialization in the Graph class #606
Changes from all commits
be6caa9
05b847d
a805fcd
15b8975
5f606f0
3ea9feb
a08852b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
#include <cstdint> | ||
#include <functional> | ||
#include <map> | ||
#include <memory> | ||
#include <ostream> | ||
#include <set> | ||
|
||
|
@@ -205,7 +206,8 @@ namespace graph | |
class UndirectedEdge : public Edge<E> | ||
{ | ||
/// \brief An invalid undirected edge. | ||
public: static UndirectedEdge<E> NullEdge; | ||
// Deprecated in favor of NullEdge(). | ||
public: static UndirectedEdge<E> GZ_DEPRECATED(8) NullEdge; | ||
|
||
/// \brief Constructor. | ||
/// \param[in] _vertices The vertices of the edge. | ||
|
@@ -257,6 +259,7 @@ namespace graph | |
}; | ||
|
||
/// \brief An invalid undirected edge. | ||
// Deprecated in favor of NullEdge(). | ||
template<typename E> | ||
UndirectedEdge<E> UndirectedEdge<E>::NullEdge( | ||
VertexId_P(kNullId, kNullId), E(), 1.0, kNullId); | ||
|
@@ -268,7 +271,8 @@ namespace graph | |
class DirectedEdge : public Edge<E> | ||
{ | ||
/// \brief An invalid directed edge. | ||
public: static DirectedEdge<E> NullEdge; | ||
// Deprecated in favor of NullEdge(). | ||
public: static DirectedEdge<E> GZ_DEPRECATED(8) NullEdge; | ||
|
||
/// \brief Constructor. | ||
/// \param[in] _vertices The vertices of the edge. | ||
|
@@ -332,9 +336,19 @@ namespace graph | |
}; | ||
|
||
/// \brief An invalid directed edge. | ||
// Deprecated in favor of NullEdge(). | ||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be better to use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. trying again in #612 |
||
} | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
} | ||
|
||
|
@@ -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. | ||
|
@@ -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); | ||
|
@@ -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. | ||
|
@@ -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; | ||
} | ||
|
@@ -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; | ||
} | ||
|
@@ -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(); | ||
|
@@ -665,7 +665,7 @@ namespace graph | |
} | ||
} | ||
|
||
return EdgeType::NullEdge; | ||
return NullEdge<E, EdgeType>(); | ||
} | ||
|
||
/// \brief Get a reference to an edge using its Id. | ||
|
@@ -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; | ||
} | ||
|
@@ -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; | ||
} | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)https://build.osrfoundation.org/view/gz-ionic/job/sdformat15-install_bottle-homebrew-amd64/210/