From 99df0956e798c4d219d52b47fbc682c5e4ba144b Mon Sep 17 00:00:00 2001
From: Erik Boasson <eb@ilities.com>
Date: Tue, 31 Mar 2020 16:27:21 +0200
Subject: [PATCH] Crypto endpoint relation compare routines cleanup

* Trying not to assume an int is at least 32 bits.

* Technically speaking, comparing "unrelated" addresses is undefined
  behaviour which can be avoided by a cast to uintptr_t.

* The early out if either local_crypto == 0 does work in context,
  provided the nodes in tree never have local_crypto == 0.  That implies
  crypto_insert_endpoint_relation must never have a 0 in there, which I
  think the callers do respect.  Still I think it is better to not hide
  these assumptions in the compare function and address the problem in
  the lookup function instead.

These changes likely make the code fractionally slower, but I do think
they improve clarity.

Signed-off-by: Erik Boasson <eb@ilities.com>
---
 .../cryptographic/src/crypto_objects.c        | 42 +++++++------------
 .../core/tests/secure_communication.c         |  2 +-
 2 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/src/security/builtin_plugins/cryptographic/src/crypto_objects.c b/src/security/builtin_plugins/cryptographic/src/crypto_objects.c
index 075744e..17929af 100644
--- a/src/security/builtin_plugins/cryptographic/src/crypto_objects.c
+++ b/src/security/builtin_plugins/cryptographic/src/crypto_objects.c
@@ -52,17 +52,13 @@ static int compare_endpoint_relation (const void *va, const void *vb)
 {
   const key_relation *ra = va;
   const key_relation *rb = vb;
-  int r;
-
-  r = (int)(ra->key_id - rb->key_id);
-  if (r != 0 || ra->local_crypto == 0 || rb->local_crypto == 0)
-  {
-    return r;
-  }
-
-  if (ra->local_crypto > rb->local_crypto)
+  if (ra->key_id > rb->key_id)
     return 1;
-  else if (ra->local_crypto < rb->local_crypto)
+  else if (ra->key_id < rb->key_id)
+    return -1;
+  else if ((uintptr_t) ra->local_crypto > (uintptr_t) rb->local_crypto)
+    return 1;
+  else if ((uintptr_t) ra->local_crypto < (uintptr_t) rb->local_crypto)
     return -1;
   else
     return 0;
@@ -72,8 +68,7 @@ static int compare_relation_key (const void *va, const void *vb)
 {
   const uint32_t *ka = va;
   const uint32_t *kb = vb;
-
-  return (int)(*ka - *kb);
+  return (*ka == *kb) ? 0 : (*ka < *kb) ? -1 : 1;
 }
 
 bool crypto_object_valid(CryptoObject *obj, CryptoObjectKind_t kind)
@@ -692,6 +687,7 @@ void crypto_insert_endpoint_relation(
     remote_participant_crypto *rpc,
     key_relation *relation)
 {
+  assert (relation->local_crypto != NULL && relation->remote_crypto != NULL);
   ddsrt_mutex_lock(&rpc->lock);
   ddsrt_avl_insert(&endpoint_relation_treedef, &rpc->relation_index, CRYPTO_OBJECT_KEEP(relation));
   ddsrt_mutex_unlock(&rpc->lock);
@@ -702,15 +698,11 @@ void crypto_remove_endpoint_relation(
     CryptoObject *lch,
     uint32_t key_id)
 {
-  key_relation template;
+  const key_relation template = { .key_id = key_id, .local_crypto = lch };
   key_relation *relation;
   ddsrt_avl_dpath_t dpath;
-
-  template.key_id = key_id;
-  template.local_crypto = lch;
-
   ddsrt_mutex_lock(&rpc->lock);
-  relation =  ddsrt_avl_lookup_dpath(&endpoint_relation_treedef, &rpc->relation_index, &template, &dpath);
+  relation = ddsrt_avl_lookup_dpath(&endpoint_relation_treedef, &rpc->relation_index, &template, &dpath);
   if (relation)
   {
     ddsrt_avl_delete_dpath(&endpoint_relation_treedef, &rpc->relation_index, relation, &dpath);
@@ -724,16 +716,14 @@ key_relation * crypto_find_endpoint_relation(
     CryptoObject *lch,
     uint32_t key_id)
 {
-  key_relation template;
-  key_relation *relation;
-
-  template.key_id = key_id;
-  template.local_crypto = lch;
-
+  const key_relation template = { .key_id = key_id, .local_crypto = lch };
+  key_relation *relation, *cand;
   ddsrt_mutex_lock(&rpc->lock);
-  relation =  CRYPTO_OBJECT_KEEP(ddsrt_avl_lookup(&endpoint_relation_treedef, &rpc->relation_index, &template));
+  if ((cand = ddsrt_avl_lookup_succ_eq (&endpoint_relation_treedef, &rpc->relation_index, &template)) == NULL || cand->key_id != key_id)
+    relation = NULL;
+  else
+    relation = CRYPTO_OBJECT_KEEP (cand);
   ddsrt_mutex_unlock(&rpc->lock);
-
   return relation;
 }
 
diff --git a/src/security/core/tests/secure_communication.c b/src/security/core/tests/secure_communication.c
index 5b46630..10769e0 100644
--- a/src/security/core/tests/secure_communication.c
+++ b/src/security/core/tests/secure_communication.c
@@ -544,7 +544,7 @@ CU_TheoryDataPoints(ddssec_secure_communication, multiple_readers) = {
     CU_DataPoints(size_t, 1, 3, 1, 3), /* number of participants per domain */
     CU_DataPoints(size_t, 3, 1, 3, 3), /* number of readers per participant */
 };
-CU_Theory((size_t n_dom, size_t n_pp, size_t n_rd), ddssec_secure_communication, multiple_readers, .timeout = 60, .disabled = false)
+CU_Theory((size_t n_dom, size_t n_pp, size_t n_rd), ddssec_secure_communication, multiple_readers, .timeout = 90, .disabled = false)
 {
   DDS_Security_ProtectionKind metadata_pk[] = { PK_N, PK_SOA, PK_EOA };
   DDS_Security_BasicProtectionKind payload_pk[] = { BPK_N, BPK_S, BPK_E };