[C++] Error checking for repeating groups

[ksergey]

Hi devs!

wrapForDecode method of a repeating group should validate input. In case of m_blockLength == 0 the method next() will always return the same group entry.

Could you please add to c++ generator check which will throw exception (optionally, like SBE_BOUNDS_CHECK_EXPECT) in case of m_blockLength==0?

Thanks

[tmontgomery]

Do you have a valid XML and test that demonstrates this not working?

[ksergey]

Minimal example

schema.xml:

<?xml version="1.0" encoding="UTF-8"?>
<sbe:messageSchema xmlns:sbe="http://fixprotocol.io/2016/sbe"
                   xmlns:xi="http://www.w3.org/2001/XInclude"
                   package="baseline"
                   id="1"
                   version="0"
                   semanticVersion="5.2"
                   description="Example base schema which can be extended."
                   byteOrder="littleEndian">
    <types>
        <composite name="messageHeader" description="Message identifiers and length of message root.">
            <type name="blockLength" primitiveType="uint16"/>
            <type name="templateId" primitiveType="uint16"/>
            <type name="schemaId" primitiveType="uint16"/>
            <type name="version" primitiveType="uint16"/>
        </composite>
        <composite name="groupSizeEncoding" description="Repeating group dimensions.">
            <type name="blockLength" primitiveType="uint8"/>
            <type name="numInGroup" primitiveType="uint8"/>
        </composite>
    </types>
    <sbe:message name="Message" id="1">
        <group name="Group1" id="1" dimensionType="groupSizeEncoding">
            <field name="value" id="1" type="uint8"/>
        </group>
        <group name="Group2" id="2" dimensionType="groupSizeEncoding">
            <field name="value" id="1" type="uint8"/>
        </group>
    </sbe:message>
</sbe:messageSchema>

java -Dsbe.generate.ir=false -Dsbe.target.language=cpp -Dsbe.xinclude.aware=true -jar sbe-all-1.18.2.jar schema.xml

main.cpp

#include "baseline/Message.h"

#include <iostream>

int main([[maybe_unused]] int argc, [[maybe_unused]] char *argv[]) {
  char buffer[13] = {};

  {
    baseline::Message msg;
    msg.wrapForEncode(buffer, 0, sizeof(buffer));

    auto grp1 = msg.group1Count(5);
    grp1.next().value(1);
    grp1.next().value(2);

    auto grp2 = msg.group2Count(4);
    grp2.next().value(1);
    grp2.next().value(0);
    grp2.next().value(250);
    grp2.next().value(1);
  }

  {
    baseline::Message msg;
    msg.wrapForDecode(buffer, 0, baseline::Message::sbeBlockLength(),
                      baseline::Message::sbeSchemaVersion(), sizeof(buffer));

    std::cout << msg << '\n';
  }

  return EXIT_SUCCESS;
}

g++ -std=c++17 main.cpp -o test

[mjpt777]

There is a bug in this example.

    auto grp1 = msg.group1Count(5);

The argument should be 2.

[mjpt777]

Also your buffer length is 1 byte too small and as a result the stack is getting corrupt.

I don't see the issue other than a buggy sample.

[ksergey]

Yes, there is bug on encoder side. How to be safe on decoder side? I can't control encoder side. Could you please at least provide access to m_blockLength inside repeating group?

[tmontgomery]

The BoundsCheckTest does check all lengths and mis-set groups, etc. just by how it works. By checking all lengths up to the real length. This catches the situation of a group size set too big compared to the buffer.

If you have a valid test that shows the issue, we will take a look and see what the best solution might be.

[mjpt777]

If garbage is encoded then you cannot expect to decode it correctly.

The block length for a group is provided as a constant. The acting block length could be made available. However none of our support customers are asking for this and I'm hesitant to avoid expanding the API without good reason.

[ksergey]

@mjpt777 fill free to close

But I don't understand why buffer bound check is fine for you, but checking group block length not.

[mjpt777]

@ksergey There are many checks we could add and techniques to be applied as part of a full comms strategy. The work we do is lead by our customers. We are happy to work with people who contribute back by some means. If the means is not commercial then they should contribute to the project.

You initially asked for a zero check on block length. A zero check on the message header provides similar validation.