Fix use of uninitialized memory in simple_tf2_core tests. #18

clalancette · 2017-04-25T15:10:40Z

Since geometry_msgs::msg::TransformStamped is declared with
an empty constructor, all of its fields are uninitialized.
This means that anytime we want to use it, we must initialize
all fields ourselves. The tests in simple_tf2_core.cpp were
failing to do this, so parts of tf2 were making decisions on
basically random data. This was causing a bunch of valgrind
failures. With this commit, the test becomes valgrind clean.

Signed-off-by: Chris Lalancette clalancette@osrfoundation.org

Since geometry_msgs::msg::TransformStamped is declared with an empty constructor, all of its fields are uninitialized. This means that anytime we want to use it, we must initialize all fields ourselves. The tests in simple_tf2_core.cpp were failing to do this, so parts of tf2 were making decisions on basically random data. This was causing a bunch of valgrind failures. With this commit, the test becomes valgrind clean. Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

clalancette · 2017-04-25T15:38:54Z

CI:
windows:
linux:
linux aarch64:
osx:

mikaelarguedas · 2017-04-25T15:23:48Z

tf2/test/simple_tf2_core.cpp

-}
-
-
-


Is that related to uninitialized messages?

No, not directly. It just wasn't used. This is a piece that can go upstream as well.

in that case can you make this in separate commits in the future. This way you can submit the same commit upstream.

mikaelarguedas · 2017-04-25T15:41:54Z

tf2/test/simple_tf2_core.cpp

+  st.transform.rotation.x = 0;
+  st.transform.rotation.y = 0;
+  st.transform.rotation.z = 0;
+  st.transform.rotation.w = 1;


Can you provide an example / test showing that they're not initialized? If they're not initialized it should be fixed in the message generation and not in the test of a leaf package.

So, we should probably have a conversation about this. This is the second time I've had to do something like this in a test (I can't remember where the other one was, but I can dig it up). What's going on here is that a declaration like:

geometry_msgs::msg::TransformStamped foo;

has a constructor that looks like this:

TransformStamped_()
{
}
explicit TransformStamped_(const ContainerAllocator & _alloc)
// INDENT-OFF (prevent uncrustify from making unnecessary indents here)
// INDENT-ON
{
(void)_alloc;
}

(from transform_stamped__struct.hpp). As far as I understand, what that means is that that constructor doesn't initialize any member variables. Thus, when you use the object later on, everything is uninitialized. When you run this under valgrind, it throws a slew of errors saying you are making decisions using uninitialized variables, etc.

I think you are right, by the way; for the long-term maintenance of this stuff, we should probably do a real constructor for these messages by changing the message generation.

So let's see. I'm discussing this with @dirk-thomas on ros2/rosidl#77 . If we decide to do a default initialization of all fields, then that will take time to implement. If we decide not to do it, then we have to fill in all of the fields. Thus, my feeling is that for the short-term, we should take this code (after review, of course), no matter which way we decide to go. Thoughts?

👍 sounds good to me

mikaelarguedas · 2017-04-25T15:45:04Z

EDIT: Just saw that you pushed -f adding valgrind information. In that case I confirm that I think this should be fixed in the constructors and not in this test

mikaelarguedas

looks good to me, all ci are green 🎉 , can you please open a PR upstream for the function removal?

clalancette · 2017-04-25T16:43:20Z

Absolutely.

dhood · 2017-04-25T16:44:36Z

