Free with the same allocator in rmw_destroy_node

[jacobperron]

Since rmw_allocate() was used to allocate memory, we should use rmw_free() to cleanup. Otherwise, if the user provided a custom allocator to the context we will be calling deallocate with the wrong allocator.

---

I hit a runtime error due to this bug while I was using a custom allocator. There may be other places in the code where we're making a similar mistake. I haven't audited the rest of the code, but I'll make sure to fix similar instances if I come across them.

This should be considered for backport to Galactic and Foxy!

[jacobperron]

I just went with a minimal change in this PR, but I agree that using the context allocator seems like the better option. I'll look into replacing all the instances of rmw_allocate/rmw_free.

[jacobperron]

So, it's not trivial to use the context's allocator for everything. In some places, though we have access to the context during initialization, we don't have a reference during cleanup. E.g. rmw_create_wait_set can use the context's allocator, but we don't have a reference to it in rmw_destroy_wait_set:

https://github.com/ros2/rmw_cyclonedds/blob/cbecd0e10734ed897a0447f14211585c96b9788a/rmw_cyclonedds_cpp/src/rmw_node.cpp#L3501

https://github.com/ros2/rmw_cyclonedds/blob/cbecd0e10734ed897a0447f14211585c96b9788a/rmw_cyclonedds_cpp/src/rmw_node.cpp#L3561

Here's a related issue upstream: https://github.com/ros2/rmw/issues/260

[eboasson]

@jacobperron given ros2/rmw#260 , it probably makes the most sense to do your fix first. @ivanpauno do you agree?

[ivanpauno]

@jacobperron given ros2/rmw#260 , it probably makes the most sense to do your fix first. @ivanpauno do you agree?

Yes, I think it makes more sense to first merge a fix here. ros2/rmw#260 is a larger task.

[eboasson]

CI looks good to me, I think all the failures are unrelated:

• Linux
• Linux aarch64
• Windows
• macOS

@ivanpauno or @hidmic agree? If so, I'll merge this PR.

[ivanpauno]

CI looks good to me, I think all the failures are unrelated:

Yes, they all look unrelated.

This one looks interesting though https://ci.ros2.org/job/ci_osx/13347/testReport/junit/projectroot.src.core.ddsi/tests/CUnit_ddsi_locator_from_string_ipv6_invalid/. I haven't seen it before, but maybe it's a flaky test in cyclonedds (?).

[eboasson]

This one looks interesting though https://ci.ros2.org/job/ci_osx/13347/testReport/junit/projectroot.src.core.ddsi/tests/CUnit_ddsi_locator_from_string_ipv6_invalid/. I haven't seen it before, but maybe it's a flaky test in cyclonedds (?).

Yes, that's weird. I have had some problems with those tests in the past because of platform differences in name resolution, but not for a long time. I sampled some of the nightly builds and I haven't seen it there, so I'm inclined to provisionally ascribe it to a name-lookup oddity.