Undefined behaviour on "out of range" values of ID layer.

[melg8]

Setup

1. add compiler flags:
   -fno-omit-frame-pointer
-fno-sanitize-recover=undefined
-fsanitize=undefined
2. generate code using schema with fixed values for ID frame:

...
    <fields>
        <enum name="CommandType" type="uint8" hexAssign="true">
            <validValue name="TestConnection1" val="27" displayName="Test connection to 1" />
            <validValue name="TestConnection2" val="38" displayName="Test connection to 2" />
        </enum>
    </fields>
...
    <frame name="OurProtocolFrame">
        <id name="Id">
            <field>
                <ref field="CommandType" />
            </field>
        </id>
...

3. Setup test where comms::processAllWithDispatch is used to dispatch messages, and incoming input has value of ID is way off, for example 68.
4. Build and run test.

Expected behavior:

Test should pass, without crash, and should notify, that ID value was invalid.

Current behavior:

Test crash with ubsanitizer reporting:
Load of value 68, which is not a valid value for type 'const McuProtocol::MsgId' on line ...
Reporting on line
MsgIdLayer.h#L689

BaseImpl::setMsgId(id, extraValues...)

Specs

Files are "Generated by commsdsl2comms v3.5.2"
compilers:

• gcc version 8.4.0
• clang version 8.0.1

Thoughts

Problem occurs because of assignment (cast) of value to the field with enumeration type, with value 68, where enum only happy with 27 and 38.
Current implementation calls

BaseImpl::setMsgId(id, extraValues...); 

Before validation of this value, which trigger ub.
But, even removing that call will not help by itself, because inside of call

msg = createMsgInternal(id, idx, &failureReason);

assignment is done before check, which correctly detects invalid value inside of DispatchMsgStaticBinSearchHelper.h

We have really strict rules about any signs from static/dynamic analysis tools, so we can't just ignore the fact of ubsan reports - that's a pipeline failure.
Wording from standard about static casting to enumeration with "out of range" value.

. I've tried some ad hoc solutions, but it's not easy to grasp all template meta-programming elements, involved in this.
Would really appreciate any temporal bugfixes, which will eliminates this problem.

PS.

In preparing of this issue I've tried to add same flags into root cmake file of this project, and run ctest after it. Your tests reproduce the same kind of problem, but in other place

1/2 Test commschamp/comms_champion#1: comms.FieldsTest .................***Failed   
comms_champion/comms/include/comms/field/adapter/FixedBitLength.h:127:69: runtime error: load of value 4, which is not a valid value for type 'const comms::field::basic::EnumValue<comms::Field<comms::option::def::Endian<comms::util::traits::endian::Big> >, FieldsTestSuite::Enum1>::ValueType' (aka 'const FieldsTestSuite::Enum1')
2: 
2: CMake Error at comms_champion/cmake/CC_RunWithValgrindScript.cmake:30 (message):
2:   Test failed with exit code 1

@arobenko if you want I can open another issue about sanitizers (address santizier give even more errors) and tests.

But our main focus is on dealing with ID field problem.

[arobenko]

Hi Melg, I'll get to the bottom of this during the next week. No promises about the time line though. It's my side project. I spend about an hour a day on it during my commute time to work and back. I suppose even if UB according to the standard does take place (and not sanitizer being too passimistic) the compiler still generates a correct code when the value fits the underlying type of the enum. Maybe you can remove usage of the sanitazing options for a bit of it blocks your development. I guess my fix will be mostly around silencing the sanitizer rather than fixing the real issue.

…

On Sat, Oct 24, 2020, 02:21 Melg Eight ***@***.***> wrote: Setup 1. add compiler flags: -fno-omit-frame-pointer -fno-sanitize-recover=undefined -fsanitize=undefined 2. generate code using schema with fixed values for ID frame: ... <fields> <enum name="CommandType" type="uint8" hexAssign="true"> <validValue name="TestConnection1" val="27" displayName="Test connection to 1" /> <validValue name="TestConnection2" val="38" displayName="Test connection to 2" /> </enum> </fields> ... <frame name="OurProtocolFrame"> <id name="Id"> <field> <ref field="CommandType" /> </field> </id> ... 1. Setup test where comms::processAllWithDispatch is used to dispatch messages, and incoming input has value of ID is way off, for example 68. 2. Build and run test. Expected behavior: Test should pass, without crash, and should notify, that ID value was invalid. Current behavior: Test crash with ubsanitizer reporting: Load of value 68, which is not a valid value for type 'const McuProtocol::MsgId' on line ... Reporting on line MsgIdLayer.h#L689 <https://github.com/commschamp/comms_champion/blob/a92c710ad0b5e84e5a085cba6f54cc0f214870f8/comms/include/comms/protocol/MsgIdLayer.h#L689> BaseImpl::setMsgId(id, extraValues...) Thoughts Problem occurs because of assignment of value to the field with enumeration type, with value 68, where enum only happy with 27 and 38. Current implementation calls BaseImpl::setMsgId(id, extraValues...); Before validation of this value, which trigger ub. But, even removing that call will not help by itself, because inside of call msg = createMsgInternal(id, idx, &failureReason); assignment is done before check, which correctly detects invalid value inside of DispatchMsgStaticBinSearchHelper.h <https://github.com/commschamp/comms_champion/blob/a92c710ad0b5e84e5a085cba6f54cc0f214870f8/comms/include/comms/details/DispatchMsgStaticBinSearchHelper.h#L80> I've tried some ad hoc solutions, but it's not easy to grasp all template meta-programming elements, involved in this. Would really appreciate any temporal bugfixes, which will eliminates this problem. PS. In preparing of this issue I've tried to add same flags into root cmake file, and run ctest after it. Your tests reproduce the same kind of problem, but in other place comms_champion/comms/include/comms/field/adapter/FixedBitLength.h:127:69: runtime error: load of value 4, which is not a valid value for type 'const comms::field::basic::EnumValue<comms::Field<comms::option::def::Endian<comms::util::traits::endian::Big> >, FieldsTestSuite::Enum1>::ValueType' (aka 'const FieldsTestSuite::Enum1') 2: 2: CMake Error at comms_champion/cmake/CC_RunWithValgrindScript.cmake:30 (message): 2: Test failed with exit code 1 If you want I can open another issue about sanitizers (address santizier give even more errors) and tests. But our main focus is on dealing with ID field problem. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <https://github.com/commschamp/comms_champion/issues/12>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASJKGVSJX73HCRLMGQGEI3SMGUP5ANCNFSM4S4YNWDA> .

[arobenko]

Hi Melg,
I managed to spend a couple of hours this weekend on this issue and realized that the problem is rather in "commsdsl" code generator than the COMMS library (transferred the issue).

The quick fix for your problem would be adding sematicType="messageId" property to the definition of the CommandType enum.

The code generator generates MsgId.h which are used for all the message definitions to specify their numeric IDs. The code generator looks for an enum with sematicType="messageId" property set and takes the values from there it also assigns appropriate underlying type copied from the <enum> definition itself. When no such field is found, then the MsgId is generated by accumulating all the numeric IDs of the defined messages (your case). The bug in commsdsl2comms code generator is that in such case it doesn't specify underlying type, which results in the reported UB.

Below is the quote from cppreference:

If the underlying type is not fixed and the source value is out of range, the result is unspecified (until C++17)undefined (since C++17).

Currently the "develop" branch of the commsdsl repo contains the fix, it puts the "unsigned" to be the underlying type of the MsgId enum in such case (reproduced and tested in test42 unittest).

Also note that I introduced multiple fixes to the develop branch of the COMMS library (mostly to the unittests themselves) and now all the unittests are passing with "address" + "undefined" sanitizers. I'll probably do small "patch" releases with the relevant fixes to the comms_champion and the commsdsl projects within a week or two.

[melg8]

That's awesome! Thanks for your work and quick responses!

[arobenko]

Should be fixed in latest v3.5.3 release.