Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] stringop-overflow warning in generateConstPropertyMethods output #832

Closed
neomantra opened this issue Dec 28, 2020 · 3 comments
Closed

Comments

@neomantra
Copy link
Contributor

While building SBE-generated C++ code for <group> fields, I see the following compiler warning for code generated from Group->Field elements with a char primitive type:

warning: 'strlen' argument missing terminating nul [-Wstringop-overflow=]

The generated code block looks like this:

class MDIncrementalRefreshTradeSummary42
{
	.....
    class NoMDEntries
    {
        .....
        SBE_NODISCARD const char *mDEntryType() const
        {
            static const std::uint8_t mDEntryTypeValues[] = { 50 };

            return (const char *)mDEntryTypeValues;
        }

Inspecting the situation, I see that:

  • SBE-message end-users should use length functions like mDEntryTypeLength()
  • The return value of mDEntryType() truly a pointer to a single char, not a NULL-terminated string.

So in that regard, this is not a SBE codegen bug. But, unfortunately any C++ programmer can pass that const char* to a myriad of functions that expect the input to be NULL terminated. This could lead to inadvertent out-of-bounds / buffer-overrun bugs.

Adding a NULL to the static array in that function would mitigate this class of bugs at small expense of static storage. So something like this at this line:

-            indent + "        static const std::uint8_t %1$sValues[] = { %2$s };\n\n" +
+            indent + "        static const std::uint8_t %1$sValues[] = { %2$s, 0 };\n\n" +

While I'm here, I'd also like to thank you for all your fantastic contributions and thought leadership to both software engineering at-large and particularly in the trading community. 🤗

@mjpt777
Copy link
Contributor

mjpt777 commented Dec 28, 2020

Can you post the snippet of the schema for the field?

@neomantra
Copy link
Contributor Author

It is the MDEntryType field of NoGroupEntries group of MDIncrementalRefreshTradeSummary42 messages from this
CME schema:

ftp://ftp.cmegroup.com/SBEFix/Production/Templates/templates_FixBinary.xml

    <ns2:message name="MDIncrementalRefreshTradeSummary42" id="42" description="MDIncrementalRefreshTradeSummary" blockLength="11" semanticType="X" sinceVersion="5">
        <field name="TransactTime" id="60" type="uInt64" description="Start of event processing time in number of nanoseconds since Unix epoch" offset="0" semanticType="UTCTimestamp"/>
        <field name="MatchEventIndicator" id="5799" type="MatchEventIndicator" description="Bitmap field of eight Boolean type indicators reflecting the end of updates for a given Globex event" offset="8" semanticType="MultipleCharValue"/>
        <group name="NoMDEntries" id="268" description="Number of Trade Summary entries" blockLength="32" dimensionType="groupSize">
            <field name="MDEntryPx" id="270" type="PRICE" description="Trade price" offset="0" semanticType="Price"/>
            <field name="MDEntrySize" id="271" type="Int32" description="Consolidated trade quantity" offset="8" semanticType="Qty"/>
            <field name="SecurityID" id="48" type="Int32" description="Security ID as defined by CME" offset="12" semanticType="int"/>
            <field name="RptSeq" id="83" type="uInt32" description="Sequence number per instrument update" offset="16" semanticType="int"/>
            <field name="NumberOfOrders" id="346" type="Int32NULL" description="The total number of real orders per instrument that participated in a match step within a match event" offset="20" semanticType="int"/>
            <field name="AggressorSide" id="5797" type="AggressorSide" description="Indicates which side is the aggressor or if there is no aggressor" offset="24" semanticType="int"/>
            <field name="MDUpdateAction" id="279" type="MDUpdateAction" description="Market Data update action" offset="25" semanticType="int"/>
            <field name="MDEntryType" id="269" type="MDEntryTypeTrade" description="Market Data entry type" semanticType="char"/>
            <field name="MDTradeEntryID" id="37711" type="uInt32NULL" description="Market Data Trade entry ID" offset="26" sinceVersion="7" semanticType="int"/>
        </group>
        <group name="NoOrderIDEntries" id="37705" description="Number of OrderID entries" blockLength="16" dimensionType="groupSize8Byte">
            <field name="OrderID" id="37" type="uInt64" description="Unique order identifier as assigned by the exchange" offset="0" semanticType="int"/>
            <field name="LastQty" id="32" type="Int32" description="Quantity bought or sold on this last fill" offset="8" semanticType="Qty"/>
        </group>
    </ns2:message>

@mjpt777
Copy link
Contributor

mjpt777 commented Dec 30, 2020

For constant values I can kind of see a point here. However this issue would exist regardless for required values if the user filled all values of the array with chars and did not put in a null termination. Adding the null terminator like you suggest might be the safest way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants