From 7e6039763baef71ad0ca741b045ada3f111968e9 Mon Sep 17 00:00:00 2001 From: Dennis Potman Date: Tue, 21 Apr 2020 09:37:09 +0200 Subject: [PATCH] Replace pp null check by assert in dds_create_writer and dds_create_reader, and rewrite logic in q_omg_security_check_remote_writer_permissions Signed-off-by: Dennis Potman --- src/core/ddsc/src/dds_reader.c | 14 +++---- src/core/ddsc/src/dds_writer.c | 13 +++--- src/core/ddsi/src/ddsi_security_omg.c | 58 +++++++++++++-------------- 3 files changed, 38 insertions(+), 47 deletions(-) diff --git a/src/core/ddsc/src/dds_reader.c b/src/core/ddsc/src/dds_reader.c index bffe59a..5af47a0 100644 --- a/src/core/ddsc/src/dds_reader.c +++ b/src/core/ddsc/src/dds_reader.c @@ -456,12 +456,11 @@ static dds_entity_t dds_create_reader_int (dds_entity_t participant_or_subscribe thread_state_awake (lookup_thread_state (), gv); const struct ddsi_guid * ppguid = dds_entity_participant_guid (&sub->m_entity); struct participant * pp = entidx_lookup_participant_guid (gv->entity_index, ppguid); - if (pp == NULL) - { - GVLOGDISC ("new_reader - participant "PGUIDFMT" not found\n", PGUID (*ppguid)); - rc = DDS_RETCODE_BAD_PARAMETER; - goto err_pp_not_found; - } + + /* When deleting a participant, the child handles (that include the subscriber) + are removed before removing the DDSI participant. So at this point, within + the subscriber lock, we can assert that the participant exists. */ + assert (pp != NULL); #ifdef DDSI_INCLUDE_SECURITY /* Check if DDS Security is enabled */ @@ -508,9 +507,8 @@ static dds_entity_t dds_create_reader_int (dds_entity_t participant_or_subscribe #ifdef DDSI_INCLUDE_SECURITY err_not_allowed: -#endif -err_pp_not_found: thread_state_asleep (lookup_thread_state ()); +#endif err_bad_qos: dds_delete_qos (rqos); dds_topic_allow_set_qos (tp); diff --git a/src/core/ddsc/src/dds_writer.c b/src/core/ddsc/src/dds_writer.c index 12d2676..bebebcc 100644 --- a/src/core/ddsc/src/dds_writer.c +++ b/src/core/ddsc/src/dds_writer.c @@ -329,12 +329,10 @@ dds_entity_t dds_create_writer (dds_entity_t participant_or_publisher, dds_entit thread_state_awake (lookup_thread_state (), gv); const struct ddsi_guid *ppguid = dds_entity_participant_guid (&pub->m_entity); struct participant *pp = entidx_lookup_participant_guid (gv->entity_index, ppguid); - if (pp == NULL) - { - GVLOGDISC ("new_writer - participant "PGUIDFMT" not found\n", PGUID (*ppguid)); - rc = DDS_RETCODE_BAD_PARAMETER; - goto err_pp_not_found; - } + /* When deleting a participant, the child handles (that include the publisher) + are removed before removing the DDSI participant. So at this point, within + the publisher lock, we can assert that the participant exists. */ + assert (pp != NULL); #ifdef DDSI_INCLUDE_SECURITY /* Check if DDS Security is enabled */ @@ -377,9 +375,8 @@ dds_entity_t dds_create_writer (dds_entity_t participant_or_publisher, dds_entit #ifdef DDSI_INCLUDE_SECURITY err_not_allowed: -#endif -err_pp_not_found: thread_state_asleep (lookup_thread_state ()); +#endif err_bad_qos: dds_delete_qos(wqos); dds_topic_allow_set_qos (tp); diff --git a/src/core/ddsi/src/ddsi_security_omg.c b/src/core/ddsi/src/ddsi_security_omg.c index c7f2b10..c073166 100644 --- a/src/core/ddsi/src/ddsi_security_omg.c +++ b/src/core/ddsi/src/ddsi_security_omg.c @@ -2137,7 +2137,6 @@ bool q_omg_security_check_remote_writer_permissions(const struct proxy_writer *p DDS_Security_SecurityException exception = DDS_SECURITY_EXCEPTION_INIT; DDS_Security_PublicationBuiltinTopicDataSecure publication_data; DDS_Security_TopicBuiltinTopicData topic_data; - bool result = true; if (!sc) return true; @@ -2156,42 +2155,39 @@ bool q_omg_security_check_remote_writer_permissions(const struct proxy_writer *p } } - if (SECURITY_INFO_IS_WRITE_PROTECTED(pwr->c.security_info)) - { - DDS_Security_PermissionsHandle permissions_handle; + if (!SECURITY_INFO_IS_WRITE_PROTECTED(pwr->c.security_info)) + return true; - if ((permissions_handle = get_permissions_handle(pp, pwr->c.proxypp)) == 0) - { - GVTRACE("Secure remote writer "PGUIDFMT" proxypp does not have permissions handle yet\n", PGUID(pwr->e.guid)); - return false; - } + DDS_Security_PermissionsHandle permissions_handle; + if ((permissions_handle = get_permissions_handle(pp, pwr->c.proxypp)) == 0) + { + GVTRACE("Secure remote writer "PGUIDFMT" proxypp does not have permissions handle yet\n", PGUID(pwr->e.guid)); + return false; + } + + q_omg_shallow_copy_PublicationBuiltinTopicDataSecure(&publication_data, &pwr->e.guid, pwr->c.xqos, &pwr->c.security_info); + bool result = sc->access_control_context->check_remote_datawriter(sc->access_control_context, permissions_handle, (int)domain_id, &publication_data, &exception); + if (!result) + { + if (!is_topic_discovery_protected(pp->sec_attr->permissions_handle, sc->access_control_context, publication_data.topic_name)) + EXCEPTION_ERROR(gv, &exception, "Access control does not allow remote writer "PGUIDFMT": %s", PGUID(pwr->e.guid)); else + DDS_Security_Exception_reset(&exception); + } + else + { + q_omg_shallow_copy_TopicBuiltinTopicData(&topic_data, publication_data.topic_name, publication_data.type_name); + result = sc->access_control_context->check_remote_topic(sc->access_control_context, permissions_handle, (int)domain_id, &topic_data, &exception); + q_omg_shallow_free_TopicBuiltinTopicData(&topic_data); + if (!result) { - q_omg_shallow_copy_PublicationBuiltinTopicDataSecure(&publication_data, &pwr->e.guid, pwr->c.xqos, &pwr->c.security_info); - result = sc->access_control_context->check_remote_datawriter(sc->access_control_context, permissions_handle, (int)domain_id, &publication_data, &exception); - if (!result) - { - if (!is_topic_discovery_protected(pp->sec_attr->permissions_handle, sc->access_control_context, publication_data.topic_name)) - EXCEPTION_ERROR(gv, &exception, "Access control does not allow remote writer "PGUIDFMT": %s", PGUID(pwr->e.guid)); - else - DDS_Security_Exception_reset(&exception); - } + if (!is_topic_discovery_protected(pp->sec_attr->permissions_handle, sc->access_control_context, publication_data.topic_name)) + EXCEPTION_ERROR(gv, &exception, "Access control does not allow remote topic %s: %s", publication_data.topic_name); else - { - q_omg_shallow_copy_TopicBuiltinTopicData(&topic_data, publication_data.topic_name, publication_data.type_name); - result = sc->access_control_context->check_remote_topic(sc->access_control_context, permissions_handle, (int)domain_id, &topic_data, &exception); - q_omg_shallow_free_TopicBuiltinTopicData(&topic_data); - if (!result) - { - if (!is_topic_discovery_protected(pp->sec_attr->permissions_handle, sc->access_control_context, publication_data.topic_name)) - EXCEPTION_ERROR(gv, &exception, "Access control does not allow remote topic %s: %s", publication_data.topic_name); - else - DDS_Security_Exception_reset(&exception); - } - } - q_omg_shallow_free_PublicationBuiltinTopicDataSecure(&publication_data); + DDS_Security_Exception_reset(&exception); } } + q_omg_shallow_free_PublicationBuiltinTopicDataSecure(&publication_data); return result; }