Skip to content

check length of display-config parameter before extracting substring #1189

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

Conversation

dseifert
Copy link
Contributor

When running rviz -d bla, rviz would segfault as it tried to extract a substring of the display-config parameter without checking its size first.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Other than style comments, lgtm, I will merge with fix ups and green CI.

@@ -172,7 +172,7 @@ bool VisualizerApp::init( int argc, char** argv )
if (vm.count("display-config"))
{
display_config = vm["display-config"].as<std::string>();
if( display_config.substr( display_config.size() - 4, 4 ) == ".vcg" )
if (display_config.size() >= 4 and display_config.substr( display_config.size() - 4, 4 ) == ".vcg" )
Copy link
Member

Choose a reason for hiding this comment

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

Please use &&. Also, I know the rviz style is weird, but please preserve the whitespace when possible, should be if ( dis... if you like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I switch to if ( dis... even if (almost) the entire rest of the file does not use this style or shall I reformat the file this way?

Copy link
Member

Choose a reason for hiding this comment

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

Nah, that's ok, it's pretty inconsistent in this version anyways.

@dseifert dseifert force-pushed the fix-segfault-on-short-display-config branch from b002d62 to 2275eab Compare January 29, 2018 06:21
@wjwwood wjwwood merged commit fe0512e into ros-visualization:kinetic-devel Jan 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants