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.

Sign up for GitHub

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

Jump to bottom

Avoid memory leaks and undefined behavior in rmw_fastrtps_dynamic_cpp typesupport code #429

Merged
merged 7 commits into from
Sep 9, 2020
Merged

Avoid memory leaks and undefined behavior in rmw_fastrtps_dynamic_cpp typesupport code #429

merged 7 commits into from
Sep 9, 2020

Conversation

MiguelCompany
Copy link
Collaborator

@MiguelCompany MiguelCompany commented Sep 7, 2020 •
edited
Loading

This fixes #428 by using a similar approach to ros2/rmw_cyclonedds#228

It also addresses #219

@MiguelCompany MiguelCompany marked this pull request as draft September 7, 2020 14:34
@MiguelCompany MiguelCompany changed the title Use correct functions to resize and get an item, avoiding memory leaks in typesupport code Avoid memory leaks and undefined behavior in rmw_fastrtps_dynamic_cpp typesupport code Sep 8, 2020
@MiguelCompany MiguelCompany marked this pull request as ready for review September 8, 2020 07:26
@MiguelCompany MiguelCompany mentioned this pull request Sep 8, 2020
Closed
@ivanpauno ivanpauno self-requested a review September 8, 2020 21:30
@ivanpauno ivanpauno added the bug Something isn't working label Sep 8, 2020
ivanpauno
ivanpauno approved these changes Sep 8, 2020
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix @MiguelCompany !!

I have left a few question, but they aren't quite important.

rmw_fastrtps_dynamic_cpp/include/rmw_fastrtps_dynamic_cpp/TypeSupport_impl.hpp Show resolved Hide resolved
rmw_fastrtps_dynamic_cpp/include/rmw_fastrtps_dynamic_cpp/TypeSupport_impl.hpp Show resolved Hide resolved
rmw_fastrtps_dynamic_cpp/include/rmw_fastrtps_dynamic_cpp/TypeSupport_impl.hpp Show resolved Hide resolved
@ivanpauno ivanpauno mentioned this pull request Sep 8, 2020
Closed
@ivanpauno
Copy link
Member

ivanpauno commented Sep 8, 2020 •
edited
Loading

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@MiguelCompany
Copy link
Collaborator Author

@ivanpauno I think builds have failed due to #382

@ivanpauno
Copy link
Member

@ivanpauno I think builds have failed due to #382

Yes, I always forget that.

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

rcl failures seem to be a flake because of another node running in the system.
Retesting that:

  • macOS Build Status

@ivanpauno
Copy link
Member

Going in, thanks for the fix @MiguelCompany !!

@ivanpauno ivanpauno merged commit 656f2f0 into ros2:master Sep 9, 2020
@ivanpauno ivanpauno mentioned this pull request Sep 24, 2020
Merged
@jacobperron
Copy link
Member

This is on the Foxy patch release board, though I'd rather not backport API changes to publicly accessible headers.

@ivanpauno Can you take a look to see if this can be backported in without the changes to API?

@ivanpauno
Copy link
Member

This is on the Foxy patch release board, though I'd rather not backport API changes to publicly accessible headers.

