From 78c255d432baf466a9fea3aef0e909b339e964d7 Mon Sep 17 00:00:00 2001 From: Erik Boasson Date: Fri, 30 Aug 2019 15:27:05 +0200 Subject: [PATCH] Explicitly check CLOSED flag when creating entity All functions creating an entity pin the parent, which guarantees that the CLOSED is not set yet, but as setting it occurs within the mutex, it may be set by the time the parent has been locked. One should not try to add a child entity at that point, so check it again. Signed-off-by: Erik Boasson --- src/core/ddsc/src/dds__entity.h | 26 +++++++++++++++++++++++--- src/core/ddsc/src/dds_entity.c | 14 ++++++++++++++ src/core/ddsc/src/dds_guardcond.c | 2 +- src/core/ddsc/src/dds_publisher.c | 2 +- src/core/ddsc/src/dds_querycond.c | 2 +- src/core/ddsc/src/dds_readcond.c | 2 +- src/core/ddsc/src/dds_reader.c | 6 +++--- src/core/ddsc/src/dds_subscriber.c | 2 +- src/core/ddsc/src/dds_topic.c | 4 ++-- src/core/ddsc/src/dds_waitset.c | 4 ++-- src/core/ddsc/src/dds_writer.c | 4 ++-- 11 files changed, 51 insertions(+), 17 deletions(-) diff --git a/src/core/ddsc/src/dds__entity.h b/src/core/ddsc/src/dds__entity.h index ff1ab0e..2928438 100644 --- a/src/core/ddsc/src/dds__entity.h +++ b/src/core/ddsc/src/dds__entity.h @@ -36,7 +36,7 @@ dds_entity_register_child ( DDS_EXPORT void dds_entity_add_ref_locked(dds_entity *e); -#define DEFINE_ENTITY_LOCK_UNLOCK(qualifier_, type_, kind_) \ +#define DEFINE_ENTITY_LOCK_UNLOCK_ONLY(qualifier_, type_, kind_) \ qualifier_ dds_return_t type_##_lock (dds_entity_t hdl, type_ **x) \ { \ dds_return_t rc; \ @@ -46,15 +46,29 @@ dds_entity_add_ref_locked(dds_entity *e); *x = (type_ *) e; \ return DDS_RETCODE_OK; \ } \ - \ qualifier_ void type_##_unlock (type_ *x) \ { \ dds_entity_unlock (&x->m_entity); \ } -#define DECL_ENTITY_LOCK_UNLOCK(qualifier_, type_) \ +#define DECL_ENTITY_LOCK_UNLOCK_ONLY(qualifier_, type_) \ qualifier_ dds_return_t type_##_lock (dds_entity_t hdl, type_ **x); \ qualifier_ void type_##_unlock (type_ *x); +#define DEFINE_ENTITY_LOCK_UNLOCK(qualifier_, type_, kind_) \ + DEFINE_ENTITY_LOCK_UNLOCK_ONLY (qualifier_, type_, kind_) \ + qualifier_ dds_return_t type_##_lock_for_create (dds_entity_t hdl, type_ **x) \ + { \ + dds_return_t rc; \ + dds_entity *e; \ + if ((rc = dds_entity_lock_for_create (hdl, kind_, &e)) < 0) \ + return rc; \ + *x = (type_ *) e; \ + return DDS_RETCODE_OK; \ + } +#define DECL_ENTITY_LOCK_UNLOCK(qualifier_, type_) \ + DECL_ENTITY_LOCK_UNLOCK_ONLY (qualifier_, type_) \ + qualifier_ dds_return_t type_##_lock_for_create (dds_entity_t hdl, type_ **x); + DDS_EXPORT inline dds_entity *dds_entity_from_handle_link (struct dds_handle_link *hdllink) { return (dds_entity *) ((char *) hdllink - offsetof (struct dds_entity, m_hdllink)); } @@ -96,6 +110,12 @@ dds_entity_lock( dds_entity_kind_t kind, dds_entity **e); +DDS_EXPORT dds_return_t +dds_entity_lock_for_create( + dds_entity_t hdl, + dds_entity_kind_t kind, + dds_entity **e); + DDS_EXPORT void dds_entity_unlock(dds_entity *e); diff --git a/src/core/ddsc/src/dds_entity.c b/src/core/ddsc/src/dds_entity.c index 86f578a..06c66d4 100644 --- a/src/core/ddsc/src/dds_entity.c +++ b/src/core/ddsc/src/dds_entity.c @@ -1099,6 +1099,20 @@ dds_return_t dds_entity_lock (dds_entity_t hdl, dds_entity_kind_t kind, dds_enti } } +dds_return_t dds_entity_lock_for_create (dds_entity_t hdl, dds_entity_kind_t kind, dds_entity **eptr) +{ + dds_return_t res; + if ((res = dds_entity_lock (hdl, kind, eptr)) < 0) + return res; + else if (!dds_handle_is_closed (&(*eptr)->m_hdllink)) + return DDS_RETCODE_OK; + else + { + ddsrt_mutex_unlock (&(*eptr)->m_mutex); + return DDS_RETCODE_BAD_PARAMETER; + } +} + void dds_entity_unlock (dds_entity *e) { ddsrt_mutex_unlock (&e->m_mutex); diff --git a/src/core/ddsc/src/dds_guardcond.c b/src/core/ddsc/src/dds_guardcond.c index 1ff1d52..d71e703 100644 --- a/src/core/ddsc/src/dds_guardcond.c +++ b/src/core/ddsc/src/dds_guardcond.c @@ -41,7 +41,7 @@ dds_entity_t dds_create_guardcondition (dds_entity_t owner) if ((rc = dds_init ()) < 0) return rc; - if ((rc = dds_entity_lock (owner, DDS_KIND_DONTCARE, &e)) != DDS_RETCODE_OK) + if ((rc = dds_entity_lock_for_create (owner, DDS_KIND_DONTCARE, &e)) != DDS_RETCODE_OK) goto err_entity_lock; switch (dds_entity_kind (e)) diff --git a/src/core/ddsc/src/dds_publisher.c b/src/core/ddsc/src/dds_publisher.c index b06306c..5236870 100644 --- a/src/core/ddsc/src/dds_publisher.c +++ b/src/core/ddsc/src/dds_publisher.c @@ -52,7 +52,7 @@ dds_entity_t dds_create_publisher (dds_entity_t participant, const dds_qos_t *qo dds_qos_t *new_qos; dds_return_t ret; - if ((ret = dds_participant_lock (participant, &par)) != DDS_RETCODE_OK) + if ((ret = dds_participant_lock_for_create (participant, &par)) != DDS_RETCODE_OK) return ret; new_qos = dds_create_qos (); diff --git a/src/core/ddsc/src/dds_querycond.c b/src/core/ddsc/src/dds_querycond.c index a589ce0..79e3c9c 100644 --- a/src/core/ddsc/src/dds_querycond.c +++ b/src/core/ddsc/src/dds_querycond.c @@ -26,7 +26,7 @@ dds_entity_t dds_create_querycondition (dds_entity_t reader, uint32_t mask, dds_ dds_return_t rc; dds_reader *r; - if ((rc = dds_reader_lock (reader, &r)) != DDS_RETCODE_OK) + if ((rc = dds_reader_lock_for_create (reader, &r)) != DDS_RETCODE_OK) return rc; else { diff --git a/src/core/ddsc/src/dds_readcond.c b/src/core/ddsc/src/dds_readcond.c index acfd1b8..e3b8f1b 100644 --- a/src/core/ddsc/src/dds_readcond.c +++ b/src/core/ddsc/src/dds_readcond.c @@ -64,7 +64,7 @@ dds_entity_t dds_create_readcondition (dds_entity_t reader, uint32_t mask) { dds_reader *rd; dds_return_t rc; - if ((rc = dds_reader_lock (reader, &rd)) != DDS_RETCODE_OK) + if ((rc = dds_reader_lock_for_create (reader, &rd)) != DDS_RETCODE_OK) return rc; else { diff --git a/src/core/ddsc/src/dds_reader.c b/src/core/ddsc/src/dds_reader.c index b18fb06..45d754f 100644 --- a/src/core/ddsc/src/dds_reader.c +++ b/src/core/ddsc/src/dds_reader.c @@ -323,7 +323,7 @@ dds_entity_t dds_create_reader (dds_entity_t participant_or_subscriber, dds_enti } } - if ((ret = dds_subscriber_lock (subscriber, &sub)) != DDS_RETCODE_OK) + if ((ret = dds_subscriber_lock_for_create (subscriber, &sub)) != DDS_RETCODE_OK) { reader = ret; goto err_sub_lock; @@ -335,7 +335,7 @@ dds_entity_t dds_create_reader (dds_entity_t participant_or_subscriber, dds_enti sub->m_entity.m_flags |= DDS_ENTITY_IMPLICIT; } - if ((ret = dds_topic_lock (t, &tp)) != DDS_RETCODE_OK) + if ((ret = dds_topic_lock_for_create (t, &tp)) != DDS_RETCODE_OK) { reader = ret; goto err_tp_lock; @@ -397,7 +397,7 @@ dds_entity_t dds_create_reader (dds_entity_t participant_or_subscriber, dds_enti ddsrt_mutex_lock (&tp->m_entity.m_mutex); assert (ret == DDS_RETCODE_OK); /* FIXME: can be out-of-resources at the very least */ thread_state_asleep (lookup_thread_state ()); - + rd->m_entity.m_iid = get_entity_instance_id (&rd->m_entity.m_domain->gv, &rd->m_entity.m_guid); dds_entity_register_child (&sub->m_entity, &rd->m_entity); diff --git a/src/core/ddsc/src/dds_subscriber.c b/src/core/ddsc/src/dds_subscriber.c index 2c3f6ae..362a375 100644 --- a/src/core/ddsc/src/dds_subscriber.c +++ b/src/core/ddsc/src/dds_subscriber.c @@ -74,7 +74,7 @@ dds_entity_t dds_create_subscriber (dds_entity_t participant, const dds_qos_t *q dds_participant *par; dds_entity_t hdl; dds_return_t ret; - if ((ret = dds_participant_lock (participant, &par)) != DDS_RETCODE_OK) + if ((ret = dds_participant_lock_for_create (participant, &par)) != DDS_RETCODE_OK) return ret; hdl = dds__create_subscriber_l (par, qos, listener); dds_participant_unlock (par); diff --git a/src/core/ddsc/src/dds_topic.c b/src/core/ddsc/src/dds_topic.c index c7c15f9..1cfca94 100644 --- a/src/core/ddsc/src/dds_topic.c +++ b/src/core/ddsc/src/dds_topic.c @@ -324,7 +324,7 @@ dds_entity_t dds_create_topic_arbitrary (dds_entity_t participant, struct ddsi_s /* FIXME: just mutex_lock ought to be good enough, but there is the pesky "closed" check still ... */ - if ((rc = dds_participant_lock (participant, &par)) != DDS_RETCODE_OK) + if ((rc = dds_participant_lock_for_create (participant, &par)) != DDS_RETCODE_OK) goto err_lock_participant; bool retry_lookup; @@ -384,7 +384,7 @@ dds_entity_t dds_create_topic_arbitrary (dds_entity_t participant, struct ddsi_s abort (); } - if ((rc = dds_participant_lock (participant, &par)) != DDS_RETCODE_OK) + if ((rc = dds_participant_lock_for_create (participant, &par)) != DDS_RETCODE_OK) goto err_lock_participant; } } while (retry_lookup); diff --git a/src/core/ddsc/src/dds_waitset.c b/src/core/ddsc/src/dds_waitset.c index 5dcd0bd..6e50c16 100644 --- a/src/core/ddsc/src/dds_waitset.c +++ b/src/core/ddsc/src/dds_waitset.c @@ -21,7 +21,7 @@ #include "dds__rhc.h" #include "dds/ddsi/ddsi_iid.h" -DEFINE_ENTITY_LOCK_UNLOCK (static, dds_waitset, DDS_KIND_WAITSET) +DEFINE_ENTITY_LOCK_UNLOCK_ONLY (static, dds_waitset, DDS_KIND_WAITSET) static bool is_triggered (struct dds_entity *e) { @@ -125,7 +125,7 @@ dds_entity_t dds_create_waitset (dds_entity_t owner) if ((rc = dds_init ()) < 0) return rc; - if ((rc = dds_entity_lock (owner, DDS_KIND_DONTCARE, &e)) != DDS_RETCODE_OK) + if ((rc = dds_entity_lock_for_create (owner, DDS_KIND_DONTCARE, &e)) != DDS_RETCODE_OK) goto err_entity_lock; switch (dds_entity_kind (e)) diff --git a/src/core/ddsc/src/dds_writer.c b/src/core/ddsc/src/dds_writer.c index 7dc4579..0be56b5 100644 --- a/src/core/ddsc/src/dds_writer.c +++ b/src/core/ddsc/src/dds_writer.c @@ -261,14 +261,14 @@ dds_entity_t dds_create_writer (dds_entity_t participant_or_publisher, dds_entit dds_entity_unpin (p_or_p); } - if ((rc = dds_publisher_lock (publisher, &pub)) != DDS_RETCODE_OK) + if ((rc = dds_publisher_lock_for_create (publisher, &pub)) != DDS_RETCODE_OK) return rc; ddsi_tran_conn_t conn = pub->m_entity.m_domain->gv.data_conn_uc; if (publisher != participant_or_publisher) pub->m_entity.m_flags |= DDS_ENTITY_IMPLICIT; - if ((rc = dds_topic_lock (topic, &tp)) != DDS_RETCODE_OK) + if ((rc = dds_topic_lock_for_create (topic, &tp)) != DDS_RETCODE_OK) goto err_tp_lock; assert (tp->m_stopic);