-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Undefined behaviour on "out of range" values of ID layer. #8
Comments
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>
.
|
Hi Melg, The quick fix for your problem would be adding The code generator generates Below is the quote from cppreference:
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. |
That's awesome! Thanks for your work and quick responses! |
Should be fixed in latest v3.5.3 release. |
Setup
-fno-omit-frame-pointer
-fno-sanitize-recover=undefined
-fsanitize=undefined
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
Specs
Files are "Generated by commsdsl2comms v3.5.2"
compilers:
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
Before validation of this value, which trigger ub.
But, even removing that call will not help by itself, because inside of call
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
@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.
The text was updated successfully, but these errors were encountered: