Problem with UndefinedBehaviorSanitizer

[gavv]

How to reproduce:

$ scons -Q --build-3rdparty=openfec,pulseaudio,sox,cpputest,google-benchmark \
  --compiler=clang --enable-debug --enable-werror \
  --enable-examples --enable-tests --enable-benchmarks \
  --sanitizers=all --enable-pulseaudio-modules \
  test

Output:

........
     TEST   roc_library
!......src/library/src/sender.cpp:181:46: runtime error: load of value 4294967295, which is not a valid value for type 'roc_interface'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/library/src/sender.cpp:181:46 in 
src/library/src/config_helpers.cpp:213:18: runtime error: load of value 4294967295, which is not a valid value for type 'roc_interface'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/library/src/config_helpers.cpp:213:18 in 
src/library/src/sender.cpp:77:46: runtime error: load of value 4294967295, which is not a valid value for type 'roc_interface'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/library/src/sender.cpp:77:46 in 
src/library/src/sender.cpp:109:46: runtime error: load of value 4294967295, which is not a valid value for type 'roc_interface'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/library/src/sender.cpp:109:46 in 
src/library/src/sender.cpp:142:46: runtime error: load of value 4294967295, which is not a valid value for type 'roc_interface'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/library/src/sender.cpp:142:46 in 
.....src/library/src/endpoint.cpp:64:42: runtime error: load of value 4294967295, which is not a valid value for type 'roc_protocol'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/library/src/endpoint.cpp:64:42 in 
src/library/src/config_helpers.cpp:231:18: runtime error: load of value 4294967295, which is not a valid value for type 'roc_protocol'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/library/src/config_helpers.cpp:231:18 in 
...src/library/src/receiver.cpp:115:46: runtime error: load of value 4294967295, which is not a valid value for type 'roc_interface'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/library/src/receiver.cpp:115:46 in 
src/library/src/receiver.cpp:77:46: runtime error: load of value 4294967295, which is not a valid value for type 'roc_interface'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/library/src/receiver.cpp:77:46 in 
.......................
OK (38 tests, 37 ran, 810 checks, 1 ignored, 0 filtered out, 1044 ms)

We have two problems here:

• First, this runtime error does not cause a crash or failure in tests, which is bad because we don't notice it in CI.

• Second, this runtime error is actually a false positive. Yes, we really store an incorrect value in enum, but we do it intentionally: this unit test checks that such a value would be handled correctly and an error will be reported.

What should we do:

• First we should somehow configure sanitizers to make this test actually failing / crashing / aborting.
• Then we should fix the error. E.g. we can cast int to enum only after validating it.

[pvimal816]

I want to work on this issue.

[pvimal816]

I thought about how would I approach this problem.

If we can somehow stop the execution of tests when a runtime error occurs then we can notice it in CI. So we can configure sanitizers to do not continue execution after runtime error and stop instead. So we just have to pass -fno-sanitize-recover=undefined(this will stop execution for 'undefined behaviour' only) option in compilation.

[gavv]

I want to work on this issue.

Great!

If we can somehow stop the execution of tests when a runtime error occurs then we can notice it in CI. So we can configure sanitizers to do not continue execution after runtime error and stop instead. So we just have to pass -fno-sanitize-recover=undefined(this will stop execution for 'undefined behaviour' only) option in compilation.

Sounds good. I think we should set -fno-sanitize-recover for any sanitizer that we're enabling in scons.

The manual also mentions halt_on_error runtime option, if just using -fno-sanitize-recover wouldn't be enough, we can try to set it too.

[pvimal816]

In c++ it is undefined behavior to set an invalid value to an enum type(at least in our case).

Some workarounds are as follows:

• Change function argument type from roc_interface to int then after check pass it to iface. I don't know if this is a good approach.
• Another option is that we can use int as the underlying type of roc_interface but that requires us to use c++ version>=11(But I think our config.h which contains the definition of roc_interface is compiled as c header file. So we can not use this solution).

Please suggest if you have any other option in mind.

[gavv]

Changing API just to calm down sanitizer is an overkill, we shouldn't do it.

We use C++98 internally, and the API headers are pure C.

Can we change the implementation of interface_from_user() to cast enum to int (maybe using reinterpret cast or cast via a union) and only then check the value?

If we don't find a way to fix this in code (without changing API), we can add the function to sanitizer blacklist.

[pvimal816]

we can use reinterprete_cast to convert the enum to int. I have tested it on ubuntu 18.04, 64 bit, g++ and clang++.

But then it will become compiler and target-dependent. If the compiler has not allotted as much memory to enum as it does for int then we would have illegal memory access?

I don't think reinterpret_cast is a good choice. What do you suggest?

[gavv]

I was thinking about casting value, but you're talking about casting address, right? Because apparently reinterpret cast doesn't allow casting between integer types. You're right, pointer cast will depend on compiler and endianness, so let's avoid it. If a regular cast or static cast of enum to int doesn't help either, I don't see any good options except using blacklist.

[pvimal816]

Yes I was talking about pointers.

[gavv]

Merged!