From 1516956056a014864afd7062e138a06e1df119f2 Mon Sep 17 00:00:00 2001
From: Michael Spang <spang@google.com>
Date: Tue, 12 Sep 2023 16:52:59 -0400
Subject: [PATCH] Make ReadClient::Callback non-movable (#29191)

The purpose of this interface is be called polymorphically through a
pointer. Moving it will invalidate the original instance and is very
likely to be a bug.

Make this type non-movable; this will fail compilation if a derived
class is stored in an inappropriate container that moves its elements.

As an example, ClusterStateCache is also currently movable, but moving
it retains a pointer to the moved-from instance via mBufferedReader, so
any further usage is likely to result in a invalid memory access.
---
 src/app/ClusterStateCache.h | 13 +++++++++++++
 src/app/ReadClient.cpp      |  1 -
 src/app/ReadClient.h        |  8 ++++++++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/app/ClusterStateCache.h b/src/app/ClusterStateCache.h
index 9e08abacde7768..2a96a949193f4f 100644
--- a/src/app/ClusterStateCache.h
+++ b/src/app/ClusterStateCache.h
@@ -71,6 +71,14 @@ class ClusterStateCache : protected ReadClient::Callback
     class Callback : public ReadClient::Callback
     {
     public:
+        Callback() = default;
+
+        // Callbacks are not expected to be copyable or movable.
+        Callback(const Callback &) = delete;
+        Callback(Callback &&)      = delete;
+        Callback & operator=(const Callback &) = delete;
+        Callback & operator=(Callback &&) = delete;
+
         /*
          * Called anytime an attribute value has changed in the cache
          */
@@ -103,6 +111,11 @@ class ClusterStateCache : protected ReadClient::Callback
         mHighestReceivedEventNumber = highestReceivedEventNumber;
     }
 
+    ClusterStateCache(const ClusterStateCache &) = delete;
+    ClusterStateCache(ClusterStateCache &&)      = delete;
+    ClusterStateCache & operator=(const ClusterStateCache &) = delete;
+    ClusterStateCache & operator=(ClusterStateCache &&) = delete;
+
     void SetHighestReceivedEventNumber(EventNumber highestReceivedEventNumber)
     {
         mHighestReceivedEventNumber.SetValue(highestReceivedEventNumber);
diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp
index c9df6638b708ef..caac866aeba08c 100644
--- a/src/app/ReadClient.cpp
+++ b/src/app/ReadClient.cpp
@@ -45,7 +45,6 @@ ReadClient::ReadClient(InteractionModelEngine * apImEngine, Messaging::ExchangeM
     mOnConnectionFailureCallback(HandleDeviceConnectionFailure, this)
 {
     mpExchangeMgr    = apExchangeMgr;
-    mpCallback       = apCallback;
     mInteractionType = aInteractionType;
 
     mpImEngine = apImEngine;
diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h
index 71844713ff1461..85bcd124e5190f 100644
--- a/src/app/ReadClient.h
+++ b/src/app/ReadClient.h
@@ -72,6 +72,14 @@ class ReadClient : public Messaging::ExchangeDelegate
     class Callback
     {
     public:
+        Callback() = default;
+
+        // Callbacks are not expected to be copyable or movable.
+        Callback(const Callback &) = delete;
+        Callback(Callback &&)      = delete;
+        Callback & operator=(const Callback &) = delete;
+        Callback & operator=(Callback &&) = delete;
+
         virtual ~Callback() = default;
 
         /**