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.

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 memory error due to missing rosout_disable_topics_generation para… #1507

Merged
merged 3 commits into from
Sep 21, 2018
Merged

Fix memory error due to missing rosout_disable_topics_generation para… #1507

merged 3 commits into from
Sep 21, 2018

Conversation

hdino
Copy link
Contributor

@hdino hdino commented Sep 21, 2018

Getting the parameter rosout_disable_topics_generation was not secured by a default value and thus could cause a memory error if it is not set (cf. https://lists.debian.org/debian-science/2018/09/msg00046.html.

Copy link
Contributor

@mgrrx mgrrx left a comment

Choose a reason for hiding this comment

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

I suggest to also initialize the variable in the initializer of the class.

@@ -107,7 +108,9 @@ void ROSOutAppender::log(::ros::console::Level level, const char* str, const cha
// check parameter server/cache for omit_topics flag
// the same parameter is checked in rosout.py for the same purpose
if (!ros::param::getCached("/rosout_disable_topics_generation", disable_topics_))
disable_topics_ = false;
{
disable_topics_ = false;
Copy link
Member

Choose a reason for hiding this comment

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

I guess we don't need this if the default is set in the initializer, right?

@hdino
Copy link
Contributor Author

hdino commented Sep 21, 2018

Generally not, but I left the check of the return value of ros::param::getCached in case a future version alters its argument. However, this might be a bit paranoid...

@dirk-thomas
Copy link
Member

Thanks for the patch.

@dirk-thomas dirk-thomas merged commit ef54759 into ros:melodic-devel Sep 21, 2018
tahsinkose pushed a commit to tahsinkose/ros_comm that referenced this pull request Apr 15, 2019
ros#1507)

* Fix memory error due to missing rosout_disable_topics_generation parameter

* Initialise disable_topics_ in constructor of ROSOutAppender

* Removed security check for rosout_disable_topics_generation parameter in favour of a class initialiser
dirk-thomas pushed a commit that referenced this pull request Aug 3, 2020
#1507)

* Fix memory error due to missing rosout_disable_topics_generation parameter

* Initialise disable_topics_ in constructor of ROSOutAppender

* Removed security check for rosout_disable_topics_generation parameter in favour of a class initialiser
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants