From 1e094c6fbb27ce3ff62fe53359070b25b0c297a4 Mon Sep 17 00:00:00 2001 From: Erik Boasson Date: Tue, 10 Sep 2019 18:58:00 +0200 Subject: [PATCH] Fix race: delete participant, receipt of own SPDP Deleting participant does: add to "deleted participants", remove from GUID hash table; so SPDP processing must first check for an existing participant and check deleted participants if nothing found. Signed-off-by: Erik Boasson --- src/core/ddsi/src/q_ddsi_discovery.c | 90 +++++++++++++++------------- src/core/ddsi/src/q_entity.c | 3 +- 2 files changed, 49 insertions(+), 44 deletions(-) diff --git a/src/core/ddsi/src/q_ddsi_discovery.c b/src/core/ddsi/src/q_ddsi_discovery.c index 0491f7c..410c4b0 100644 --- a/src/core/ddsi/src/q_ddsi_discovery.c +++ b/src/core/ddsi/src/q_ddsi_discovery.c @@ -411,8 +411,7 @@ static int handle_SPDP_dead (const struct receiver_state *rst, nn_wctime_t times struct q_globals * const gv = rst->gv; ddsi_guid_t guid; - if (!(gv->logconfig.c.mask & DDS_LC_DISCOVERY)) - GVLOGDISC ("SPDP ST%x", statusinfo); + GVLOGDISC ("SPDP ST%x", statusinfo); if (datap->present & PP_PARTICIPANT_GUID) { @@ -514,7 +513,6 @@ static int handle_SPDP_alive (const struct receiver_state *rst, seqno_t seq, nn_ NN_DISC_BUILTIN_ENDPOINT_SUBSCRIPTION_ANNOUNCER | NN_DISC_BUILTIN_ENDPOINT_PUBLICATION_ANNOUNCER; struct addrset *as_meta, *as_default; - struct proxy_participant *proxypp; unsigned builtin_endpoint_set; unsigned prismtech_builtin_endpoint_set; ddsi_guid_t privileged_pp_guid; @@ -524,7 +522,7 @@ static int handle_SPDP_alive (const struct receiver_state *rst, seqno_t seq, nn_ if (!(datap->present & PP_PARTICIPANT_GUID) || !(datap->present & PP_BUILTIN_ENDPOINT_SET)) { GVWARNING ("data (SPDP, vendor %u.%u): no/invalid payload\n", rst->vendor.id[0], rst->vendor.id[1]); - return 1; + return 0; } /* At some point the RTI implementation didn't mention @@ -563,49 +561,56 @@ static int handle_SPDP_alive (const struct receiver_state *rst, seqno_t seq, nn_ prismtech_builtin_endpoint_set |= NN_DISC_BUILTIN_ENDPOINT_CM_PUBLISHER_WRITER | NN_DISC_BUILTIN_ENDPOINT_CM_SUBSCRIBER_WRITER; } - /* Local SPDP packets may be looped back, and that may include ones - currently being deleted. The first thing that happens when - deleting a participant is removing it from the hash table, and - consequently the looped back packet may appear to be from an - unknown participant. So we handle that, too. */ - - if (is_deleted_participant_guid (gv->deleted_participants, &datap->participant_guid, DPG_REMOTE)) + /* Do we know this GUID already? */ { - RSTTRACE ("SPDP ST0 "PGUIDFMT" (recently deleted)", PGUID (datap->participant_guid)); - return 1; - } - - { - int islocal = 0; - if (ephash_lookup_participant_guid (gv->guid_hash, &datap->participant_guid)) - islocal = 1; - if (islocal) + struct entity_common *existing_entity; + if ((existing_entity = ephash_lookup_guid_untyped (gv->guid_hash, &datap->participant_guid)) == NULL) { - RSTTRACE ("SPDP ST0 "PGUIDFMT" (local %d)", PGUID (datap->participant_guid), islocal); + /* Local SPDP packets may be looped back, and that can include ones + for participants currently being deleted. The first thing that + happens when deleting a participant is removing it from the hash + table, and consequently the looped back packet may appear to be + from an unknown participant. So we handle that. */ + if (is_deleted_participant_guid (gv->deleted_participants, &datap->participant_guid, DPG_REMOTE)) + { + RSTTRACE ("SPDP ST0 "PGUIDFMT" (recently deleted)", PGUID (datap->participant_guid)); + return 0; + } + } + else if (existing_entity->kind == EK_PARTICIPANT) + { + RSTTRACE ("SPDP ST0 "PGUIDFMT" (local)", PGUID (datap->participant_guid)); return 0; } - } - - if ((proxypp = ephash_lookup_proxy_participant_guid (gv->guid_hash, &datap->participant_guid)) != NULL) - { - /* SPDP processing is so different from normal processing that we - are even skipping the automatic lease renewal. Therefore do it - regardless of - gv.config.arrival_of_data_asserts_pp_and_ep_liveliness. */ - RSTTRACE ("SPDP ST0 "PGUIDFMT" (known)", PGUID (datap->participant_guid)); - lease_renew (ddsrt_atomic_ldvoidp (&proxypp->lease), now_et ()); - ddsrt_mutex_lock (&proxypp->e.lock); - if (proxypp->implicitly_created || seq > proxypp->seq) + else if (existing_entity->kind == EK_PROXY_PARTICIPANT) { - if (proxypp->implicitly_created) - GVLOGDISC (" (NEW was-implicitly-created)"); - else - GVLOGDISC (" (update)"); - proxypp->implicitly_created = 0; - update_proxy_participant_plist_locked (proxypp, seq, datap, UPD_PROXYPP_SPDP, timestamp); + struct proxy_participant *proxypp = (struct proxy_participant *) existing_entity; + int interesting = 0; + RSTTRACE ("SPDP ST0 "PGUIDFMT" (known)", PGUID (datap->participant_guid)); + /* SPDP processing is so different from normal processing that we are + even skipping the automatic lease renewal. Therefore do it regardless + of gv.config.arrival_of_data_asserts_pp_and_ep_liveliness. */ + lease_renew (ddsrt_atomic_ldvoidp (&proxypp->lease), now_et ()); + ddsrt_mutex_lock (&proxypp->e.lock); + if (proxypp->implicitly_created || seq > proxypp->seq) + { + interesting = 1; + if (!(gv->logconfig.c.mask & DDS_LC_TRACE)) + GVLOGDISC ("SPDP ST0 "PGUIDFMT, PGUID (datap->participant_guid)); + GVLOGDISC (proxypp->implicitly_created ? " (NEW was-implicitly-created)" : " (update)"); + proxypp->implicitly_created = 0; + update_proxy_participant_plist_locked (proxypp, seq, datap, UPD_PROXYPP_SPDP, timestamp); + } + ddsrt_mutex_unlock (&proxypp->e.lock); + return interesting; + } + else + { + /* mismatch on entity kind: that should never have gotten past the + input validation */ + GVWARNING ("data (SPDP, vendor %u.%u): "PGUIDFMT" kind mismatch\n", rst->vendor.id[0], rst->vendor.id[1], PGUID (datap->participant_guid)); + return 0; } - ddsrt_mutex_unlock (&proxypp->e.lock); - return 0; } GVLOGDISC ("SPDP ST0 "PGUIDFMT" bes %x ptbes %x NEW", PGUID (datap->participant_guid), builtin_endpoint_set, prismtech_builtin_endpoint_set); @@ -791,10 +796,9 @@ static void handle_SPDP (const struct receiver_state *rst, seqno_t seq, nn_wctim { struct q_globals * const gv = rst->gv; const struct CDRHeader *data = vdata; /* built-ins not deserialized (yet) */ - RSTTRACE("SPDP ST%x", statusinfo); if (data == NULL) { - RSTTRACE(" no payload?\n"); + RSTTRACE ("SPDP ST%x no payload?\n", statusinfo); return; } else diff --git a/src/core/ddsi/src/q_entity.c b/src/core/ddsi/src/q_entity.c index 1dd8df7..24add4d 100644 --- a/src/core/ddsi/src/q_entity.c +++ b/src/core/ddsi/src/q_entity.c @@ -921,6 +921,7 @@ static void gc_delete_participant (struct gcreq *gcreq) dds_return_t delete_participant (struct q_globals *gv, const struct ddsi_guid *ppguid) { struct participant *pp; + GVLOGDISC ("delete_participant("PGUIDFMT")\n", PGUID (*ppguid)); if ((pp = ephash_lookup_participant_guid (gv->guid_hash, ppguid)) == NULL) return DDS_RETCODE_BAD_PARAMETER; builtintopic_write (gv->builtin_topic_interface, &pp->e, now(), false); @@ -4202,7 +4203,7 @@ void update_proxy_reader (struct proxy_reader *prd, seqno_t seq, struct addrset prd->c.as = as; /* Rebuild writer endpoints */ - + while ((m = ddsrt_avl_lookup_succ_eq (&prd_writers_treedef, &prd->writers, &wrguid)) != NULL) { struct prd_wr_match *next;