Add some better memory safety in discovery controller code. (#8281)

Instead of assuming that subclasses give us a buffer of size at least CHIP_DEVICE_CONFIG_MAX_DISCOVERED_NODES, just enforce that via the type system.

Also switch to ranged for loops over the resulting buffers.

The changes to Span.h are for two reasons:

1) To support ranged for over a FixedSpan.
2) To disallow constructing a FixedSpan from a buffer known to be too small.

src/controller/AbstractMdnsDiscoveryController.cpp

@@ -31,25 +31,25 @@ namespace Controller {
 
 void AbstractMdnsDiscoveryController::OnNodeDiscoveryComplete(const chip::Mdns::DiscoveredNodeData & nodeData)
 {
-    Mdns::DiscoveredNodeData * mDiscoveredNodes = GetDiscoveredNodes();
-    for (int i = 0; i < CHIP_DEVICE_CONFIG_MAX_DISCOVERED_NODES; ++i)
+    auto discoveredNodes = GetDiscoveredNodes();
+    for (auto & discoveredNode : discoveredNodes)
     {
-        if (!mDiscoveredNodes[i].IsValid())
+        if (!discoveredNode.IsValid())
         {
             continue;
         }
-        if (strcmp(mDiscoveredNodes[i].hostName, nodeData.hostName) == 0)
+        if (strcmp(discoveredNode.hostName, nodeData.hostName) == 0)
         {
-            mDiscoveredNodes[i] = nodeData;
+            discoveredNode = nodeData;
             return;
         }
     }
     // Node not yet in the list
-    for (int i = 0; i < CHIP_DEVICE_CONFIG_MAX_DISCOVERED_NODES; ++i)
+    for (auto & discoveredNode : discoveredNodes)
     {
-        if (!mDiscoveredNodes[i].IsValid())
+        if (!discoveredNode.IsValid())
         {
-            mDiscoveredNodes[i] = nodeData;
+            discoveredNode = nodeData;
             return;
         }
     }
@@ -63,21 +63,21 @@ CHIP_ERROR AbstractMdnsDiscoveryController::SetUpNodeDiscovery()
     ReturnErrorOnFailure(chip::Mdns::Resolver::Instance().StartResolver(&DeviceLayer::InetLayer, kMdnsPort));
 #endif
 
-    Mdns::DiscoveredNodeData * mDiscoveredNodes = GetDiscoveredNodes();
-    for (int i = 0; i < CHIP_DEVICE_CONFIG_MAX_DISCOVERED_NODES; ++i)
+    auto discoveredNodes = GetDiscoveredNodes();
+    for (auto & discoveredNode : discoveredNodes)
     {
-        mDiscoveredNodes[i].Reset();
+        discoveredNode.Reset();
     }
     return CHIP_NO_ERROR;
 }
 
 const Mdns::DiscoveredNodeData * AbstractMdnsDiscoveryController::GetDiscoveredNode(int idx)
 {
     // TODO(cecille): Add assertion about main loop.
-    Mdns::DiscoveredNodeData * mDiscoveredNodes = GetDiscoveredNodes();
-    if (mDiscoveredNodes[idx].IsValid())
+    auto discoveredNodes = GetDiscoveredNodes();
+    if (0 <= idx && idx < CHIP_DEVICE_CONFIG_MAX_DISCOVERED_NODES && discoveredNodes.data()[idx].IsValid())
     {
-        return &mDiscoveredNodes[idx];
+        return discoveredNodes.data() + idx;
     }
     return nullptr;
 }

src/controller/AbstractMdnsDiscoveryController.h

@@ -18,6 +18,7 @@
 
 #pragma once
 
+#include <lib/support/Span.h>
 #include <mdns/Resolver.h>
 #include <platform/CHIPDeviceConfig.h>
 
@@ -45,9 +46,10 @@ class DLL_EXPORT AbstractMdnsDiscoveryController : public Mdns::ResolverDelegate
     void OnNodeDiscoveryComplete(const chip::Mdns::DiscoveredNodeData & nodeData) override;
 
 protected:
+    using DiscoveredNodeList = FixedSpan<Mdns::DiscoveredNodeData, CHIP_DEVICE_CONFIG_MAX_DISCOVERED_NODES>;
     CHIP_ERROR SetUpNodeDiscovery();
     const Mdns::DiscoveredNodeData * GetDiscoveredNode(int idx);
-    virtual Mdns::DiscoveredNodeData * GetDiscoveredNodes() = 0;
+    virtual DiscoveredNodeList GetDiscoveredNodes() = 0;
 };
 
 } // namespace Controller

src/controller/CHIPCommissionableNodeController.h

@@ -55,7 +55,7 @@ class DLL_EXPORT CommissionableNodeController : public AbstractMdnsDiscoveryCont
     }
 
 protected:
-    Mdns::DiscoveredNodeData * GetDiscoveredNodes() override { return mDiscoveredCommissioners; }
+    DiscoveredNodeList GetDiscoveredNodes() override { return DiscoveredNodeList(mDiscoveredCommissioners); }
 
 private:
     Mdns::DiscoveredNodeData mDiscoveredCommissioners[CHIP_DEVICE_CONFIG_MAX_DISCOVERED_NODES];

src/controller/CHIPDeviceController.h

@@ -348,7 +348,7 @@ class DLL_EXPORT DeviceController : public Messaging::ExchangeDelegate,
     //////////// ResolverDelegate Implementation ///////////////
     void OnNodeIdResolved(const chip::Mdns::ResolvedNodeData & nodeData) override;
     void OnNodeIdResolutionFailed(const chip::PeerId & peerId, CHIP_ERROR error) override;
-    Mdns::DiscoveredNodeData * GetDiscoveredNodes() override { return mCommissionableNodes; }
+    DiscoveredNodeList GetDiscoveredNodes() override { return DiscoveredNodeList(mCommissionableNodes); }
 #endif // CHIP_DEVICE_CONFIG_ENABLE_MDNS
 
     // This function uses `OperationalCredentialsDelegate` to generate the operational certificates

src/lib/support/Span.h

@@ -83,15 +83,41 @@ template <class T, size_t N>
 class FixedSpan
 {
 public:
-    using pointer = T *;
+    using pointer       = T *;
+    using const_pointer = const T *;
 
     constexpr FixedSpan() : mDataBuf(nullptr) {}
-    constexpr explicit FixedSpan(pointer databuf) : mDataBuf(databuf) {}
+
+    // We want to allow construction from things that look like T*, but we want
+    // to make construction from an array use the constructor that asserts the
+    // array is big enough.  This requires that both constructors be templates
+    // (because otherwise the non-template would be favored by overload
+    // resolution, since due to decay to pointer it matches just as well as the
+    // template).
+    //
+    // To do that we have a template constructor enabled only when the type
+    // passed to it is a pointer type, and rely on our assignment of that
+    // pointer type to mDataBuf to verify that the pointer is to a type
+    // compatible enough with T (T itself, a subclass, a non-const version if T
+    // is const, etc).
+    template <class U, typename = std::enable_if_t<std::is_pointer<U>::value>>
+    constexpr explicit FixedSpan(U databuf) : mDataBuf(databuf)
+    {}
+    template <class U, size_t M>
+    constexpr explicit FixedSpan(U (&databuf)[M]) : mDataBuf(databuf)
+    {
+        static_assert(M >= N, "Passed-in buffer too small for FixedSpan");
+    }
 
     constexpr pointer data() const { return mDataBuf; }
     constexpr size_t size() const { return N; }
     constexpr bool empty() const { return data() == nullptr; }
 
+    constexpr pointer begin() { return mDataBuf; }
+    constexpr pointer end() { return mDataBuf + N; }
+    constexpr const_pointer begin() const { return mDataBuf; }
+    constexpr const_pointer end() const { return mDataBuf + N; }
+
     // Allow data_equal for spans that are over the same type up to const-ness.
     template <class U, typename = std::enable_if_t<std::is_same<std::remove_const_t<T>, std::remove_const_t<U>>::value>>
     bool data_equal(const FixedSpan<U, N> & other) const

src/lib/support/tests/TestSpan.cpp

@@ -173,6 +173,13 @@ static void TestFixedByteSpan(nlTestSuite * inSuite, void * inContext)
     uint8_t arr2[] = { 3, 2, 1 };
     FixedByteSpan<3> s4(arr2);
     NL_TEST_ASSERT(inSuite, !s4.data_equal(s2));
+
+    size_t idx = 0;
+    for (auto & entry : s4)
+    {
+        NL_TEST_ASSERT(inSuite, entry == arr2[idx++]);
+    }
+    NL_TEST_ASSERT(inSuite, idx == 3);
 }
 
 #define NL_TEST_DEF_FN(fn) NL_TEST_DEF("Test " #fn, fn)