Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Build Themis with sanitizers #548

Merged
merged 5 commits into from
Nov 8, 2019
Merged

Build Themis with sanitizers #548

merged 5 commits into from
Nov 8, 2019

Commits on Oct 1, 2019

  1. Support builds with sanitizers 

    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.
    @ilammy
    ilammy committed Oct 1, 2019
    Configuration menu
    Copy the full SHA
    b9001d6 View commit details
    Browse the repository at this point in the history

  2. Run tests with sanitizers on CI 

    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.
    @ilammy
    ilammy committed Oct 1, 2019
    Configuration menu
    Copy the full SHA
    172d4eb View commit details
    Browse the repository at this point in the history

  3. Blacklist some UBSan warnings 

    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.
    @ilammy
    ilammy committed Oct 1, 2019
    Configuration menu
    Copy the full SHA
    4b9000c View commit details
    Browse the repository at this point in the history

  4. Correctly signed size of Soter containers 

    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.
    @ilammy
    ilammy committed Oct 1, 2019
    Configuration menu
    Copy the full SHA
    bcdc149 View commit details
    Browse the repository at this point in the history

  5. Initialize CRC safely 

    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
    ilammy committed Oct 1, 2019
    Configuration menu
    Copy the full SHA
    f8f56a0 View commit details
    Browse the repository at this point in the history