From c06519de4326529edd1db6a8b1164c726825d1a3 Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Wed, 22 Feb 2023 13:46:18 -0500
Subject: [PATCH] Check for too-large events before we modify our buffer state.

We could end up in a situation where an INFO event that fit into the DEBUG
buffer but did not fit into the INFO buffer would get written to the buffer, and
possibly evict some other events, but then we would error out from
LogEventPrivate, and leave ourselves in an inconsistent state.

The fix is to do the "this event fits in all the buffers that it might need to
fit in" check before we start modifying any state.
---
 src/app/EventManagement.cpp | 38 ++++++++++++++++---------------------
 src/app/EventManagement.h   | 15 +++++++++++----
 2 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/src/app/EventManagement.cpp b/src/app/EventManagement.cpp
index a4777e0b1b471b..49e988f4c5c450 100644
--- a/src/app/EventManagement.cpp
+++ b/src/app/EventManagement.cpp
@@ -162,21 +162,32 @@ CHIP_ERROR EventManagement::CopyToNextBuffer(CircularEventBuffer * apEventBuffer
     return err;
 }
 
-CHIP_ERROR EventManagement::EnsureSpaceInCircularBuffer(size_t aRequiredSpace)
+CHIP_ERROR EventManagement::EnsureSpaceInCircularBuffer(size_t aRequiredSpace, PriorityLevel aPriority)
 {
     CHIP_ERROR err                    = CHIP_NO_ERROR;
     size_t requiredSpace              = aRequiredSpace;
     CircularEventBuffer * eventBuffer = mpEventBuffer;
     ReclaimEventCtx ctx;
 
+    // Check that we have this much space in all our event buffers that might
+    // hold the event. If we do not, that will prevent the event from being
+    // properly evicted into higher-priority bufers.  We want to discover
+    // this early, so that testing surfaces the need to make those buffers
+    // larger.
+    for (auto * currentBuffer = mpEventBuffer; currentBuffer; currentBuffer = currentBuffer->GetNextCircularEventBuffer())
+    {
+        VerifyOrExit(requiredSpace <= currentBuffer->GetTotalDataLength(), err = CHIP_ERROR_BUFFER_TOO_SMALL);
+        if (currentBuffer->IsFinalDestinationForPriority(aPriority))
+        {
+            break;
+        }
+    }
+
     // check whether we actually need to do anything, exit if we don't
     VerifyOrExit(requiredSpace > eventBuffer->AvailableDataLength(), err = CHIP_NO_ERROR);
 
     while (true)
     {
-        // check that the request can ultimately be satisfied.
-        VerifyOrExit(requiredSpace <= eventBuffer->GetTotalDataLength(), err = CHIP_ERROR_BUFFER_TOO_SMALL);
-
         if (requiredSpace > eventBuffer->AvailableDataLength())
         {
             ctx.mpEventBuffer             = eventBuffer;
@@ -408,7 +419,6 @@ CHIP_ERROR EventManagement::LogEventPrivate(EventLoggingDelegate * apDelegate, c
     uint32_t requestSize         = 0;
     aEventNumber                 = 0;
     CircularTLVWriter checkpoint = writer;
-    CircularEventBuffer * buffer = nullptr;
     EventLoadOutContext ctxt     = EventLoadOutContext(writer, aEventOptions.mPriority, mLastEventNumber);
     EventOptions opts;
 #if CHIP_CONFIG_EVENT_LOGGING_UTC_TIMESTAMPS & CHIP_SYSTEM_CONFIG_PLATFORM_PROVIDES_TIME
@@ -440,28 +450,12 @@ CHIP_ERROR EventManagement::LogEventPrivate(EventLoggingDelegate * apDelegate, c
     SuccessOrExit(err);
 
     // Ensure we have space in the in-memory logging queues
-    err = EnsureSpaceInCircularBuffer(requestSize);
+    err = EnsureSpaceInCircularBuffer(requestSize, aEventOptions.mPriority);
     SuccessOrExit(err);
 
     err = ConstructEvent(&ctxt, apDelegate, &opts);
     SuccessOrExit(err);
 
-    // Check the number of bytes written.  If the event is too large
-    // to be evicted from subsequent buffers, drop it now.
-    buffer = mpEventBuffer;
-    while (true)
-    {
-        VerifyOrExit(buffer->GetTotalDataLength() >= writer.GetLengthWritten(), err = CHIP_ERROR_BUFFER_TOO_SMALL);
-        if (buffer->IsFinalDestinationForPriority(opts.mPriority))
-        {
-            break;
-        }
-
-        buffer = buffer->GetNextCircularEventBuffer();
-        assert(buffer != nullptr);
-        // code guarantees that every PriorityLevel has a buffer destination.
-    }
-
     mBytesWritten += writer.GetLengthWritten();
 
 exit:
diff --git a/src/app/EventManagement.h b/src/app/EventManagement.h
index 4a07ae341b1fe6..3d42532e8c18ff 100644
--- a/src/app/EventManagement.h
+++ b/src/app/EventManagement.h
@@ -403,13 +403,20 @@ class EventManagement
     CHIP_ERROR CopyToNextBuffer(CircularEventBuffer * apEventBuffer);
 
     /**
-     * @brief eusure current buffer has enough space, if not, when current buffer is final destination of last tail's event
-     * priority, we need to drop event, otherwises, move the last event to the buffer with higher priority
+     * @brief Ensure that:
      *
-     * @param[in] aRequiredSpace  require space
+     * 1) There could be aRequiredSpace bytes available (if enough things were
+     *    evicted) in all buffers that can hold events with priority aPriority.
+     *
+     * 2) There are in fact aRequiredSpace bytes available in our
+     *    lowest-priority buffer.  This might involve evicting some events to
+     *    higher-priority buffers or dropping them.
+     *
+     * @param[in] aRequiredSpace  required space
+     * @param[in] aPriority       priority of the event we are making space for.
      *
      */
-    CHIP_ERROR EnsureSpaceInCircularBuffer(size_t aRequiredSpace);
+    CHIP_ERROR EnsureSpaceInCircularBuffer(size_t aRequiredSpace, PriorityLevel aPriority);
 
     /**
      * @brief Iterate the event elements inside event tlv and mark the fabric index as kUndefinedFabricIndex if