aws
Add build flag to enable the Undefined Behavior Sanitizer
The flag can be enabled in CMake using -DENABLE_UB_SANITIZER=ON similar to the existing flag for the address sanitizer.
Building aws-sdk-cpp (and its dependencies) with this flag indicates a couple of issues, like:
aws-sdk-cpp/src/aws-cpp-sdk-core/include/aws/core/client/AWSClientAsyncCRTP.h:51:40: runtime error: downcast of address 0x7ffe7bff5b90 which does not point to an object of type 'Aws::S3::S3Client'
aws-sdk-cpp/src/aws-cpp-sdk-core/include/aws/core/utils/stream/StreamBufProtectedWriter.h:46:67: runtime error: downcast of address 0x7ffd80fa21e8 which does not point to an object of type 'StreamBufProtectedWriter'
aws-sdk-cpp/crt/aws-crt-cpp/crt/aws-c-common/source/hash_table.c:68:12: runtime error: call to function aws_byte_cursor_eq through pointer to incorrect function type 'bool (*)(const void *, const void *)'
aws-sdk-cpp/crt/aws-crt-cpp/crt/aws-c-sdkutils/source/endpoints_util.c:50:24: runtime error: addition of unsigned offset to 0x595033296ca0 overflowed to 0x595033296c9f
aws-sdk-cpp/tests/aws-cpp-sdk-core-tests/utils/crypto/SymmetricCiphersTest.cpp:689:12: runtime error: null pointer passed as argument 1, which is declared to never be null
aws-sdk-cpp/tests/aws-cpp-sdk-core-tests/utils/crypto/SymmetricCiphersTest.cpp:689:43: runtime error: null pointer passed as argument 2, which is declared to never be null
Existing PRs to fix above issues are: https://github.com/awslabs/aws-c-http/pull/531 and https://github.com/awslabs/aws-c-sdkutils/pull/58 Related issue for one issue: https://github.com/aws/aws-sdk-cpp/issues/3464
Some of these issues are also seen when a SDK-user builds with UBSan. Maybe we should add a CI-step building with this flag?
Check all that applies:
- [x] Did a review by yourself.
- [ ] Added proper tests to cover this PR. (Can be added to CI?)
- [x] Checked if this PR is a breaking (APIs have been changed) change.
- [x] Checked if this PR will not introduce cross-platform inconsistent behavior.
- [x] Checked if this PR would require a ReadMe/Wiki update.
Check which platforms you have built SDK on to verify the correctness of this PR.
- [x] Linux
- [ ] Windows
- [ ] Android
- [ ] MacOS
- [ ] IOS
- [ ] Other Platforms
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
So i have two high level thoughts on this
I know ENABLE_ADDRESS_SANITIZER exists and this would just be adding a mirror to that. However we've been trying to move away from making a cmake option for everything and only making knobs for things that cannot be provided to cmake as a argument. for instance instead of creating this flag one could just use
cmake -DCMAKE_CXX_FLAGS="-ggdb -fsanitize=undefined"
when running cmake generation instead of relying on a option we define ourselves. infact in our CI step we actually use address sanatizer that way instead of providing the cmake option directly.
Maybe we should add a CI-step building with this flag?
I whole heartily agree on this sentiment, but until your PRs are merged and the other issue dealt with the build will actually fail so i would be against adding a flag until its passing.
so summarizing:
- i would prefer to stop the adding of specific cmake options for things that can be provided at generation time.
- we definitely need to add a CI step. likely just updating this argument in our CI scripts once your changes are merged.
- i would prefer to stop the adding of specific cmake options for things that can be provided at generation time.
Sounds reasonable. The problem I faced was how to get the same flags in crt dependencies when building with the default BUILD_DEPS=ON. We want to enable UBSan on dependencies to avoid false positives.
This was solved in this PR by using cached CMake variables, but maybe there is some other way.. I will take a look.