Though Typesupport.hpp and Typesupport_impl.hpp are public headers, nobody uses them.
I don't think this can be fixed without the API changes, IMO it's worth backporting (it's fixing several leaks + potential undefined behavior).

@nuclearsandwich
Copy link
Member

Bumping this to a future Dashing patch release pending an agreement on the backport discussion above. Dashing will follow Foxy's lead.

@MiguelCompany MiguelCompany deleted the bugfix/428 branch April 6, 2021 07:41
nuclearsandwich pushed a commit that referenced this pull request May 18, 2021
@MiguelCompany @nuclearsandwich
Avoid memory leaks and undefined behavior in rmw_fastrtps_dynamic_cpp…
07b9ed5 
… typesupport code (#429)

Signed-off-by: Miguel Company <[email protected]>
Signed-off-by: Steven! Ragnarök <[email protected]>
@nuclearsandwich nuclearsandwich mentioned this pull request May 18, 2021
Closed
jacobperron pushed a commit that referenced this pull request Sep 29, 2021
@MiguelCompany @jacobperron
Avoid memory leaks and undefined behavior in rmw_fastrtps_dynamic_cpp…
80dfb90 
… typesupport code (#429)

Signed-off-by: Miguel Company <[email protected]>
@jacobperron jacobperron mentioned this pull request Jan 31, 2022
Merged
jacobperron added a commit that referenced this pull request Feb 3, 2022
@jacobperron @MiguelCompany
Avoid memory leaks and undefined behavior in rmw_fastrtps_dynamic_cpp…
2a69277 
… typesupport code (#429) (#577)

Signed-off-by: Miguel Company <[email protected]>

Co-authored-by: Miguel Company <[email protected]>
timonegk pushed a commit to timonegk/ros-rolling-rmw-fastrtps-dynamic-cpp-release that referenced this pull request May 21, 2022
@hidmic @timonegk
Import Debian changes 6.2.1-1jammy
3438ed9 
ros-rolling-rmw-fastrtps-dynamic-cpp (6.2.1-1jammy) jammy; urgency=high
.
  * Add content filter topic feature (#513 <ros2/rmw_fastrtps#513>)
  * Add sequence numbers to message info structure (#587 <ros2/rmw_fastrtps#587>)
  * Contributors: Chen Lihui, Ivan Santiago Paunovic
.
ros-rolling-rmw-fastrtps-dynamic-cpp (6.2.0-1jammy) jammy; urgency=high
.
  * Add EventsExecutor (#468 <ros2/rmw_fastrtps#468>)
  * Install headers to include/${PROJECT_NAME} (#578 <ros2/rmw_fastrtps#578>)
  * Contributors: Shane Loretz, iRobot ROS
.
ros-rolling-rmw-fastrtps-dynamic-cpp (6.1.2-1jammy) jammy; urgency=high
.
.
.
ros-rolling-rmw-fastrtps-dynamic-cpp (6.1.1-1jammy) jammy; urgency=high
.
.
.
ros-rolling-rmw-fastrtps-dynamic-cpp (6.1.0-1jammy) jammy; urgency=high
.
  * Add client/service QoS getters. (#560 <ros2/rmw_fastrtps#560>)
  * Contributors: mauropasse
.
ros-rolling-rmw-fastrtps-dynamic-cpp (6.0.0-1jammy) jammy; urgency=high
.
.
.
ros-rolling-rmw-fastrtps-dynamic-cpp (5.2.2-1jammy) jammy; urgency=high
.
  * Correctly recalculate serialized size on bounded sequences. (#540 <ros2/rmw_fastrtps#540>)
  * Fix type size alignment. (#550 <ros2/rmw_fastrtps#550>)
  * Contributors: Miguel Company
.
ros-rolling-rmw-fastrtps-dynamic-cpp (5.2.1-1jammy) jammy; urgency=high
.
.
.
ros-rolling-rmw-fastrtps-dynamic-cpp (5.2.0-1jammy) jammy; urgency=high
.
  * Add rmw_publisher_wait_for_all_acked support. (#519 <ros2/rmw_fastrtps#519>)
  * Contributors: Barry Xu
.
ros-rolling-rmw-fastrtps-dynamic-cpp (5.1.0-1jammy) jammy; urgency=high
.
  * Loan messages implementation (#523 <ros2/rmw_fastrtps#523>)
    * Added is_plain_ attribute to base TypeSupport.
    * Added new methods to base TypeSupport.
    * Implementation of rmw_borrow_loaned_message.
    * Implementation of rmw_return_loaned_message_from_publisher.
    * Enable loan messages on publishers of plain types.
    * Implementation for taking loaned messages.
    * Enable loan messages on subscriptions of plain types.
  * Contributors: Miguel Company
.
ros-rolling-rmw-fastrtps-dynamic-cpp (5.0.0-1jammy) jammy; urgency=high
.
  * Refactor to use DDS standard API (#518 <ros2/rmw_fastrtps#518>)
  * Unique network flows (#502 <ros2/rmw_fastrtps#502>)
  * updating quality declaration links (re: ros2/docs.ros2.org#52 <ros2/docs.ros2.org#52>) (#520 <ros2/rmw_fastrtps#520>)
  * Contributors: Miguel Company, shonigmann
.
ros-rolling-rmw-fastrtps-dynamic-cpp (4.5.0-1jammy) jammy; urgency=high
.
.
.
ros-rolling-rmw-fastrtps-dynamic-cpp (4.4.0-1jammy) jammy; urgency=high
.
  * Add RMW function to check QoS compatibility (#511 <ros2/rmw_fastrtps#511>)
  * Capture cdr exceptions (#505 <ros2/rmw_fastrtps#505>)
  * Load profiles based on topic names in rmw_fastrtps_dynamic_cpp (#497 <ros2/rmw_fastrtps#497>)
  * Contributors: Eduardo Ponz Segrelles, Jacob Perron, Miguel Company
.
ros-rolling-rmw-fastrtps-dynamic-cpp (4.3.0-1jammy) jammy; urgency=high
.
  * Set rmw_dds_common::GraphCache callback after init succeeds. (#496 <ros2/rmw_fastrtps#496>)
  * Handle typesupport errors on fetch. (#495 <ros2/rmw_fastrtps#495>)
  * Contributors: Michel Hidalgo
.
ros-rolling-rmw-fastrtps-dynamic-cpp (4.2.0-1jammy) jammy; urgency=high
.
.
.
ros-rolling-rmw-fastrtps-dynamic-cpp (4.1.0-1jammy) jammy; urgency=high
.
  * Check for correct context shutdown (#486 <ros2/rmw_fastrtps#486>)
  * New environment variable to change easily the publication mode (#470 <ros2/rmw_fastrtps#470>)
  * Contributors: Ignacio Montesino Valle, José Luis Bueno López
.
ros-rolling-rmw-fastrtps-dynamic-cpp (4.0.0-1jammy) jammy; urgency=high
.
  * Discriminate when the Client has gone from when the Client has not completely matched (#467 <ros2/rmw_fastrtps#467>)
    * Workaround when the client is gone before server sends response
    * Change add to the map to listener callback
  * Update the package.xml files with the latest Open Robotics maintainers (#459 <ros2/rmw_fastrtps#459>)
  * Update Quality Declarations and READMEs (#455 <ros2/rmw_fastrtps#455>)
    * Add QL of external dependencies to rmw_fastrtps_dynamic_cpp QD
    * Add QD links for dependencies to rmw_fastrtps_dynamic_cpp QD
    * Provide external dependencies QD links
    * Add README to rmw_fastrtps_dynamic
    * Add QD for rmw_fastrtps_dynamic
  * Contributors: JLBuenoLopez-eProsima, Jaime Martin Losa, José Luis Bueno López, Michael Jeronimo
.
ros-rolling-rmw-fastrtps-dynamic-cpp (3.1.4-1jammy) jammy; urgency=high
.
  * Ensure rmw_destroy_node() completes despite run-time errors. (#458 <ros2/rmw_fastrtps#458>)
  * Contributors: Michel Hidalgo
.
ros-rolling-rmw-fastrtps-dynamic-cpp (3.1.3-1jammy) jammy; urgency=high
.
  * Return RMW_RET_UNSUPPORTED in rmw_get_serialized_message_size (#452 <ros2/rmw_fastrtps#452>)
  * Contributors: Alejandro Hernández Cordero
.
ros-rolling-rmw-fastrtps-dynamic-cpp (3.1.2-1jammy) jammy; urgency=high
.
  * Updated publisher/subscription allocation and wait set API return codes (#443 <ros2/rmw_fastrtps#443>)
  * Added rmw_logging tests (#442 <ros2/rmw_fastrtps#442>)
  * Contributors: Alejandro Hernández Cordero
.
ros-rolling-rmw-fastrtps-dynamic-cpp (3.1.1-1jammy) jammy; urgency=high
.
  * Fix array get_function semantics (#448 <ros2/rmw_fastrtps#448>)
  * Make service/client construction/destruction implementation compliant (#445 <ros2/rmw_fastrtps#445>)
  * Make sure type can be unregistered successfully (#437 <ros2/rmw_fastrtps#437>)
  * Contributors: Barry Xu, Ivan Santiago Paunovic, Michel Hidalgo
.
ros-rolling-rmw-fastrtps-dynamic-cpp (3.1.0-1jammy) jammy; urgency=high
.
  * Add tests for native entity getters. (#439 <ros2/rmw_fastrtps#439>)
  * Avoid deadlock if graph update fails. (#438 <ros2/rmw_fastrtps#438>)
  * Contributors: Michel Hidalgo
.
ros-rolling-rmw-fastrtps-dynamic-cpp (3.0.0-1jammy) jammy; urgency=high
.
  * Call Domain::removePublisher while failure occurs in create_publisher (#434 <ros2/rmw_fastrtps#434>)
  * Avoid memory leaks and undefined behavior in rmw_fastrtps_dynamic_cpp typesupport code (#429 <ros2/rmw_fastrtps#429>)
  * Contributors: Barry Xu, Miguel Company
.
ros-rolling-rmw-fastrtps-dynamic-cpp (2.6.0-1jammy) jammy; urgency=high
.
  * Ensure compliant matched pub/sub count API. (#424 <ros2/rmw_fastrtps#424>)
  * Ensure compliant publisher QoS queries. (#425 <ros2/rmw_fastrtps#425>)
  * Contributors: Michel Hidalgo
.
ros-rolling-rmw-fastrtps-dynamic-cpp (2.5.0-1jammy) jammy; urgency=high
.
.
.
ros-rolling-rmw-fastrtps-dynamic-cpp (2.4.0-1jammy) jammy; urgency=high
.
  * Ensure compliant subscription API. (#419 <ros2/rmw_fastrtps#419>)
  * Contributors: Michel Hidalgo
.
ros-rolling-rmw-fastrtps-dynamic-cpp (2.3.0-1jammy) jammy; urgency=high
.
  * Ensure compliant publisher API. (#414 <ros2/rmw_fastrtps#414>)
  * Contributors: Michel Hidalgo
.
ros-rolling-rmw-fastrtps-dynamic-cpp (2.2.0-1jammy) jammy; urgency=high
.
  * Set context actual domain id (#410 <ros2/rmw_fastrtps#410>)
  * Contributors: Ivan Santiago Paunovic
.
ros-rolling-rmw-fastrtps-dynamic-cpp (2.1.0-1jammy) jammy; urgency=high
.
  * Ensure compliant node construction/destruction API. (#408 <ros2/rmw_fastrtps#408>)
  * Contributors: Michel Hidalgo
.
ros-rolling-rmw-fastrtps-dynamic-cpp (2.0.0-1jammy) jammy; urgency=high
.
  * Remove domain_id and localhost_only from node API (#407 <ros2/rmw_fastrtps#407>)
  * Amend rmw_init() implementation: require enclave. (#406 <ros2/rmw_fastrtps#406>)
  * Contributors: Ivan Santiago Paunovic, Michel Hidalgo
.
ros-rolling-rmw-fastrtps-dynamic-cpp (1.1.0-1jammy) jammy; urgency=high
.
  * Ensure compliant init/shutdown API implementation. (#401 <ros2/rmw_fastrtps#401>)
  * Finalize context iff shutdown. (#396 <ros2/rmw_fastrtps#396>)
  * Make service wait for response reader (#390 <ros2/rmw_fastrtps#390>)
  * Contributors: Michel Hidalgo, Miguel Company
.
ros-rolling-rmw-fastrtps-dynamic-cpp (1.0.1-1jammy) jammy; urgency=high
.
.
.
ros-rolling-rmw-fastrtps-dynamic-cpp (1.0.0-1jammy) jammy; urgency=high
.
  * Fix single rmw build for rmw_fastrtps_dynamic_cpp (#381 <ros2/rmw_fastrtps#381>)
  * Remove API related to manual by node liveliness (#379 <ros2/rmw_fastrtps#379>)
  * Contributors: Ivan Santiago Paunovic
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.9.1-1jammy) jammy; urgency=high
.
  * Added doxyfiles (#372 <ros2/rmw_fastrtps#372>)
  * Contributors: Alejandro Hernández Cordero
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.9.0-1jammy) jammy; urgency=high
.
  * Fixed rmw_fastrtps_dynamic_cpp package description. (#376 <ros2/rmw_fastrtps#376>)
  * Rename rosidl_message_bounds_t. (#373 <ros2/rmw_fastrtps#373>)
  * Feature/services timestamps. (#369 <ros2/rmw_fastrtps#369>)
  * Add support for taking a sequence of messages. (#366 <ros2/rmw_fastrtps#366>)
  * security-context -> enclave. (#365 <ros2/rmw_fastrtps#365>)
  * Rename rosidl_generator_c namespace to rosidl_runtime_c. (#367 <ros2/rmw_fastrtps#367>)
  * Remove custom typesupport for rmw_dds_common interfaces. (#364 <ros2/rmw_fastrtps#364>)
  * Added rosidl_runtime c and cpp depencencies. (#351 <ros2/rmw_fastrtps#351>)
  * Switch to one Participant per Context. (#312 <ros2/rmw_fastrtps#312>)
  * Add rmw_*_event_init() functions. (#354 <ros2/rmw_fastrtps#354>)
  * Fixing type support C/CPP mix on rmw_fastrtps_dynamic_cpp. (#350 <ros2/rmw_fastrtps#350>)
  * Fix build warning in Ubuntu Focal. (#346 <ros2/rmw_fastrtps#346>)
  * Code style only: wrap after open parenthesis if not in one line. (#347 <ros2/rmw_fastrtps#347>)
  * Passing down type support information (#342 <ros2/rmw_fastrtps#342>)
  * Implement functions to get publisher and subcription informations like QoS policies from topic name. (#336 <ros2/rmw_fastrtps#336>)
  * Contributors: Alejandro Hernández Cordero, Dirk Thomas, Ingo Lütkebohle, Ivan Santiago Paunovic, Jaison Titus, Miaofei Mei, Michael Carroll, Miguel Company, Mikael Arguedas
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.8.1-1jammy) jammy; urgency=high
.
  * use return_loaned_message_from (#334 <ros2/rmw_fastrtps#334>)
  * Restrict traffic to localhost only if env var is provided (#331 <ros2/rmw_fastrtps#331>)
  * Zero copy api (#322 <ros2/rmw_fastrtps#322>)
  * update signature for added pub/sub options (#329 <ros2/rmw_fastrtps#329>)
  * Contributors: Brian Marchi, Karsten Knese, William Woodall
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.8.0-1jammy) jammy; urgency=high
.
  * Add function for getting clients by node (#293 <ros2/rmw_fastrtps#293>)
  * Use rcpputils::find_and_replace instead of std::regex_replace (#291 <ros2/rmw_fastrtps#291>)
  * Export typesupport_fastrtps package dependencies (#294 <ros2/rmw_fastrtps#294>)
  * Implement get_actual_qos() for subscriptions (#287 <ros2/rmw_fastrtps#287>)
  * Contributors: Jacob Perron, M. M, kurcha01-arm
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.7.3-1jammy) jammy; urgency=high
.
.
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.7.2-1jammy) jammy; urgency=high
.
  * add support for WString in rmw_fastrtps_dynamic_cpp (#278 <ros2/rmw_fastrtps#278>)
  * Centralize topic name creation logic and update to match FastRTPS 1.8 API (#272 <ros2/rmw_fastrtps#272>)
  * Contributors: Dirk Thomas, Nick Burek
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.7.1-1jammy) jammy; urgency=high
.
  * Support arbitrary message namespaces  (#266 <ros2/rmw_fastrtps#266>)
  * Add qos interfaces with no-op (#271 <ros2/rmw_fastrtps#271>)
  * Updates for preallocation API. (#274 <ros2/rmw_fastrtps#274>)
  * Contributors: Jacob Perron, Michael Carroll, Ross Desmond
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.7.0-1jammy) jammy; urgency=high
.
  * Add function to get publisher actual qos settings (#267 <ros2/rmw_fastrtps#267>)
  * pass context to wait set and fini context (#252 <ros2/rmw_fastrtps#252>)
  * Add missing logic to dynamic RMW client implementation (#254 <ros2/rmw_fastrtps#254>)
  * Merge pull request #250 <ros2/rmw_fastrtps#250> from ros2/support_static_lib
  * use namespace_prefix from shared package
  * Use empty() instead of size() to check if a vector/map contains elements and fixed some incorrect logging (#245 <ros2/rmw_fastrtps#245>)
  * Contributors: Dirk Thomas, Jacob Perron, Johnny Willemsen, William Woodall, ivanpauno
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.6.1-1jammy) jammy; urgency=high
.
  * Add topic cache object for managing topic relations (#236 <ros2/rmw_fastrtps#236>)
  * Fastrtps 1.7.0 (#233 <ros2/rmw_fastrtps#233>)
  * RMW_FastRTPS configuration from XML only (#243 <ros2/rmw_fastrtps#243>)
  * refactor to support init options and context (#237 <ros2/rmw_fastrtps#237>)
  * Methods to retrieve matched counts on pub/sub (#234 <ros2/rmw_fastrtps#234>)
  * Fixing failing tests on rmw_fastrtps_dynamic_cpp. (#242 <ros2/rmw_fastrtps#242>)
  * use uint8_array (#240 <ros2/rmw_fastrtps#240>)
  * fix linter warnings (#241 <ros2/rmw_fastrtps#241>)
  * Contributors: Dirk Thomas, Juan Carlos, Karsten Knese, Michael Carroll, MiguelCompany, Ross Desmond, William Woodall
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.6.0-1jammy) jammy; urgency=high
.
  * Merge pull request #232 <ros2/rmw_fastrtps#232> from ros2/array-terminology
  * rename files
  * rename dynamic array to sequence
  * Add semicolons to all RCLCPP and RCUTILS macros. (#229 <ros2/rmw_fastrtps#229>)
  * Include node namespaces in get_node_names (#224 <ros2/rmw_fastrtps#224>)
  * add rmw_get_serialization_format (#215 <ros2/rmw_fastrtps#215>)
  * Merge pull request #218 <ros2/rmw_fastrtps#218> from ros2/pr203
  * Refs #3061 <https://github.com/ros2/rmw_fastrtps/issues/3061>. Adapting code on rmw_fastrtps_dynamic_cpp.
  * Refs #3061 <https://github.com/ros2/rmw_fastrtps/issues/3061>. Package rmw_fastrtps_cpp duplicated as rmw_fastrtps_dynamic_cpp.
  * Contributors: Chris Lalancette, Dirk Thomas, Karsten Knese, Michael Carroll, Miguel Company
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.5.1-1jammy) jammy; urgency=high
.
.
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.5.0-1jammy) jammy; urgency=high
.
.
.
ros-rolling-rmw-fastrtps-dynamic-cpp (0.4.0-1jammy) jammy; urgency=high
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivanpauno ivanpauno ivanpauno approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[rmw_fastrtps_dynamic_cpp] Typesupport implementation leaks and it relies in conceptually broken cpp
4 participants
@MiguelCompany @ivanpauno @jacobperron @nuclearsandwich