From 89da711a26a87451ed2bc26e29959e45c6e3bffe Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Thu, 16 Sep 2021 23:22:20 -0700 Subject: [PATCH] Correctly sanitize nonnull pointers. (#834) Only the special nullability attributes (`_Nonnull`) work correctly through type aliases like we're using with `Ptr`. But they aren't strictly UB and so have to be specially enabled in our sanitizer config in order to usefully catch nullness errors early. Turn on those sanitizers as well. Also, now that we are using fully remote build output caching for our CI and not trying to squeeze under an arbitrary size limit, re-enable the nice error messages for all the UBSan checks. Note that this will have a (very) slow CI run as it will have to recompile ~everything and upload fresh artifacts. But those should then be effective cache hits going forward. --- bazel/cc_toolchains/clang_cc_toolchain_config.bzl | 6 +----- executable_semantics/common/ptr.h | 7 +++---- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/bazel/cc_toolchains/clang_cc_toolchain_config.bzl b/bazel/cc_toolchains/clang_cc_toolchain_config.bzl index 702c0107d8da..a9670d6b23dd 100644 --- a/bazel/cc_toolchains/clang_cc_toolchain_config.bzl +++ b/bazel/cc_toolchains/clang_cc_toolchain_config.bzl @@ -444,7 +444,7 @@ def _impl(ctx): flag_sets = [flag_set( actions = all_compile_actions + all_link_actions, flag_groups = [flag_group(flags = [ - "-fsanitize=address,undefined", + "-fsanitize=address,undefined,nullability", "-fsanitize-address-use-after-scope", # We don't need the recovery behavior of UBSan as we expect # builds to be clean. Not recoverying is a bit cheaper. @@ -453,10 +453,6 @@ def _impl(ctx): # and combined with line numbers is unlikely to result in many # ambiguities. "-fsanitize-undefined-strip-path-components=-1", - # Force some expensive UBSan checks to the cheaper trap mode. - # The dedicated debugging message is unlikely to be critical for - # these. - "-fsanitize-trap=alignment,bool,null,return,unreachable", # Needed due to clang AST issues, such as in # clang/AST/Redeclarable.h line 199. "-fno-sanitize=vptr", diff --git a/executable_semantics/common/ptr.h b/executable_semantics/common/ptr.h index c971e5635459..b3cc5f174189 100644 --- a/executable_semantics/common/ptr.h +++ b/executable_semantics/common/ptr.h @@ -11,12 +11,11 @@ namespace Carbon { // A non-nullable pointer. Written as `Nonnull` instead of `T*`. // -// Note LLVM primarily enforces the attribute on function calls that can be -// proven to be called with nullptr; in other places, this is essentially a -// comment. +// Sanitizers enforce this dynamically on assignment, return, and when passing +// as an argument. Static analysis will also track erroneous uses of `nullptr`. template >* = nullptr> -using Nonnull = T _Nonnull __attribute__((nonnull)); +using Nonnull = T _Nonnull; } // namespace Carbon