Segmentation fault on `dds_delete_topic_descriptor`

[paulsohn]

I'm not actually sure if this is a DDS issue or an RMW issue, but dds_delete_topic_descriptor called from RMW dynamic_type_register occasionally gives a segfault. This occurs on node initialization phase.

glog stack trace:

*** Aborted at 1762942890 (unix time) try "date -d @1762942890" if you are using GNU date ***
PC: @                0x0 (unknown)
*** SIGSEGV (@0x0) received by PID 5866 (TID 0x7f13f4736680) from PID 0; stack trace: ***
    @     0x7f13f4fcc4d6 google::(anonymous namespace)::FailureSignalHandler()
    @     0x7f13f4442520 (unknown)
    @     0x7f13f433a65d ddsi_topic_descriptor_fini
    @     0x7f13f435c912 dds_delete_topic_descriptor
    @     0x7f13f46ebdfa dynamic_type_register()
    @     0x7f13f46f2118 create_msg_dds_dynamic_type()
    @     0x7f13f46c8166 create_cdds_subscription()
    @     0x7f13f46c8518 create_subscription()
    @     0x7f13f46caa20 rmw_create_subscription
    @     0x7f13f4be7b70 rcl_subscription_init
    @     0x7f13f4e1db36 rclcpp::SubscriptionBase::SubscriptionBase()
    @     0x7f13f3330ba2 rclcpp::Subscription<>::Subscription()
    @     0x7f13f3331dac _ZNSt17_Function_handlerIFSt10shared_ptrIN6rclcpp16SubscriptionBaseEEPNS1_15node_interfaces17NodeBaseInterfaceERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKNS1_3QoSEEZNS1_27create_subscription_factoryIN21tier4_perception_msgs3msg27DetectedObjectsWithFeature_ISaIvEEEZN8autoware29image_projection_based_fusion10FusionNodeIN24autoware_perception_msgs3msg16DetectedObjects_ISN_EESO_SV_EC4ESE_RKNS1_11NodeOptionsEEUlS0_IKSO_EE0_SN_NS1_12SubscriptionISO_SN_SO_SO_NS1_23message_memory_strategy21MessageMemoryStrategyISO_SN_EEEES16_SO_EENS1_19SubscriptionFactoryEOT0_RKNS1_32SubscriptionOptionsWithAllocatorIT1_EENT3_9SharedPtrES0_INS1_16topic_statistics27SubscriptionTopicStatisticsIT4_EEEEUlS6_SE_SH_E_E9_M_invokeERKSt9_Any_dataOS6_SE_SH_
    @     0x7f13f4db949c rclcpp::node_interfaces::NodeTopics::create_subscription()
(...)

My working environment is a bit complex;

• CycloneDDS version is d11bbd1 , really close to the current master
• For RMW side, I cherry-picked Support Cyclone DDS v0.10.x and master + ROS 2 type definitions in DDS ros2/rmw_cyclonedds#501 onto the humble branch so that I can use the latest master.

Is there any possibility that deleting a topic descriptor suffers from some race condition?
At a glimpse it seems like finding a topic and deleting a topic descriptor has raced, and the ddsi_topic_descriptor_fini function lacks a locking mechanism.

[eboasson]

Hi @paulsohn, it looks to me like I wasn't thinking straight when I wrote that code (slightly shortened here):

static bool dynamic_type_register(struct sertype_rmw * st, dds_dynamic_type_t dt, dds_entity_t dds_ppant)
{
  dds_typeinfo_t *type_info;
  auto rc = dds_dynamic_type_register(&dt, &type_info);
  if (rc != DDS_RETCODE_OK)
    goto fail_typeinfo;
  dds_topic_descriptor_t *desc;
  rc = dds_create_topic_descriptor(
    DDS_FIND_SCOPE_GLOBAL, dds_ppant, type_info, 0, &desc);
  if (rc != DDS_RETCODE_OK)
   goto fail_descriptor;
...
  dds_dynamic_type_unref(&dt);
fail_descriptor:
  dds_delete_topic_descriptor(desc);
fail_typeinfo:
  dds_free_typeinfo(type_info);
}

So if dds_create_topic_descriptor fails it calls dds_delete_topic_descriptor with a garbage argument. Same for type_info and just to make things worse still, it fails to call dds_dynamic_type_unref on error, too ...

I can't quickly build ROS 2 (macOS is not supported any more by ROS 2 and it takes me some time to get a build going again ...), but perhaps you can try the following:

  dds_delete_topic_descriptor(desc);
fail_descriptor:
  dds_free_typeinfo(type_info);
fail_typeinfo:
  dds_dynamic_type_unref(&dt);
}

I think there's a good chance that modification will fix the crash. If so, it would also be interesting to know on what type it fails and why. It seems unreasonable to expect you to figure out the why, but perhaps you can find out which topic/type it concerns.

Note that it is meant to continue functioning even if it can't created the topic descriptor, it only does that because it is the easiest way to get hold of the DDS XTypes metadata for the type. If it doesn't have that metadata, it'll still work fine, you just won't be able to do inspect the type or subscribe to the data using the Cyclone tooling.

[paulsohn]

@eboasson Thank you for comment! It makes sense that RMW side might invoked garbage-freeing because of the flawed goto position.

At the moment I am just an integrator and know very little about the implementation, or the DDS specification itself, although I just started to learn. Here are some questions on top of my head:

1. What is the intended behavior if dynamic_type_register() fails? The previous implementation crashed the process, but obviously it wouldn't be something expected. If type registration fails, then does it retry until it succeeds, just give up the registration silently, or lead to the entire node failure?
2. Are we sure that we want to call dds_dynamic_type_unref(&dt), even for fail_descriptor and fail_typeinfo cases?
3. Is there any particular reason that we are using goto syntax here? For example, the equivalent logic without goto is possible:

static bool dynamic_type_register(struct sertype_rmw * st, dds_dynamic_type_t dt, dds_entity_t dds_ppant)
{
  dds_typeinfo_t *type_info;
  auto rc = dds_dynamic_type_register(&dt, &type_info);
  if (rc == DDS_RETCODE_OK) {
    dds_topic_descriptor_t *desc;
    rc = dds_create_topic_descriptor(
        DDS_FIND_SCOPE_GLOBAL, dds_ppant, type_info, 0, &desc);
    if (rc == DDS_RETCODE_OK) {
      // ...
      dds_delete_topic_descriptor(desc);
    }
    dds_free_typeinfo(type_info);
  }
  dds_dynamic_type_unref(&dt);
}

As goto is used in other places as well, I assume that you are aware of the goto-free convention but using it nevertheless, mainly for preventing deep if nesting. But in such case, I would suggest more comprehensive foolproof notation, e.g.

// unless fail_descriptor
  dds_delete_topic_descriptor(desc);
fail_descriptor:
// unless fail_typeinfo
  dds_free_typeinfo(type_info);
fail_typeinfo:
// unless false
  dds_dynamic_type_unref(&dt);
}

Still, this is a bit more like an aesthetics though, and I would prefer goto-free whenever it's possible.