From 52edbe94e98e8d2c1ef98610ef30f3bcb2f8d774 Mon Sep 17 00:00:00 2001
From: Erik Boasson <eb@ilities.com>
Date: Wed, 8 Apr 2020 11:35:49 +0200
Subject: [PATCH] plist handling of invalid input and keyhashes

This fixes some issues with the new discovery data ("plist" topics)
discovered on interoperating with some other DDS implementations:

* The interpretation of a keyhash as if it were a valid sample was wrong
  in various ways: inconsistent endianness, incorrect encoding
  identifier and a missing sentinel.  As Cyclone follows the spec and
  always provides a well-formed payload, the problem only surfaces when
  interoperating with implementations that expect the recipient to make
  do with a keyhash.

* Various paths failed to check for failure causing potential null
  pointer dereferences.

Signed-off-by: Erik Boasson <eb@ilities.com>
---
 src/core/ddsi/src/ddsi_serdata_plist.c | 16 ++++++++++++----
 src/core/ddsi/src/q_ddsi_discovery.c   |  7 +++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/src/core/ddsi/src/ddsi_serdata_plist.c b/src/core/ddsi/src/ddsi_serdata_plist.c
index 225f9e9..528fa81 100644
--- a/src/core/ddsi/src/ddsi_serdata_plist.c
+++ b/src/core/ddsi/src/ddsi_serdata_plist.c
@@ -102,6 +102,8 @@ static struct ddsi_serdata *serdata_plist_from_ser (const struct ddsi_sertopic *
 {
   const struct ddsi_sertopic_plist *tp = (const struct ddsi_sertopic_plist *) tpcmn;
   struct ddsi_serdata_plist *d = serdata_plist_new (tp, kind, size, NN_RMSG_PAYLOADOFF (fragchain->rmsg, NN_RDATA_PAYLOAD_OFF (fragchain)));
+  if (d == NULL)
+    return NULL;
   uint32_t off = 4; /* must skip the CDR header */
   assert (fragchain->min == 0);
   assert (fragchain->maxp1 >= off); /* CDR header must be in first fragment */
@@ -128,6 +130,8 @@ static struct ddsi_serdata *serdata_plist_from_ser_iov (const struct ddsi_sertop
   const struct ddsi_sertopic_plist *tp = (const struct ddsi_sertopic_plist *) tpcmn;
   assert (niov >= 1);
   struct ddsi_serdata_plist *d = serdata_plist_new (tp, kind, size, iov[0].iov_base);
+  if (d == NULL)
+    return NULL;
   memcpy (d->data + d->pos, (const char *) iov[0].iov_base + 4, iov[0].iov_len - 4);
   d->pos += (uint32_t) iov[0].iov_len - 4;
   for (ddsrt_msg_iovlen_t i = 1; i < niov; i++)
@@ -141,14 +145,18 @@ static struct ddsi_serdata *serdata_plist_from_ser_iov (const struct ddsi_sertop
 static struct ddsi_serdata *serdata_plist_from_keyhash (const struct ddsi_sertopic *tpcmn, const ddsi_keyhash_t *keyhash)
 {
   const struct ddsi_sertopic_plist *tp = (const struct ddsi_sertopic_plist *) tpcmn;
-  const struct { uint16_t identifier, options; nn_parameter_t par; ddsi_keyhash_t kh; } in = {
-    .identifier = CDR_BE,
+  const struct { uint16_t identifier, options; nn_parameter_t par; ddsi_keyhash_t kh; nn_parameter_t sentinel; } in = {
+    .identifier = PL_CDR_BE,
     .options = 0,
     .par = {
       .parameterid = ddsrt_toBE2u (tp->keyparam),
-      .length = sizeof (*keyhash)
+      .length = ddsrt_toBE2u ((uint16_t) sizeof (*keyhash))
     },
-    *keyhash
+    .kh = *keyhash,
+    .sentinel = {
+      .parameterid = ddsrt_toBE2u (PID_SENTINEL),
+      .length = 0
+    }
   };
   const ddsrt_iovec_t iov = { .iov_base = (void *) &in, .iov_len = sizeof (in) };
   return serdata_plist_from_ser_iov (tpcmn, SDK_KEY, 1, &iov, sizeof (in) - 4);
diff --git a/src/core/ddsi/src/q_ddsi_discovery.c b/src/core/ddsi/src/q_ddsi_discovery.c
index 865a8dc..f8ee6eb 100644
--- a/src/core/ddsi/src/q_ddsi_discovery.c
+++ b/src/core/ddsi/src/q_ddsi_discovery.c
@@ -1534,6 +1534,13 @@ int builtins_dqueue_handler (const struct nn_rsample_info *sampleinfo, const str
                PGUID (srcguid), sampleinfo->seq);
     goto done_upd_deliv;
   }
+  if (d == NULL)
+  {
+    GVLOG (DDS_LC_DISCOVERY | DDS_LC_WARNING, "data(builtin, vendor %u.%u): "PGUIDFMT" #%"PRId64": deserialization failed\n",
+           sampleinfo->rst->vendor.id[0], sampleinfo->rst->vendor.id[1],
+           PGUID (srcguid), sampleinfo->seq);
+    goto done_upd_deliv;
+  }
 
   d->timestamp = (sampleinfo->timestamp.v != DDSRT_WCTIME_INVALID.v) ? sampleinfo->timestamp : ddsrt_time_wallclock ();
   d->statusinfo = statusinfo;