From 8ca17805380787aff9bff6deb2b3196f0baaabff Mon Sep 17 00:00:00 2001 From: Marcel Jordense Date: Wed, 4 Mar 2020 18:34:30 +0100 Subject: [PATCH] Correct problem with cleanup of security handshake Signed-off-by: Marcel Jordense --- .../ddsi/include/dds/ddsi/ddsi_handshake.h | 3 +- src/core/ddsi/src/ddsi_handshake.c | 23 ++++++++---- src/core/ddsi/src/q_entity.c | 30 ++++++++-------- src/core/ddsi/src/q_transmit.c | 7 +++- .../dds/security/core/dds_security_fsm.h | 18 ++++++++++ src/security/core/src/dds_security_fsm.c | 35 +++++++++---------- 6 files changed, 74 insertions(+), 42 deletions(-) diff --git a/src/core/ddsi/include/dds/ddsi/ddsi_handshake.h b/src/core/ddsi/include/dds/ddsi/ddsi_handshake.h index f473d0f..307eb72 100644 --- a/src/core/ddsi/include/dds/ddsi/ddsi_handshake.h +++ b/src/core/ddsi/include/dds/ddsi/ddsi_handshake.h @@ -131,10 +131,9 @@ void ddsi_handshake_register(struct participant *pp, struct proxy_participant *p * * @param[in] pp The local participant. * @param[in] proxypp The remote participant. - * @param[in] handshake The handshake. * */ -void ddsi_handshake_remove(struct participant *pp, struct proxy_participant *proxypp, struct ddsi_handshake *handshake); +void ddsi_handshake_remove(struct participant *pp, struct proxy_participant *proxypp); /** * @brief Searches for the handshake associated with the specified participants diff --git a/src/core/ddsi/src/ddsi_handshake.c b/src/core/ddsi/src/ddsi_handshake.c index 5d1b967..847c796 100644 --- a/src/core/ddsi/src/ddsi_handshake.c +++ b/src/core/ddsi/src/ddsi_handshake.c @@ -21,6 +21,7 @@ #include "dds/ddsi/ddsi_entity_index.h" #include "dds/ddsi/ddsi_plist.h" #include "dds/ddsi/q_entity.h" +#include "dds/ddsi/q_gc.h" #include "dds/security/dds_security_api_types.h" #include "dds/security/dds_security_api.h" #include "dds/ddsi/ddsi_security_omg.h" @@ -991,6 +992,7 @@ void ddsi_handshake_release(struct ddsi_handshake *handshake) DDS_Security_DataHolder_free(handshake->handshake_message_out); DDS_Security_DataHolder_free(handshake->remote_auth_request_token); DDS_Security_OctetSeq_deinit(&handshake->pdata); + dds_security_fsm_free(handshake->fsm); ddsrt_mutex_destroy(&handshake->lock); ddsrt_free(handshake); } @@ -1146,22 +1148,31 @@ static struct ddsi_handshake * ddsi_handshake_find_locked( return ddsrt_avl_lookup(&handshake_treedef, &hsadmin->handshakes, &handles); } -void ddsi_handshake_remove(struct participant *pp, struct proxy_participant *proxypp, struct ddsi_handshake *handshake) +static void gc_delete_handshale (struct gcreq *gcreq) +{ + struct ddsi_handshake *handshake = gcreq->arg; + + ddsi_handshake_release(handshake); + gcreq_free(gcreq); +} + +void ddsi_handshake_remove(struct participant *pp, struct proxy_participant *proxypp) { struct ddsi_hsadmin *hsadmin = pp->e.gv->hsadmin; + struct ddsi_handshake *handshake = NULL; ddsrt_mutex_lock(&hsadmin->lock); - if (!handshake) - handshake = ddsi_handshake_find_locked(hsadmin, pp, proxypp); + handshake = ddsi_handshake_find_locked(hsadmin, pp, proxypp); if (handshake) { + struct gcreq *gcreq = gcreq_new (pp->e.gv->gcreq_queue, gc_delete_handshale); ddsrt_avl_delete(&handshake_treedef, &hsadmin->handshakes, handshake); ddsrt_atomic_st32(&handshake->deleting, 1); + dds_security_fsm_stop(handshake->fsm); + gcreq->arg = handshake; + gcreq_enqueue (gcreq); } ddsrt_mutex_unlock(&hsadmin->lock); - if (handshake && handshake->fsm) - dds_security_fsm_free(handshake->fsm); - ddsi_handshake_release(handshake); } struct ddsi_handshake * ddsi_handshake_find(struct participant *pp, struct proxy_participant *proxypp) diff --git a/src/core/ddsi/src/q_entity.c b/src/core/ddsi/src/q_entity.c index ab60485..69b75b5 100644 --- a/src/core/ddsi/src/q_entity.c +++ b/src/core/ddsi/src/q_entity.c @@ -668,7 +668,7 @@ static void disconnect_participant_secure(struct participant *pp) entidx_enum_proxy_participant_init (&it, gv->entity_index); while ((proxypp = entidx_enum_proxy_participant_next (&it)) != NULL) { - ddsi_handshake_remove(pp, proxypp, NULL); + ddsi_handshake_remove(pp, proxypp); } entidx_enum_proxy_participant_fini (&it); } @@ -3102,19 +3102,21 @@ static void match_volatile_secure_endpoints (struct participant *pp, struct prox guid = pp->e.guid; guid.entityid.u = NN_ENTITYID_P2P_BUILTIN_PARTICIPANT_VOLATILE_SECURE_READER; - rd = entidx_lookup_reader_guid (pp->e.gv->entity_index, &guid); - assert(rd); + if ((rd = entidx_lookup_reader_guid (pp->e.gv->entity_index, &guid)) == NULL) + return; + guid.entityid.u = NN_ENTITYID_P2P_BUILTIN_PARTICIPANT_VOLATILE_SECURE_WRITER; - wr = entidx_lookup_writer_guid (pp->e.gv->entity_index, &guid); - assert(wr); + if ((wr = entidx_lookup_writer_guid (pp->e.gv->entity_index, &guid)) == NULL) + return; guid = proxypp->e.guid; guid.entityid.u = NN_ENTITYID_P2P_BUILTIN_PARTICIPANT_VOLATILE_SECURE_READER; - prd = entidx_lookup_proxy_reader_guid (pp->e.gv->entity_index, &guid); - assert(rd); + if ((prd = entidx_lookup_proxy_reader_guid (pp->e.gv->entity_index, &guid)) == NULL) + return; + guid.entityid.u = NN_ENTITYID_P2P_BUILTIN_PARTICIPANT_VOLATILE_SECURE_WRITER; - pwr = entidx_lookup_proxy_writer_guid (pp->e.gv->entity_index, &guid); - assert(wr); + if ((pwr = entidx_lookup_proxy_writer_guid (pp->e.gv->entity_index, &guid)) == NULL) + return; connect_proxy_writer_with_reader_wrapper(&pwr->e, &rd->e, tnow); connect_writer_with_proxy_reader_wrapper(&wr->e, &prd->e, tnow); @@ -4771,7 +4773,7 @@ void handshake_end_cb(struct ddsi_handshake *handshake, struct participant *pp, case STATE_HANDSHAKE_OK: DDS_CLOG (DDS_LC_DISCOVERY, &gv->logconfig, "handshake (lguid="PGUIDFMT" rguid="PGUIDFMT") succeeded\n", PGUID (pp->e.guid), PGUID (proxypp->e.guid)); update_proxy_participant_endpoint_matching(proxypp, pp); - ddsi_handshake_remove(pp, proxypp, handshake); + ddsi_handshake_remove(pp, proxypp); break; case STATE_HANDSHAKE_TIMED_OUT: @@ -4780,7 +4782,7 @@ void handshake_end_cb(struct ddsi_handshake *handshake, struct participant *pp, downgrade_to_nonsecure(proxypp); update_proxy_participant_endpoint_matching(proxypp, pp); } - ddsi_handshake_remove(pp, proxypp, handshake); + ddsi_handshake_remove(pp, proxypp); break; case STATE_HANDSHAKE_FAILED: DDS_CERROR (&gv->logconfig, "handshake (lguid="PGUIDFMT" rguid="PGUIDFMT") failed: (%d) Failed\n", PGUID (pp->e.guid), PGUID (proxypp->e.guid), (int)result); @@ -4788,11 +4790,11 @@ void handshake_end_cb(struct ddsi_handshake *handshake, struct participant *pp, downgrade_to_nonsecure(proxypp); update_proxy_participant_endpoint_matching(proxypp, pp); } - ddsi_handshake_remove(pp, proxypp, handshake); + ddsi_handshake_remove(pp, proxypp); break; default: DDS_CERROR (&gv->logconfig, "handshake (lguid="PGUIDFMT" rguid="PGUIDFMT") failed: (%d) Unknown failure\n", PGUID (pp->e.guid), PGUID (proxypp->e.guid), (int)result); - ddsi_handshake_remove(pp, proxypp, handshake); + ddsi_handshake_remove(pp, proxypp); break; } } @@ -4840,7 +4842,7 @@ static void disconnect_proxy_participant_secure(struct proxy_participant *proxyp entidx_enum_participant_init (&it, gv->entity_index); while ((pp = entidx_enum_participant_next (&it)) != NULL) { - ddsi_handshake_remove(pp, proxypp, NULL); + ddsi_handshake_remove(pp, proxypp); } entidx_enum_participant_fini (&it); } diff --git a/src/core/ddsi/src/q_transmit.c b/src/core/ddsi/src/q_transmit.c index cdc7485..774e4f8 100644 --- a/src/core/ddsi/src/q_transmit.c +++ b/src/core/ddsi/src/q_transmit.c @@ -1159,7 +1159,7 @@ static int maybe_grow_whc (struct writer *wr) int write_sample_p2p_wrlock_held(struct writer *wr, seqno_t seq, struct ddsi_plist *plist, struct ddsi_serdata *serdata, struct ddsi_tkmap_instance *tk, struct proxy_reader *prd) { struct ddsi_domaingv * const gv = wr->e.gv; - int r; + int r = 0; nn_mtime_t tnow; int rexmit = 1; struct wr_prd_match *wprd = NULL; @@ -1170,10 +1170,14 @@ int write_sample_p2p_wrlock_held(struct writer *wr, seqno_t seq, struct ddsi_pli serdata->twrite = tnow; serdata->timestamp = now(); + if (prd->filter) { if ((wprd = ddsrt_avl_lookup (&wr_readers_treedef, &wr->readers, &prd->e.guid)) != NULL) { + if (wprd->seq == MAX_SEQ_NUMBER) + goto prd_is_deleting; + rexmit = prd->filter(wr, prd, serdata); /* determine if gap has to added */ if (rexmit) @@ -1212,6 +1216,7 @@ int write_sample_p2p_wrlock_held(struct writer *wr, seqno_t seq, struct ddsi_pli writer_hbcontrol_note_asyncwrite(wr, tnow); } +prd_is_deleting: return r; } diff --git a/src/security/core/include/dds/security/core/dds_security_fsm.h b/src/security/core/include/dds/security/core/dds_security_fsm.h index 503c5f6..96f24d9 100644 --- a/src/security/core/include/dds/security/core/dds_security_fsm.h +++ b/src/security/core/include/dds/security/core/dds_security_fsm.h @@ -147,6 +147,24 @@ dds_security_fsm_dispatch(struct dds_security_fsm *fsm, int32_t event_id, bool p DDS_EXPORT const dds_security_fsm_state* dds_security_fsm_current_state(struct dds_security_fsm *fsm); +/** + * Stops the state machine. + * Stops all running timeouts and events and cleaning all memory + * related to this machine. + * + * When calling this from another thread, then it may block until + * a possible concurrent event has finished. After this call, the + * fsm may not be used anymore. + * + * When in the fsm action callback function context, this will + * not block. It will garbage collect when the event has been + * handled. + * + * @param fsm The state machine to b stopped + */ +DDS_EXPORT void +dds_security_fsm_stop(struct dds_security_fsm *fsm); + /** * Free the state machine. * Stops all running timeouts and events and cleaning all memory diff --git a/src/security/core/src/dds_security_fsm.c b/src/security/core/src/dds_security_fsm.c index 8685ed0..e2b9436 100644 --- a/src/security/core/src/dds_security_fsm.c +++ b/src/security/core/src/dds_security_fsm.c @@ -49,7 +49,6 @@ struct dds_security_fsm { struct dds_security_fsm *next_fsm; struct dds_security_fsm *prev_fsm; - bool busy; bool deleting; struct dds_security_fsm_control *control; const dds_security_fsm_transition *transitions; @@ -73,7 +72,6 @@ struct dds_security_fsm_control struct fsm_event *first_event; struct fsm_event *last_event; ddsrt_fibheap_t timers; - ddsrt_thread_t tid; bool running; }; @@ -230,7 +228,7 @@ static void fsm_check_auto_state_change (struct dds_security_fsm *fsm) } } -static void fsm_state_change (struct thread_state1 *ts1, struct dds_security_fsm_control *control, struct fsm_event *event) +static void fsm_state_change (struct dds_security_fsm_control *control, struct fsm_event *event) { struct dds_security_fsm *fsm = event->fsm; int event_id = event->event_id; @@ -246,19 +244,15 @@ static void fsm_state_change (struct thread_state1 *ts1, struct dds_security_fsm clear_state_timer (fsm); fsm->current = fsm->transitions[i].end; set_state_timer (fsm); - fsm->busy = true; ddsrt_mutex_unlock (&control->lock); - thread_state_awake (ts1, control->gv); if (fsm->transitions[i].func) fsm->transitions[i].func (fsm, fsm->arg); if (fsm->current && fsm->current->func) fsm->current->func (fsm, fsm->arg); - thread_state_asleep (ts1); ddsrt_mutex_lock (&control->lock); - fsm->busy = false; if (!fsm->deleting) fsm_check_auto_state_change (fsm); else @@ -278,12 +272,10 @@ static void fsm_handle_timeout (struct dds_security_fsm_control *control, struct fsm_dispatch (fsm, DDS_SECURITY_FSM_EVENT_TIMEOUT, true); break; case FSM_TIMEOUT_OVERALL: - fsm->busy = true; ddsrt_mutex_unlock (&control->lock); if (fsm->overall_timeout_action) fsm->overall_timeout_action (fsm, fsm->arg); ddsrt_mutex_lock (&control->lock); - fsm->busy = false; if (fsm->deleting) ddsrt_cond_broadcast(&control->cond); break; @@ -298,15 +290,13 @@ static uint32_t handle_events (struct dds_security_fsm_control *control) struct thread_state1 * const ts1 = lookup_thread_state (); struct fsm_event *event; - control->tid = ddsrt_thread_self(); - thread_state_awake (ts1, control->gv); ddsrt_mutex_lock (&control->lock); while (control->running) { if ((event = get_event(control)) != NULL) { - fsm_state_change (ts1, control, event); + fsm_state_change (control, event); ddsrt_free (event); } else @@ -456,7 +446,6 @@ struct dds_security_fsm * dds_security_fsm_create (struct dds_security_fsm_contr fsm->overall_timeout_event.kind = FSM_TIMEOUT_OVERALL; fsm->overall_timeout_event.endtime = DDS_NEVER; fsm->overall_timeout_event.fsm = fsm; - fsm->busy = false; fsm->deleting = false; fsm->next_fsm = NULL; fsm->prev_fsm = NULL; @@ -482,14 +471,9 @@ static void fsm_deactivate (struct dds_security_fsm_control *control, struct dds clear_state_timer (fsm); clear_overall_timer (fsm); fsm->current = NULL; - if (!ddsrt_thread_equal(control->tid, ddsrt_thread_self())) - { - while (fsm->busy) - ddsrt_cond_wait(&control->cond, &control->lock); - } } -void dds_security_fsm_free (struct dds_security_fsm *fsm) +void dds_security_fsm_stop (struct dds_security_fsm *fsm) { struct dds_security_fsm_control *control; @@ -509,6 +493,19 @@ static void fsm_delete (struct dds_security_fsm_control *control, struct dds_sec ddsrt_free(fsm); } +void dds_security_fsm_free (struct dds_security_fsm *fsm) +{ + struct dds_security_fsm_control *control; + + assert(fsm); + assert(fsm->control); + + control = fsm->control; + ddsrt_mutex_lock (&control->lock); + fsm_delete (control, fsm); + ddsrt_mutex_unlock (&control->lock); +} + struct dds_security_fsm_control * dds_security_fsm_control_create (struct ddsi_domaingv *gv) { struct dds_security_fsm_control *control;