tf2/test/simple_tf2_core.cpp

 TEST(tf2, setTransformFail)
 {
  tf2::BufferCore tfc;
  geometry_msgs::msg::TransformStamped st;
+  st.header.frame_id = "foo";


I think this test was relying on 'bad' values.

Here's the output of an upstream job (http://build.ros.org/job/Ipr__geometry2__ubuntu_trusty_amd64/83/consoleFull search for setTransformFail):

16:02:56 [----------] 3 tests from tf2 16:02:56 [ RUN ] tf2.setTransformFail 16:02:56 Error: TF_SELF_TRANSFORM: Ignoring transform from authority "authority1" with frame_id and child_frame_id "" because they are the same 16:02:56 at line 220 in /tmp/catkin_workspace/src/geometry2/tf2/src/buffer_core.cpp 16:02:56 Error: TF_NO_CHILD_FRAME_ID: Ignoring transform from authority "authority1" because child_frame_id not set 16:02:56 at line 226 in /tmp/catkin_workspace/src/geometry2/tf2/src/buffer_core.cpp 16:02:56 Error: TF_NO_FRAME_ID: Ignoring transform with child_frame_id "" from authority "authority1" because frame_id not set 16:02:56 at line 232 in /tmp/catkin_workspace/src/geometry2/tf2/src/buffer_core.cpp 16:02:56 Error: TF_DENORMALIZED_QUATERNION: Ignoring transform for child_frame_id "" from authority "authority1" because of an invalid quaternion in the transform (0.000000 0.000000 0.000000 0.000000) 16:02:56 at line 256 in /tmp/catkin_workspace/src/geometry2/tf2/src/buffer_core.cpp

But the output of the CI for this job:

16:26:36 3: [ RUN ] tf2.setTransformFail 16:26:36 3: Error: TF_SELF_TRANSFORM: Ignoring transform from authority "authority1" with frame_id and child_frame_id "foo" because they are the same 16:26:36 3: at line 235 in /Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/geometry2/tf2/src/buffer_core.cpp 16:26:36 3: [ OK ] tf2.setTransformFail (0 ms)

Right, will fix.

thanks, I'll use the 'request changes' button from now on so comments don't get buried accidentally

clalancette · 2017-04-25T16:52:09Z

Here's the PR for removing the dead functions upstream: ros/geometry2#227

dhood · 2017-04-25T16:53:58Z

@clalancette please address https://github.com/ros2/geometry2/pull/18/files#r113249081 because the CI is green but the nature of the tests have been changed.

* Fixes policy CMP0135 warning for CMake >= 3.24 Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com> (cherry picked from commit 806f79a81f95290615ed8ca657653344ce9699db) Co-authored-by: Cristóbal Arroyo <69475004+Crola1702@users.noreply.github.com>

clalancette added the in progress Actively being worked on (Kanban column) label Apr 25, 2017

clalancette mentioned this pull request Apr 25, 2017

OSX nightly release build failed a tf2 test ros2/build_farmer#15

Closed

clalancette force-pushed the fix-simple-tf2-core-test branch from b0c0784 to 491bd81 Compare April 25, 2017 15:36

mikaelarguedas reviewed Apr 25, 2017

View reviewed changes

clalancette mentioned this pull request Apr 25, 2017

review default constructors ros2/rosidl#77

Closed

mikaelarguedas approved these changes Apr 25, 2017

View reviewed changes

dhood reviewed Apr 25, 2017

View reviewed changes

clalancette merged commit 110fd71 into ros2 Apr 25, 2017

clalancette deleted the fix-simple-tf2-core-test branch April 25, 2017 16:52

clalancette removed the in progress Actively being worked on (Kanban column) label Apr 25, 2017

dhood mentioned this pull request Jun 19, 2017

new test failure in tf2_geometry_msgs package #24

Closed

tfoote mentioned this pull request Jun 23, 2017

initialize whole quaternion #28

Merged

clalancette mentioned this pull request Nov 2, 2017

Tracking bug for reverting changes made to ported packages for field initialization ros2/ros2#422

Closed

20 tasks

mikaelarguedas mentioned this pull request Jul 5, 2018

revert now unnecessary message initializations #64

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix use of uninitialized memory in simple_tf2_core tests. #18

Fix use of uninitialized memory in simple_tf2_core tests. #18

clalancette commented Apr 25, 2017

clalancette commented Apr 25, 2017 •

edited

Loading

mikaelarguedas Apr 25, 2017

clalancette Apr 25, 2017

mikaelarguedas Apr 25, 2017

mikaelarguedas Apr 25, 2017

clalancette Apr 25, 2017

clalancette Apr 25, 2017

mikaelarguedas Apr 25, 2017

mikaelarguedas commented Apr 25, 2017

mikaelarguedas left a comment

clalancette commented Apr 25, 2017

dhood Apr 25, 2017

clalancette Apr 25, 2017

dhood Apr 25, 2017

clalancette Apr 25, 2017

clalancette commented Apr 25, 2017

dhood commented Apr 25, 2017

Fix use of uninitialized memory in simple_tf2_core tests. #18

Fix use of uninitialized memory in simple_tf2_core tests. #18

Conversation

clalancette commented Apr 25, 2017

clalancette commented Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelarguedas commented Apr 25, 2017

mikaelarguedas left a comment

Choose a reason for hiding this comment

clalancette commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Apr 25, 2017

dhood commented Apr 25, 2017

clalancette commented Apr 25, 2017 •

edited

Loading