Skip to content

Commit

Permalink
Check for too-large events before we modify our buffer state. (projec…
Browse files Browse the repository at this point in the history
…t-chip#25257)

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.
  • Loading branch information
bzbarsky-apple authored Feb 23, 2023
1 parent ebc08a5 commit b236bfb
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 26 deletions.
38 changes: 16 additions & 22 deletions src/app/EventManagement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -165,21 +165,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;
Expand Down Expand Up @@ -415,7 +426,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;
Timestamp timestamp;
Expand Down Expand Up @@ -449,28 +459,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:
Expand Down
15 changes: 11 additions & 4 deletions src/app/EventManagement.h
Original file line number Diff line number Diff line change
Expand Up @@ -436,13 +436,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
Expand Down

0 comments on commit b236bfb

Please sign in to comment.