search

o3de / o3de-extras

Other
61 stars 63 forks source link

Invalid names can lead to a crash #413

Open Antoni-Robotec opened 1 year ago

Antoni-Robotec commented 1 year ago

Invalid names can lead to a crash. To replicate:

  1. Open a prefab with the JointsTrajectoryComponent
  2. Rename the action name so that it contains consecutive '/'
  3. Run the simulation terminate called after throwing an instance of 'rclcpp::exceptions::RCLError' what(): topic name must not contain repeated '/', at ./src/rcl/node_resolve_name.c:102 Aborted (core dumped) A similar error occurs when assigning a name with consecutive '/' to a topic. We are probably missing some try-catch statements.
adamdbrw commented 1 year ago

@Antoni-Robotec it would be good to use name validation / rosify tools which are already available in the Gem

adamdbrw commented 1 year ago

Linked to #414

Antoni-Robotec commented 1 year ago

I looked into it a little and the validation tools don't perform a full check. https://docs.ros2.org/dashing/api/rcl/validate__topic__name_8h.html A full validation can be done with: https://docs.ros2.org/dashing/api/rmw/validate__full__topic__name_8h.html However, it first requires adding the namespace (I think it's equivalent if we don't add it and validate the namespace separately?) and expanding the name (for example, currently we allow for stuff like "{node}" to be in the topic name, which when creating the server is expanded to the name of the node we are publishing from): https://docs.ros2.org/dashing/api/rcl/expand__topic__name_8h.html I think what needs to be done is deciding whether performing this in the field validation is feasible and either doing so, or adding try catch statements around server creation.

michalpelka commented 1 year ago

As discussed in issue triage, the issue is a case of issue https://github.com/o3de/o3de-extras/issues/414. It is being implemented. Raised priority from minor to major.