Fix CHIPDeviceCallbacksMgr callback info storage (#7842)

The code in CHIPDeviceCallbacksMgr is storing information about each
registered callback in some inline space reserved for this purpose.

However, the space consists of two separate scalar fields (mInfoPtr and
mInfoScalar). Type punning these fields as a struct is undefined
behavior in C++. In fact these structures do not even fit on 32 bit
platforms without relying on padding that's added between the fields.

GCC can diagnose the invalid casts used here and issues the following
diagnostic (unless strict aliasing is disabled, which it is currently in
all builds except ESP32):

../third_party/connectedhomeip/src/app/util/CHIPDeviceCallbacksMgr.h: In instantiation of 'CHIP_ERROR chip::app::CHIPDeviceCallbacksMgr::GetCallback(T&, chip::Callback::CallbackDeque&, chip::Callback::Cancelable**) [with T = {anonymous}::ReportCallbackInfo; CHIP_ERROR = int]':
../third_party/connectedhomeip/src/app/util/CHIPDeviceCallbacksMgr.cpp:123:5:   required from here
../third_party/connectedhomeip/src/app/util/CHIPDeviceCallbacksMgr.h:84:52: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

Use a character array instead and add a static_assert that the data
fits. Placing structures in character arrays is legal as C++ has
an explicit carve-out for this case.

src/app/tests/BUILD.gn

@@ -22,6 +22,7 @@ chip_test_suite("tests") {
   output_name = "libAppTests"
 
   test_sources = [
+    "TestCHIPDeviceCallbacksMgr.cpp",
     "TestClusterInfo.cpp",
     "TestCommandInteraction.cpp",
     "TestCommandPathParams.cpp",
@@ -38,6 +39,7 @@ chip_test_suite("tests") {
 
   public_deps = [
     "${chip_root}/src/app",
+    "${chip_root}/src/app/util:device_callbacks_manager",
     "${chip_root}/src/lib/core",
     "${chip_root}/src/protocols",
     "${nlunit_test_root}:nlunit-test",

src/app/util/BUILD.gn

@@ -0,0 +1,29 @@
+# Copyright (c) 2021 Project CHIP Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import("//build_overrides/chip.gni")
+
+source_set("device_callbacks_manager") {
+  sources = [
+    "CHIPDeviceCallbacksMgr.cpp",
+    "CHIPDeviceCallbacksMgr.h",
+  ]
+
+  public_deps = [
+    "${chip_root}/src/lib/core",
+    "${chip_root}/src/lib/support",
+  ]
+
+  cflags = [ "-Wconversion" ]
+}

src/app/util/CHIPDeviceCallbacksMgr.cpp

@@ -64,8 +64,9 @@ CHIP_ERROR CHIPDeviceCallbacksMgr::AddResponseCallback(NodeId nodeId, uint8_t se
     VerifyOrReturnError(onFailureCallback != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
     ResponseCallbackInfo info = { nodeId, sequenceNumber };
-    memcpy(&onSuccessCallback->mInfoPtr, &info, sizeof(info));
-    memcpy(&onFailureCallback->mInfoPtr, &info, sizeof(info));
+    static_assert(sizeof(onSuccessCallback->mInfo) >= sizeof(info), "Callback info too large");
+    memcpy(&onSuccessCallback->mInfo, &info, sizeof(info));
+    memcpy(&onFailureCallback->mInfo, &info, sizeof(info));
 
     // If some callbacks have already been registered for the same ResponseCallbackInfo, it usually means that the response
     // has not been received for a previous command with the same sequenceNumber. Cancel the previously registered callbacks.
@@ -106,7 +107,8 @@ CHIP_ERROR CHIPDeviceCallbacksMgr::AddReportCallback(NodeId nodeId, EndpointId e
     VerifyOrReturnError(onReportCallback != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
     ReportCallbackInfo info = { nodeId, endpointId, clusterId, attributeId };
-    memmove(&onReportCallback->mInfoPtr, &info, sizeof(info));
+    static_assert(sizeof(onReportCallback->mInfo) >= sizeof(info), "Callback info too large");
+    memcpy(&onReportCallback->mInfo, &info, sizeof(info));
 
     // If a callback has already been registered for the same ReportCallbackInfo, let's cancel it.
     CancelCallback(info, mReports);

src/app/util/CHIPDeviceCallbacksMgr.h

@@ -26,6 +26,8 @@
 
 #pragma once
 
+#include <type_traits>
+
 #include <app/util/basic-types.h>
 #include <core/CHIPCallback.h>
 #include <core/CHIPError.h>
@@ -81,7 +83,11 @@ class DLL_EXPORT CHIPDeviceCallbacksMgr
         Callback::Cancelable * ca = &queue;
         while (ca != nullptr && ca->mNext != &queue)
         {
-            if (*reinterpret_cast<T *>(&ca->mNext->mInfoPtr) == info)
+            T callbackInfo;
+            static_assert(std::is_pod<T>::value, "Callback info must be POD");
+            static_assert(sizeof(ca->mNext->mInfo) >= sizeof(callbackInfo), "Callback info too large");
+            memcpy(&callbackInfo, ca->mNext->mInfo, sizeof(callbackInfo));
+            if (callbackInfo == info)
             {
                 *callback = ca->mNext;
                 return CHIP_NO_ERROR;

src/controller/BUILD.gn

@@ -18,7 +18,6 @@ static_library("controller") {
   output_name = "libChipController"
 
   sources = [
-    "${chip_root}/src/app/util/CHIPDeviceCallbacksMgr.cpp",
     "AbstractMdnsDiscoveryController.cpp",
     "CHIPCluster.cpp",
     "CHIPCluster.h",
@@ -38,6 +37,7 @@ static_library("controller") {
 
   public_deps = [
     "${chip_root}/src/app",
+    "${chip_root}/src/app/util:device_callbacks_manager",
     "${chip_root}/src/lib/core",
     "${chip_root}/src/lib/mdns",
     "${chip_root}/src/lib/support",

src/lib/core/CHIPCallback.h

@@ -49,9 +49,7 @@ class Cancelable
      */
     Cancelable * mNext;
     Cancelable * mPrev;
-
-    void * mInfoPtr;
-    uint64_t mInfoScalar;
+    alignas(uint64_t) char mInfo[16];
 
     /**
      * @brief when non-null, indicates the Callback is registered with

src/system/SystemLayer.cpp

@@ -41,6 +41,7 @@
 
 // Include system and language headers
 #include <stddef.h>
+#include <string.h>
 
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
 #include <errno.h>
@@ -67,6 +68,24 @@
 namespace chip {
 namespace System {
 
+namespace {
+
+Timer::Epoch GetTimerEpoch(const Callback::Cancelable * timer)
+{
+    Timer::Epoch timerEpoch;
+    static_assert(sizeof(timerEpoch) <= sizeof(timer->mInfo), "mInfo is too small for timer epoch");
+    memcpy(&timerEpoch, &timer->mInfo, sizeof(timerEpoch));
+    return timerEpoch;
+}
+
+void SetTimerEpoch(Callback::Cancelable * timer, Timer::Epoch timerEpoch)
+{
+    static_assert(sizeof(timerEpoch) <= sizeof(timer->mInfo), "mInfo is too small for timer epoch");
+    memcpy(&timer->mInfo, &timerEpoch, sizeof(timerEpoch));
+}
+
+} // namespace
+
 using namespace ::chip::Callback;
 
 #if CHIP_SYSTEM_CONFIG_USE_LWIP
@@ -227,13 +246,17 @@ Error Layer::NewTimer(Timer *& aTimerPtr)
 
 static bool TimerReady(const Timer::Epoch epoch, const Cancelable * timer)
 {
-    return !Timer::IsEarlierEpoch(epoch, timer->mInfoScalar);
+    return !Timer::IsEarlierEpoch(epoch, GetTimerEpoch(timer));
 }
 
 static int TimerCompare(void * p, const Cancelable * a, const Cancelable * b)
 {
     (void) p;
-    return (a->mInfoScalar > b->mInfoScalar) ? 1 : (a->mInfoScalar < b->mInfoScalar) ? -1 : 0;
+
+    Timer::Epoch epochA = GetTimerEpoch(a);
+    Timer::Epoch epochB = GetTimerEpoch(b);
+
+    return (epochA > epochB) ? 1 : (epochA < epochB) ? -1 : 0;
 }
 
 /**
@@ -266,7 +289,7 @@ void Layer::StartTimer(uint32_t aMilliseconds, chip::Callback::Callback<> * aCal
 
     Cancelable * ca = aCallback->Cancel();
 
-    ca->mInfoScalar = Timer::GetCurrentEpoch() + aMilliseconds;
+    SetTimerEpoch(ca, Timer::GetCurrentEpoch() + aMilliseconds);
 
     mTimerCallbacks.InsertBy(ca, TimerCompare, nullptr);
 
@@ -633,10 +656,10 @@ bool Layer::GetTimeout(struct timeval & aSleepTime)
     if (lAwakenEpoch != kCurrentEpoch)
     {
         Cancelable * ca = mTimerCallbacks.First();
-        if (ca != nullptr && !Timer::IsEarlierEpoch(kCurrentEpoch, ca->mInfoScalar))
+        if (ca != nullptr && !Timer::IsEarlierEpoch(kCurrentEpoch, GetTimerEpoch(ca)))
         {
             anyTimer     = true;
-            lAwakenEpoch = ca->mInfoScalar;
+            lAwakenEpoch = GetTimerEpoch(ca);
         }
     }