CircularEventBufferWrapper::GetNextBuffer does not seem to handle lookahead via reader copies correctly

[bzbarsky-apple]

Problem

A typical pattern we have in our code is that a new TLV reader (B) is initialized with an existing TLV reader (A), and then is advanced in various ways to figure out something about the state of the TLV that A has not read yet.

When A is backed by a TLVBackingStore, B ends up using the exact same backing store object. This means that the backing store GetNextBuffer must not modify any state of the store for this setup to work when the read-ahead crosses a buffer boundary. Otherwise advancing B will change the state, and then going back to A and trying to read the data will not get the right data.

Proposed Solution

Need to either fix CircularEventBufferWrapper to not mutate any state or carefully audit whether any of the readers that use CircularEventBufferWrapper are ever used (including internally within TLVReader, e.g. via TLVReader::FindElementWithTag) in the above-described pattern.

@yunhanw-google @mrjerryjohns

[mrjerryjohns]

@robszewczyk @yunhanw-google as part of implementing eventing on the server side, please validate this.

[yunhanw-google]

for CircularEventBufferWrapper, I guess your concern is CircularEventBufferWrapper may modify the CHIPCircularTLVBuffer or TLVBackingStore state by GetNextBuffer,
CHIP_ERROR CircularEventBufferWrapper::GetNextBuffer(TLVReader & aReader, const uint8_t *& aBufStart, uint32_t & aBufLen)

When reviewing the logic,
CHIPCircularTLVBuffer::GetNextBuffer(TLVReader & ioReader, const uint8_t *& outBufStart, uint32_t & outBufLen), it seems this function does not mutate state for current tlv.

[bzbarsky-apple]

Good catch. Looks like it indeed does not.

Might be good to have a const method that gets called from GetNextBuffer for all the real work, so we can have the compiler enforce the "does not mutate any of our members" requirement...

[yunhanw-google]

agree, actually GetNextBuffer is overriable method, for TLVReader that use CHIP_ERROR TLVPacketBufferBackingStore::GetNextBuffer(chip::TLV::TLVReader & reader, const uint8_t *& bufStart, uint32_t & bufLen), it do modify the state.
But for CHIPCircularTLVBuffer, as you see, GetNextBuffer does not modify the state, since GetNextBuffer is overridable and used by circular one and non-circular, so we cannot mark it as const.

[yunhanw-google]

would close it now. let me if any further concern, many thanks.