[C++] Possible bug in decodeLength()

[szymonwieloch]

I believe that you code generator has a bug in the decodeLength() method of messages. Inside this method a new temporal instance of the message class is created in order to iterate over elements and provide length of the message (actually part of it that is understood by the parser). It goes like this:

MessageType skipper(m_buffer, m_offset, m_bufferLength, sbeBlockLength(), m_actingVersion, m_codecState); 

As you can see, I am currently using precedence checks and it's possible that the bug is limited to this version.
The purpose of creating this temporal instance is to have a "fresh" instance that you can use to skip over variable length fields.

The problem is with two parts of this code:

1. Call to sbeBlockLength() instead of using m_actingBlockLength.
2. Passing m_codecState

Problem 1 is about passing a constant block length characteristic to the version of schema that was used by code generator. Instead the value from the received header should be used. This violates the forward compatibility paradigm as the received message may have a longer block length. To reproduce problem 1:

• Create a schema version 1 with few fields and use the generator to generate the code.
• Update the schema to version 2 and add few more fields. Generate a code for it.
• Generate a message using schema 2 code and parse it with schema 1 code.
• Call the decodeLength() method. Expect completely corrupted result.

Problem 2 is about codec state. Whenever you completely reset the parser, you should also reset the codec state. Otherwise precedence checks will throw and exception in a completely correct scenario. To reproduce:

• Create a schema with few <data> fields
• Wrap message for decode.
• Read the first <data> field. This will change the codec state.
• Call the decodeLength() method. Precedence checks will make it throw because the temporal instance is in an invalid state.

Suggestion for fixing

Since creation of a "fresh" instance exists in few places (for example operator << also creates a new temporal instance) I suggest wrapping this piece of code inside a new method to reduce duplication. You already have the sbeRewind() method but the new one needs to return a new instance of the message class, without affecting the original instance:

SBE_NODISCARD MessageType sbeReset() {
    return MessageType(
    m_buffer,
    m_offset,
    m_bufferLength,
    m_actingBlockLength,
    m_actingVersion
#if defined(SBE_PRECEDENCE_CHECKS)
    ,CodecState::V0_BLOCK
#endif
);
}

[szymonwieloch]

Actually there seems to be a 3rd problem: precedence checks should not allow call to this function in an unwrapped state.

[vyazelenko]

/cc @ZachBray

[ZachBray]

Good spot.

Regarding Problem 1, it looks like the incorrect block length has been used since the decodeLength() method was added in 2020.

Regarding Problem 2, I believe the generator no longer passes m_codecState since the recent fix in a similar area where I removed that constructor.

@szymonwieloch, if you are happy to raise a PR with a fix, please go ahead. Otherwise, I'll pick this up when I get some time.

[ZachBray]

@szymonwieloch, in case you'd like to raise a PR and get a fix soon, we're planning to release a minor version of SBE within the next week.

If you do raise a PR, please add a test mimicking your reproduction steps.

[vyazelenko]

Fixed in #1045.