Skip to content

[C++] Possible bug in decodeLength() #1044

Closed
@szymonwieloch

Description

@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
);
}

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions