niklas/cyclonedds
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix race: delete participant, receipt of own SPDP

Browse source 
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 <eb@ilities.com>
This commit is contained in:
Erik Boasson 2019-09-10 18:58:00 +02:00 • committed by eboasson
parent d9dac3b7e2
commit 1e094c6fbb
2 changed files with 49 additions and 44 deletions
Download patch file Download diff file Expand all files Collapse all files

90
src/core/ddsi/src/q_ddsi_discovery.c
View file

@ -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; struct q_globals * const gv = rst->gv;
ddsi_guid_t guid; 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) 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_SUBSCRIPTION_ANNOUNCER |
NN_DISC_BUILTIN_ENDPOINT_PUBLICATION_ANNOUNCER; NN_DISC_BUILTIN_ENDPOINT_PUBLICATION_ANNOUNCER;
struct addrset *as_meta, *as_default; struct addrset *as_meta, *as_default;
struct proxy_participant *proxypp;
unsigned builtin_endpoint_set; unsigned builtin_endpoint_set;
unsigned prismtech_builtin_endpoint_set; unsigned prismtech_builtin_endpoint_set;
ddsi_guid_t privileged_pp_guid; 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)) 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]); 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 /* 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; 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 /* Do we know this GUID already? */
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))
{ {
RSTTRACE ("SPDP ST0 "PGUIDFMT" (recently deleted)", PGUID (datap->participant_guid)); struct entity_common *existing_entity;
return 1; if ((existing_entity = ephash_lookup_guid_untyped (gv->guid_hash, &datap->participant_guid)) == NULL)
}
{
int islocal = 0;
if (ephash_lookup_participant_guid (gv->guid_hash, &datap->participant_guid))
islocal = 1;
if (islocal)
{ {
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; return 0;
} }
} else if (existing_entity->kind == EK_PROXY_PARTICIPANT)
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)
{ {
if (proxypp->implicitly_created) struct proxy_participant *proxypp = (struct proxy_participant *) existing_entity;
GVLOGDISC (" (NEW was-implicitly-created)"); int interesting = 0;
else RSTTRACE ("SPDP ST0 "PGUIDFMT" (known)", PGUID (datap->participant_guid));
GVLOGDISC (" (update)"); /* SPDP processing is so different from normal processing that we are
proxypp->implicitly_created = 0; even skipping the automatic lease renewal. Therefore do it regardless
update_proxy_participant_plist_locked (proxypp, seq, datap, UPD_PROXYPP_SPDP, timestamp); 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); 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; struct q_globals * const gv = rst->gv;
const struct CDRHeader *data = vdata; /* built-ins not deserialized (yet) */ const struct CDRHeader *data = vdata; /* built-ins not deserialized (yet) */
RSTTRACE("SPDP ST%x", statusinfo);
if (data == NULL) if (data == NULL)
{ {
RSTTRACE(" no payload?\n"); RSTTRACE ("SPDP ST%x no payload?\n", statusinfo);
return; return;
} }
else else

3
src/core/ddsi/src/q_entity.c
View file

@ -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) dds_return_t delete_participant (struct q_globals *gv, const struct ddsi_guid *ppguid)
{ {
struct participant *pp; struct participant *pp;
GVLOGDISC ("delete_participant("PGUIDFMT")\n", PGUID (*ppguid));
if ((pp = ephash_lookup_participant_guid (gv->guid_hash, ppguid)) == NULL) if ((pp = ephash_lookup_participant_guid (gv->guid_hash, ppguid)) == NULL)
return DDS_RETCODE_BAD_PARAMETER; return DDS_RETCODE_BAD_PARAMETER;
builtintopic_write (gv->builtin_topic_interface, &pp->e, now(), false); 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; prd->c.as = as;
/* Rebuild writer endpoints */ /* Rebuild writer endpoints */
while ((m = ddsrt_avl_lookup_succ_eq (&prd_writers_treedef, &prd->writers, &wrguid)) != NULL) while ((m = ddsrt_avl_lookup_succ_eq (&prd_writers_treedef, &prd->writers, &wrguid)) != NULL)
{ {
struct prd_wr_match *next; struct prd_wr_match *next;