From 549b84029c9abf02adb54a2bb60da42e6fcbac3a Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Wed, 14 Dec 2022 23:30:23 -0500
Subject: [PATCH] Fix management of the mNumReportsInFlight count in reporting
 engine.

If a ReadHandler failed out of SendReportData (e.g. because the session it's on
had been marked as defunct), we would increment mNumReportsInFlight and never
decrement it.  After this happened CHIP_IM_MAX_REPORTS_IN_FLIGHT times (4 by
default), we would stop being able to send out any more data reports.

This situation is pretty easy to trigger as follows:

1. Use chip-tool to commission a device with node id 17.
2. Start chip-tool interactive mode.
3. Run the following commands in interactive mode:

   onoff subscribe on-off 0 60 17 1 --keepSubscriptions true
   onoff subscribe on-off 0 60 17 1 --keepSubscriptions true
   onoff subscribe on-off 0 60 17 1 --keepSubscriptions true
   onoff subscribe on-off 0 60 17 1 --keepSubscriptions true
   onoff subscribe on-off 0 2 17 1 --keepSubscriptions true

4. quit interactive mode (Ctrl-C or quit() command).
5. Wait 60 seconds for all the subscriptions to error out.

After this the device will no longer respond with data reports to any
read or subscribe requests.
---
 src/app/ReadHandler.cpp      | 35 ++++++++++++++++++++---------------
 src/app/ReadHandler.h        |  2 ++
 src/app/reporting/Engine.cpp |  4 ++++
 3 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp
index bc7cfacdd3a458..75c76cbaba08ae 100644
--- a/src/app/ReadHandler.cpp
+++ b/src/app/ReadHandler.cpp
@@ -206,6 +206,7 @@ CHIP_ERROR ReadHandler::SendStatusReport(Protocols::InteractionModel::Status aSt
 CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, bool aMoreChunks)
 {
     VerifyOrReturnLogError(IsReportable(), CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrDie(!IsAwaitingReportResponse()); // Should not be reportable!
     if (IsPriming() || IsChunkedReport())
     {
         mSessionHandle.Grab(mExchangeCtx->GetSessionHandle());
@@ -230,27 +231,31 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b
         mCurrentReportsBeginGeneration = InteractionModelEngine::GetInstance()->GetReportingEngine().GetDirtySetGeneration();
     }
     SetStateFlag(ReadHandlerFlags::ChunkedReport, aMoreChunks);
-    bool noResponseExpected = IsType(InteractionType::Read) && !aMoreChunks;
-    if (!noResponseExpected)
-    {
-        MoveToState(HandlerState::AwaitingReportResponse);
-    }
+    bool responseExpected = IsType(InteractionType::Subscribe) || aMoreChunks;
 
     mExchangeCtx->UseSuggestedResponseTimeout(app::kExpectedIMProcessingTime);
-    CHIP_ERROR err =
-        mExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::ReportData, std::move(aPayload),
-                                  Messaging::SendFlags(noResponseExpected ? Messaging::SendMessageFlags::kNone
-                                                                          : Messaging::SendMessageFlags::kExpectResponse));
-    if (err == CHIP_NO_ERROR && noResponseExpected)
-    {
-        InteractionModelEngine::GetInstance()->GetReportingEngine().OnReportConfirm();
-    }
-
+    CHIP_ERROR err = mExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::ReportData, std::move(aPayload),
+                                               responseExpected ? Messaging::SendMessageFlags::kExpectResponse
+                                                                : Messaging::SendMessageFlags::kNone);
     if (err == CHIP_NO_ERROR)
     {
+        if (responseExpected)
+        {
+            MoveToState(HandlerState::AwaitingReportResponse);
+        }
+        else
+        {
+            // Make sure we're not treated as an in-flight report waiting for a
+            // response by the reporting engine.
+            InteractionModelEngine::GetInstance()->GetReportingEngine().OnReportConfirm();
+        }
+
         if (IsType(InteractionType::Subscribe) && !IsPriming())
         {
-            err = RefreshSubscribeSyncTimer();
+            // Ignore the error from RefreshSubscribeSyncTimer.  If we've
+            // successfully sent the message, we need to return success from
+            // this method.
+            RefreshSubscribeSyncTimer();
         }
     }
     if (!aMoreChunks)
diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h
index c4f1472f617263..06e8ed5c9a23b4 100644
--- a/src/app/ReadHandler.h
+++ b/src/app/ReadHandler.h
@@ -252,6 +252,8 @@ class ReadHandler : public Messaging::ExchangeDelegate
      *  @retval #Others If fails to send report data
      *  @retval #CHIP_NO_ERROR On success.
      *
+     *  If an error is returned, the ReadHandler guarantees that it is not in
+     *  a state where it's waiting for a response.
      */
     CHIP_ERROR SendReportData(System::PacketBufferHandle && aPayload, bool aMoreChunks);
 
diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp
index 6e25f59a44f307..96d55fdaeeebf8 100644
--- a/src/app/reporting/Engine.cpp
+++ b/src/app/reporting/Engine.cpp
@@ -848,6 +848,10 @@ CHIP_ERROR Engine::SendReport(ReadHandler * apReadHandler, System::PacketBufferH
     // We can only have 1 report in flight for any given read - increment and break out.
     mNumReportsInFlight++;
     err = apReadHandler->SendReportData(std::move(aPayload), aHasMoreChunks);
+    if (err != CHIP_NO_ERROR)
+    {
+        --mNumReportsInFlight;
+    }
     return err;
 }