Fix subtle concurrency bugs in ddsrt_init(), ddsrt_fini()

src/core/ddsc/src/dds_init.c

@@ -80,9 +80,10 @@ static bool cyclonedds_entity_ready (uint32_t s)
 
 dds_return_t dds_init (void)
 {
-  dds_return_t ret;
-
-  ddsrt_init ();
+  dds_return_t ret = ddsrt_init();
+  if( ret != DDS_RETCODE_OK ){
+    return ret;
+  }
   ddsrt_mutex_t * const init_mutex = ddsrt_get_singleton_mutex ();
   ddsrt_cond_t * const init_cond = ddsrt_get_singleton_cond ();
 

src/core/ddsc/tests/config.c

@@ -25,6 +25,11 @@
 
 #define FORCE_ENV
 
+static void ddsrt_init_noret(void)
+{
+  (void)ddsrt_init();
+}
+
 static void config__check_env (const char *env_variable, const char *expected_value)
 {
   const char *env_uri = NULL;
@@ -52,7 +57,7 @@ static void config__check_env (const char *env_variable, const char *expected_va
 #endif /* FORCE_ENV */
 }
 
-CU_Test (ddsc_config, simple_udp, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test (ddsc_config, simple_udp, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t participant;
   config__check_env ("CYCLONEDDS_URI", CONFIG_ENV_SIMPLE_UDP);
@@ -62,7 +67,7 @@ CU_Test (ddsc_config, simple_udp, .init = ddsrt_init, .fini = ddsrt_fini)
   dds_delete (participant);
 }
 
-CU_Test (ddsc_config, user_config, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test (ddsc_config, user_config, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain;
   domain = dds_create_domain (1,
@@ -83,7 +88,7 @@ CU_Test (ddsc_config, user_config, .init = ddsrt_init, .fini = ddsrt_fini)
   dds_delete (domain);
 }
 
-CU_Test (ddsc_config, ignoredpartition, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test (ddsc_config, ignoredpartition, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
 #ifndef DDS_HAS_NETWORK_PARTITIONS
   CU_PASS("no network partitions in build");
@@ -273,7 +278,7 @@ static void logger(void *ptr, const dds_log_data_t *data)
   }
 }
 
-CU_Test(ddsc_security_config, empty, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddsc_security_config, empty, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   /* Expected traces when creating participant with an empty security element.  We need to
      test this one here to be sure that it refuses to start when security is configured
@@ -313,7 +318,7 @@ CU_Test(ddsc_security_config, empty, .init = ddsrt_init, .fini = ddsrt_fini)
 #endif
 }
 
-CU_Test(ddsc_security_qos, empty, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddsc_security_qos, empty, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   /* Expected traces when creating participant with some (not all) security QoS
      settings.  We need to test this one here to be sure that it also refuses to
@@ -353,7 +358,7 @@ CU_Test(ddsc_security_qos, empty, .init = ddsrt_init, .fini = ddsrt_fini)
 #endif
 }
 
-CU_Test(ddsc_config, invalid_envvar, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddsc_config, invalid_envvar, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   const char *log_expected[] = {
     "*invalid expansion*",
@@ -379,7 +384,7 @@ CU_Test(ddsc_config, invalid_envvar, .init = ddsrt_init, .fini = ddsrt_fini)
   dds_set_trace_sink (NULL, NULL);
 }
 
-CU_Test(ddsc_config, too_deep_nesting, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddsc_config, too_deep_nesting, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   const char *log_expected[] = {
     "*too deeply nested*",
@@ -399,7 +404,7 @@ CU_Test(ddsc_config, too_deep_nesting, .init = ddsrt_init, .fini = ddsrt_fini)
   dds_set_trace_sink (NULL, NULL);
 }
 
-CU_Test(ddsc_config, multiple_domains, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddsc_config, multiple_domains, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   static const char *config = "\
 <CycloneDDS>\

src/ddsrt/include/dds/ddsrt/cdtors.h

@@ -31,15 +31,20 @@ extern "C" {
  * 
  * This doesn't need to be done more than once per process. It keeps track of the number of invocations,
  * and if the current call isn't the first one, it just waits for initializing (by the first one) to finish.
- * Note calling @ref ddsrt_init in parallel to the last invocation of @ref ddsrt_fini results in undefined behavior.
+ * It is safe to call @ref ddsrt_init() concurrently with itself and @ref ddsrt_fini().
+ * 
+ * @return DDS_RETCODE_OUT_OF_RESOURCES on failure (refcount saturated), otherwise DDS_RETCODE_OK
  */
-void ddsrt_init(void);
+dds_return_t ddsrt_init(void);
 
 /**
  * @brief Finalize ddsrt units
  * 
  * Since @ref ddsrt_init increments the reference count, it is decremented here. Only the last invocation
  * (when the reference count is 1) actually finalizes it.
+ * It is safe to call @ref ddsrt_fini() concurrently with itself and @ref ddsrt_init().
+ * If one or more threads call @ref ddsrt_init() whilst the last call to @ref ddsrt_fini()
+ * had already decided to actually finalize it, it will detect this condition and simply re-initialize.
  */
 void ddsrt_fini(void);
 

src/ddsrt/src/cdtors.c

@@ -45,46 +45,46 @@ static void ddsrt_init_impl (void)
 
 static void ddsrt_fini_impl (void)
 {
-  ddsrt_cond_destroy (&init_cond);
-  ddsrt_mutex_destroy (&init_mutex);
-  ddsrt_random_fini ();
   ddsrt_atomics_fini ();
+  ddsrt_random_fini ();
 #if _WIN32
-  ddsrt_winsock_fini ();
   ddsrt_time_fini ();
+  ddsrt_winsock_fini ();
 #endif
+  ddsrt_cond_destroy (&init_cond);
+  ddsrt_mutex_destroy (&init_mutex);
 }
 
-void ddsrt_init (void)
+dds_return_t ddsrt_init (void)
 {
 #if defined __GNUC__ && __GNUC__ >= 14
   DDSRT_WARNING_GNUC_OFF(analyzer-infinite-loop)
 #endif
   uint32_t v, v1;
-retry_init:
   do {
     v = ddsrt_atomic_ld32 (&init_status);
     // We get here for each application thread that needs at some point
     // to play with ddsi_thread states, and the corresponding call to
     // ddsrt_fini is dependent on the platform actually doing thread
-    // cleanup.  It is not safe to assume it will always work, so best
-    // to saturate the counter.
+    // cleanup. If the counter is saturated, we can't guarantee correctness,
+    // so it is best to return with an error.
     if ((v & INIT_COUNT_MASK) == INIT_COUNT_MASK)
-      v1 = v;
+      return DDS_RETCODE_OUT_OF_RESOURCES;
     else
       v1 = v + 1;
   } while (!ddsrt_atomic_cas32 (&init_status, v, v1));
 
-  if (v & INIT_STATUS_OK)
-    return;
+  if (v & INIT_STATUS_OK) {
+    ; // Proceed
+  }
   else if (v == 0)
   {
     ddsrt_init_impl ();
     ddsrt_atomic_or32 (&init_status, INIT_STATUS_OK);
   }
   else
   {
-    while (v != 0 && !(v & INIT_STATUS_OK))
+    while ((ddsrt_atomic_ld32(&init_status) & INIT_STATUS_OK) == 0)
     {
 #ifndef __COVERITY__
       /* This sleep makes Coverity warn about possibly sleeping while holding in a lock
@@ -93,13 +93,12 @@ void ddsrt_init (void)
          skip the sleep when being analyzed. */
       dds_sleepfor (10000000);
 #endif
-      v = ddsrt_atomic_ld32 (&init_status);
     }
-    goto retry_init;
   }
 #if defined __GNUC__ && __GNUC__ >= 14
   DDSRT_WARNING_GNUC_ON(analyzer-infinite-loop)
 #endif
+  return DDS_RETCODE_OK;
 }
 
 void ddsrt_fini (void)
@@ -108,12 +107,7 @@ void ddsrt_fini (void)
   do {
     v = ddsrt_atomic_ld32 (&init_status);
     assert ((v & INIT_STATUS_OK) && (v & INIT_COUNT_MASK) > 0);
-    if ((v & INIT_COUNT_MASK) == INIT_COUNT_MASK)
-    {
-      // saturated counter, don't try to do this little bit of cleanup
-      v1 = v;
-    }
-    else if ((v & INIT_COUNT_MASK) > 1)
+    if ((v & INIT_COUNT_MASK) > 1)
     {
       // "init once" or other "counted" ones remain
       v1 = v - 1;
@@ -128,7 +122,17 @@ void ddsrt_fini (void)
   if (v1 == 1)
   {
     ddsrt_fini_impl ();
-    ddsrt_atomic_dec32 (&init_status);
+    if( ddsrt_atomic_dec32_ov(&init_status) > 1 ){
+      /*
+      We're in a subtle edgecase where one or more threads incremented the refcount from ddsrt_init(),
+      at a time after the point where I came out of the while loop, but before my final decrement.
+      Any such threads will now be waiting for the init to happen,
+      but none of them will actually do it since none of them got (v == 0).
+      Undo the damage by re-initializing and then unblocking them so they can proceed.
+      */
+      ddsrt_init_impl();
+      ddsrt_atomic_or32(&init_status, INIT_STATUS_OK);
+    }
   }
 }
 

src/security/core/tests/config.c

@@ -122,10 +122,15 @@ static void reset_logger(void)
   }
 }
 
+static void ddsrt_init_noret(void)
+{
+  (void)ddsrt_init();
+}
+
 /* Expected traces when creating domain with an empty security element.  We need to
    test this one here to be sure that it refuses to start when security is configured
    but the implementation doesn't include support for it. */
-CU_Test(ddssec_config, empty, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, empty, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain;
   const char *log_expected[] = {
@@ -155,7 +160,7 @@ CU_Test(ddssec_config, empty, .init = ddsrt_init, .fini = ddsrt_fini)
 
 /* Create domain without security element, there shouldn't
    be traces that mention security. */
-CU_Test(ddssec_config, non, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, non, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain;
   const char *log_expected[] = {
@@ -174,7 +179,7 @@ CU_Test(ddssec_config, non, .init = ddsrt_init, .fini = ddsrt_fini)
 }
 
 /* Expected traces when creating domain with the security elements. */
-CU_Test(ddssec_config, missing, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, missing, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain;
   const char *log_expected[] = {
@@ -211,7 +216,7 @@ CU_Test(ddssec_config, missing, .init = ddsrt_init, .fini = ddsrt_fini)
 }
 
 /* Expected traces when creating domain with the security elements. */
-CU_Test(ddssec_config, all, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, all, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain, participant;
   const char *log_expected[] = {
@@ -298,7 +303,7 @@ CU_Test(ddssec_config, all, .init = ddsrt_init, .fini = ddsrt_fini)
 }
 
 /* Expected traces when creating participant with the security elements. */
-CU_Test(ddssec_config, security, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, security, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain, participant;
   const char *log_expected[] = {
@@ -380,7 +385,7 @@ CU_Test(ddssec_config, security, .init = ddsrt_init, .fini = ddsrt_fini)
 }
 
 /* Expected traces when creating domain with the security elements. */
-CU_Test(ddssec_config, deprecated, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, deprecated, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain, participant;
   const char *log_expected[] = {
@@ -466,7 +471,7 @@ CU_Test(ddssec_config, deprecated, .init = ddsrt_init, .fini = ddsrt_fini)
 }
 
 /* Expected traces when creating participant with the security elements. */
-CU_Test(ddssec_config, qos, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, qos, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain, participant;
   dds_qos_t * qos;
@@ -512,7 +517,7 @@ CU_Test(ddssec_config, qos, .init = ddsrt_init, .fini = ddsrt_fini)
 }
 
 /* Expected traces when creating participant with the security elements. */
-CU_Test(ddssec_config, qos_props, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, qos_props, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain, participant;
   dds_qos_t * qos;
@@ -563,7 +568,7 @@ CU_Test(ddssec_config, qos_props, .init = ddsrt_init, .fini = ddsrt_fini)
 }
 
 /* Expect qos settings used when creating participant with config security elements and qos. */
-CU_Test(ddssec_config, config_qos, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, config_qos, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain, participant;
   dds_qos_t * qos;
@@ -631,7 +636,7 @@ CU_Test(ddssec_config, config_qos, .init = ddsrt_init, .fini = ddsrt_fini)
 
 /* Expect config used when creating participant with config security elements and
    qos containing only non-security properties. */
-CU_Test(ddssec_config, other_prop, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, other_prop, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain, participant;
   dds_qos_t * qos;
@@ -687,7 +692,7 @@ CU_Test(ddssec_config, other_prop, .init = ddsrt_init, .fini = ddsrt_fini)
 }
 
 /* Expected traces when creating participant with the security elements. */
-CU_Test(ddssec_config, qos_invalid, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, qos_invalid, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain, participant;
   dds_qos_t * qos;
@@ -751,7 +756,7 @@ CU_Test(ddssec_config, qos_invalid, .init = ddsrt_init, .fini = ddsrt_fini)
 }
 
 /* Expected traces when creating participant with the security elements. */
-CU_Test(ddssec_config, qos_invalid_proprietary, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, qos_invalid_proprietary, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain, participant;
   dds_qos_t * qos;
@@ -815,7 +820,7 @@ CU_Test(ddssec_config, qos_invalid_proprietary, .init = ddsrt_init, .fini = ddsr
 }
 
 /* Expect qos settings used when creating participant with config security elements and qos. */
-CU_Test(ddssec_config, config_qos_missing_crl, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, config_qos_missing_crl, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain, participant;
   dds_qos_t * qos;
@@ -877,7 +882,7 @@ CU_Test(ddssec_config, config_qos_missing_crl, .init = ddsrt_init, .fini = ddsrt
 }
 
 /* Expected traces when creating participant overriding security settings from QoS. */
-CU_Test(ddssec_config, config_qos_override_crl, .init = ddsrt_init, .fini = ddsrt_fini)
+CU_Test(ddssec_config, config_qos_override_crl, .init = ddsrt_init_noret, .fini = ddsrt_fini)
 {
   dds_entity_t domain, participant;
   dds_qos_t * qos;