Build Themis with sanitizers #548
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add a bunch of configuration options to enable various sanitizers available for GCC and Clang: - WITH_ASAN - Address Sanitizer, mostly memory safety issues - WITH_MSAN - Memory Sanitizer, other types of memory safety - WITH_TSAN - Thread Sanitizer, mostly locks and threading - WITH_UBSAN - Undefined Behavior Sanitizer, various issues The exact supported set of the sanitizers vary depending on the compiler version so we check for them at startup and use only those which are available. UBSan is especially different between GCC and Clang. Obviously, these are opt-in options which are not enabled by default. They are useful during development but should not affect production builds. There is also an option to make sanitizer warnings fatal which will be useful for CI. Normally the developers do not need to enable it so that they see all issues, but for CI we would like to abort the build if the sanitizers report any issues.
Teach CircleCI to execrise the test suite with all available sanitizers. Unfortunately, some sanitizers are not available on all compilers, and some produce a lot of unrelated errors so they are currently disabled.
Unfortunately, there are quite a few places which trigger UBSan (and rightly so). Currently we don't have time to review and fix them properly, so let's disable the relevant warnings for now. ed25519 implementation has been written by C gurus and has some questionable numeric code. These warnings could be avoided by refactoring the code to use correct types and casts, but it requires careful attention. Leave it as is for now. Other warnings are caused by misaligned accesses when handling soter_container_t types. Everywhere in our codebase we cast pointers to arbitrarily aligned bytes to C structs and technically this is undefined behavior because on some processor architectures it may cause CPU exceptions. This has been known for a long time, but we did nothing about it. Silence these warnings for now, but we'd better fix them at some point in the future so that we don't have to maintain this exclusion blacklist. And by the way, GCC does not support -fsanitize-blacklist so we have disabled UBSan for GCC completely. The alternative is to annotate affected functions with an attribute, but there are so many of them that I don't want to litter history with it.
The code expects the size field to contain unsigned size, but the structure definition has signed size. We never use negative values for anything, but expect the correct overflow behavior. One of the tests actually triggers UBSan warning about this. Switch the type to correct uint32_t to have the correct behavior. This is technically public API, however no-one should be using the structure field directly because it's encoded in big endian. Switching to unsigned type also does not change the ABI (on the platforms that we care about), so all in all this is a safe fix.
UBSan gives us another warning about using "~0L" expression to get "all 1-bits" initialization value. Technically, this is undefined behavior because you cannot bit-flip a signed value and you cannot cast a negative signed value into an unsigned one. Just use a different equivalent initializer which expresses the same idea and keeps UBSan happy.
ilammy
added
infrastructure
ilammy
requested review from
Lagovas,
shadinua,
storojs72 and
vixentael
as code owners
vixentael
approved these changes
Oct 30, 2019
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is another patch set that I have found in my repo and never cared to merge. It makes Themis a safer place for writing new C code by ensuring that the code does not do anything funny, like causing obvious memory safety issues or glaring undefined behavior.
A bunch of new Makefile configuration options enable various sanitizers available for GCC and Clang:
WITH_ASAN– Address Sanitizer, mostly memory safety issuesWITH_MSAN– Memory Sanitizer, other types of memory safetyWITH_TSAN– Thread Sanitizer, mostly locks and threadingWITH_UBSAN– Undefined Behavior Sanitizer, various issuesThe exact supported set of the sanitizers vary depending on the compiler type and version so we check for them at startup and use only those which are available. UBSan is especially different between GCC and Clang.
Obviously, these are opt-in options which are not enabled by default. They are useful during development but should not affect production builds.
Teach CircleCI to exercise the test suite with all available sanitizers, failing the build if any sanitizer reports a possible violation. Unfortunately, some sanitizers are not available on all compilers, and some produce a lot of unrelated errors so they are currently disabled.
In particular, UBSan currently produces some strict warnings about our code. They are mostly caused by third-party code and some known legacy warts, which have been silenced for now. Other reported instances are fixed right away. See individual commit message for details.