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

correctly initialise Ogre types and check return value of 'transform' #1396

Closed
wants to merge 1 commit into from
Closed

Conversation

christian-rauch
Copy link
Contributor

Description

In some sections of the code, the Ogre types where not correctly initialized and return values have not been checked. This PR resolves these issues to prevent crashes due to invalid marker scales from uninitialized memory.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution. However, I think it will be sufficient to return on failure. Can you confirm?

Ogre::Quaternion orient;
transform(new_message, pos, orient, scale);

if(!transform(new_message, pos, orient, scale)) { return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it suffice to have this line fixed, i.e. just return on failure?
All the uninitialized variables wouldn't be used in this case.

@christian-rauch
Copy link
Contributor Author

Yes, in this case it would be sufficient to return and keep the values uninitialised.
But uninitialised values are the root of evil. I usually initialise values on their creating as a good habit and the performance costs of initialising 3-element vectors is neglectable.

@rhaschke
Copy link
Contributor

@christian-rauch, could you provide a marker message causing rviz to fail on this?
From my understanding, transform() shouldn't fail here, since the validity of the transform is checked in advance by a TF message filter. So, I'm curious, which message causes this issue.

@christian-rauch
Copy link
Contributor Author

christian-rauch commented Aug 12, 2019

The issue was related to the transforms, not the marker per se. Due to a misconfiguration from my side, I had two roots in the TF tree and transform returned false at

if (!context_->getFrameManager()->transform(message->header.frame_id, stamp, message->pose, pos, orient))
. This caused the function to return without setting scale.

@rhaschke
Copy link
Contributor

My point is, that if the transform is missing, the message filter should bail out before already.
If that's not happening, we have another issue.

@christian-rauch
Copy link
Contributor Author

I understand. But I am unable to reproduce this issue since I fixed the original problem in the URDF and a couple of other places.

@rhaschke
Copy link
Contributor

Superseded by #1400. @christian-rauch, please validate.

@rhaschke rhaschke closed this Aug 12, 2019
@rhaschke rhaschke mentioned this pull request Aug 12, 2019
@christian-rauch christian-rauch deleted the marker_scale_fix branch August 13, 2019 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants