Fix crashes trying to commission bridge-app. (#14898)

There were several bugs here, that were exposed by bridge-app because bridge-app defines an endpoint count that is larger than the number of actually defined endpoints, so there are some endpoint indices that do not actually have an endpoint present at them.

Specific issues being fixed:

1) emberAfClusterIndex was using the endpointType at an index without
   checking that anything is defined there, which crashed with a
   null-deref.  The fix for that is to check the endpoint id (which we
   later checked anyway) before working with the endpoint type.

2) emberAfClusterIndexInMatchingEndpoints had a similar issue to
   emberAfClusterIndex, but is unused, so was just removed instead of
   fixing it.

3) With those issues fixed, we still crashed because the
   path-expansion iterator would try to expand wildcards across
   endpoints that were not defined, which led to us trying to
   emberAfClusterIndex for the invalid-endpoint id, and end up with
   the same null-deref.  The fix there is to actually skip all
   disabled endpoints (whether defined or not), because we don't want
   to expand wildcards out to disabled endpoints anyway.

src/app/AttributePathExpandIterator.cpp

@@ -46,6 +46,7 @@ extern Optional<ClusterId> emberAfGetNthClusterId(chip::EndpointId endpoint, uin
 extern Optional<AttributeId> emberAfGetServerAttributeIdByIndex(chip::EndpointId endpoint, chip::ClusterId cluster,
                                                                 uint16_t attributeIndex);
 extern uint8_t emberAfClusterIndex(EndpointId endpoint, ClusterId clusterId, EmberAfClusterMask mask);
+extern bool emberAfEndpointIndexIsEnabled(uint16_t index);
 
 namespace chip {
 namespace app {
@@ -138,6 +139,12 @@ bool AttributePathExpandIterator::Next()
 
         for (; mEndpointIndex < mEndEndpointIndex; (mEndpointIndex++, mClusterIndex = UINT8_MAX, mAttributeIndex = UINT16_MAX))
         {
+            if (!emberAfEndpointIndexIsEnabled(mEndpointIndex))
+            {
+                // Not an enabled endpoint; skip it.
+                continue;
+            }
+
             EndpointId endpointId = emberAfEndpointFromIndex(mEndpointIndex);
 
             if (mClusterIndex == UINT8_MAX)

src/app/util/attribute-storage.cpp

@@ -702,35 +702,17 @@ const EmberAfCluster * emberAfFindClusterInType(const EmberAfEndpointType * endp
     return NULL;
 }
 
-uint8_t emberAfClusterIndexInMatchingEndpoints(EndpointId endpoint, ClusterId clusterId, EmberAfClusterMask mask)
-{
-    uint8_t ep;
-    uint8_t index = 0xFF;
-    for (ep = 0; ep < emberAfEndpointCount(); ep++)
-    {
-        const EmberAfEndpointType * endpointType = emAfEndpoints[ep].endpointType;
-        if (emberAfFindClusterInType(endpointType, clusterId, mask) != NULL)
-        {
-            index++;
-            if (emAfEndpoints[ep].endpoint == endpoint)
-            {
-                return index;
-            }
-        }
-    }
-    return 0xFF;
-}
-
 uint8_t emberAfClusterIndex(EndpointId endpoint, ClusterId clusterId, EmberAfClusterMask mask)
 {
-    uint8_t ep;
-    uint8_t index = 0xFF;
-    for (ep = 0; ep < emberAfEndpointCount(); ep++)
+    for (uint8_t ep = 0; ep < emberAfEndpointCount(); ep++)
     {
-        const EmberAfEndpointType * endpointType = emAfEndpoints[ep].endpointType;
-        if (emberAfFindClusterInType(endpointType, clusterId, mask, &index) != NULL)
+        // Check the endpoint id first, because that way we avoid examining the
+        // endpoint type for endpoints that are not actually defined.
+        if (emAfEndpoints[ep].endpoint == endpoint)
         {
-            if (emAfEndpoints[ep].endpoint == endpoint)
+            const EmberAfEndpointType * endpointType = emAfEndpoints[ep].endpointType;
+            uint8_t index                            = 0xFF;
+            if (emberAfFindClusterInType(endpointType, clusterId, mask, &index) != NULL)
             {
                 return index;
             }

src/app/util/attribute-storage.h

@@ -145,20 +145,6 @@ const EmberAfEndpointType * emberAfFindEndpointType(chip::EndpointId endpointId)
 const EmberAfCluster * emberAfFindClusterInType(const EmberAfEndpointType * endpointType, chip::ClusterId clusterId,
                                                 EmberAfClusterMask mask, uint8_t * index = nullptr);
 
-// For a given cluster and mask, retrieves the list of endpoints sorted by endpoint that contain the matching cluster and returns
-// the index within that list that matches the given endpoint.
-//
-// Mask is either CLUSTER_MASK_CLIENT or CLUSTER_MASK_SERVER
-// For example, if you have 3 endpoints, 10, 11, 12, and cluster X server is
-// located on 11 and 12, and cluster Y server is located only on 10 then
-//    clusterIndex(X,11,CLUSTER_MASK_SERVER) returns 0,
-//    clusterIndex(X,12,CLUSTER_MASK_SERVER) returns 1,
-//    clusterIndex(X,10,CLUSTER_MASK_SERVER) returns 0xFF
-//    clusterIndex(Y,10,CLUSTER_MASK_SERVER) returns 0
-//    clusterIndex(Y,11,CLUSTER_MASK_SERVER) returns 0xFF
-//    clusterIndex(Y,12,CLUSTER_MASK_SERVER) returns 0xFF
-uint8_t emberAfClusterIndexInMatchingEndpoints(chip::EndpointId endpoint, chip::ClusterId clusterId, EmberAfClusterMask mask);
-
 //
 // Given a cluster ID, endpoint ID and a cluster mask, finds a matching cluster within that endpoint
 // with a matching mask. If one is found, the relative index of that cluster within the list of clusters on that
@@ -239,6 +225,8 @@ void emberAfClusterMessageSentCallback(const chip::MessageSendDestination & dest
 // returns true if the mask matches a passed interval
 bool emberAfCheckTick(EmberAfClusterMask mask, uint8_t passedMask);
 
+// Check whether there is an endpoint defined with the given endpoint id that is
+// enabled.
 bool emberAfEndpointIsEnabled(chip::EndpointId endpoint);
 
 // Note the difference in implementation from emberAfGetNthCluster().

src/app/util/mock/attribute-storage.cpp

@@ -212,6 +212,11 @@ uint8_t emberAfClusterIndex(chip::EndpointId endpoint, chip::ClusterId cluster,
     return UINT8_MAX;
 }
 
+bool emberAfEndpointIndexIsEnabled(uint16_t index)
+{
+    return index < ArraySize(endpoints);
+}
+
 // This duplication of basic utilities is really unfortunate, but we can't link
 // to the normal attribute-storage.cpp because we redefine some of its symbols
 // above.