From c6f96d687f32bc7bcd06b7162dae4784a600dd41 Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Thu, 16 Dec 2021 17:33:24 -0800 Subject: [PATCH] Re-enabling RBE ubsan (#27861) * Cherry-pick #27439 * Fix StatusGetTime misalignment * Fix memcpy with nullptr * Fix boringssl ubsan issue * Fix slice ubsan issue * Ignore msan errors for ubsan * Add upb UBSAN exception * Add local ubsan run comment * Revert "Fix StatusGetTime misalignment" This reverts commit a1d0c34e8ea6933d09386867e8d4836d9e1b4e1c. * Fix status_helper * Added alignas * Fix fuzzer_corpus_test * Removed binder_transport_test from ubsan --- src/core/lib/gprpp/status_helper.cc | 5 ++++- src/core/lib/security/context/security_context.cc | 4 +++- src/core/lib/slice/slice.cc | 4 +++- test/core/transport/binder/BUILD | 4 ++++ test/core/util/fuzzer_corpus_test.cc | 4 +++- test/core/util/ubsan_suppressions.txt | 5 +++++ tools/bazel.rc | 14 +++++++++++--- tools/remote_build/rbe_common.bazelrc | 10 +++++++++- 8 files changed, 42 insertions(+), 8 deletions(-) diff --git a/src/core/lib/gprpp/status_helper.cc b/src/core/lib/gprpp/status_helper.cc index 5fde9ca9904..5746fd8b50f 100644 --- a/src/core/lib/gprpp/status_helper.cc +++ b/src/core/lib/gprpp/status_helper.cc @@ -230,7 +230,10 @@ absl::optional StatusGetTime(const absl::Status& status, if (p.has_value()) { absl::optional sv = p->TryFlat(); if (sv.has_value()) { - return *reinterpret_cast(sv->data()); + // copy the content before casting to avoid misaligned address access + alignas(absl::Time) char buf[sizeof(const absl::Time)]; + memcpy(buf, sv->data(), sizeof(const absl::Time)); + return *reinterpret_cast(buf); } else { std::string s = std::string(*p); return *reinterpret_cast(s.c_str()); diff --git a/src/core/lib/security/context/security_context.cc b/src/core/lib/security/context/security_context.cc index 7e0aa2b8e2d..903bcb0ed8b 100644 --- a/src/core/lib/security/context/security_context.cc +++ b/src/core/lib/security/context/security_context.cc @@ -235,7 +235,9 @@ void grpc_auth_context::add_property(const char* name, const char* value, grpc_auth_property* prop = &properties_.array[properties_.count++]; prop->name = gpr_strdup(name); prop->value = static_cast(gpr_malloc(value_length + 1)); - memcpy(prop->value, value, value_length); + if (value != nullptr) { + memcpy(prop->value, value, value_length); + } prop->value[value_length] = '\0'; prop->value_length = value_length; } diff --git a/src/core/lib/slice/slice.cc b/src/core/lib/slice/slice.cc index 857db368adc..e0c4cb038a0 100644 --- a/src/core/lib/slice/slice.cc +++ b/src/core/lib/slice/slice.cc @@ -215,7 +215,9 @@ grpc_slice grpc_slice_from_copied_buffer(const char* source, size_t length) { slice = grpc_core::UnmanagedMemorySlice( length, grpc_core::UnmanagedMemorySlice::ForceHeapAllocation()); } - memcpy(GRPC_SLICE_START_PTR(slice), source, length); + if (length > 0) { + memcpy(GRPC_SLICE_START_PTR(slice), source, length); + } return slice; } diff --git a/test/core/transport/binder/BUILD b/test/core/transport/binder/BUILD index e69964d753c..dd14892b19b 100644 --- a/test/core/transport/binder/BUILD +++ b/test/core/transport/binder/BUILD @@ -92,6 +92,10 @@ grpc_cc_test( "gtest", ], language = "C++", + tags = [ + # To avoid `symbolizer buffer too small` warning of UBSAN + "noubsan", + ], uses_polling = False, deps = [ ":mock_objects", diff --git a/test/core/util/fuzzer_corpus_test.cc b/test/core/util/fuzzer_corpus_test.cc index 0638baf2038..450609aec35 100644 --- a/test/core/util/fuzzer_corpus_test.cc +++ b/test/core/util/fuzzer_corpus_test.cc @@ -57,7 +57,9 @@ TEST_P(FuzzerCorpusTest, RunOneExample) { grpc_load_file(GetParam().c_str(), 0, &buffer))); size_t length = GRPC_SLICE_LENGTH(buffer); void* data = gpr_malloc(length); - memcpy(data, GPR_SLICE_START_PTR(buffer), length); + if (length > 0) { + memcpy(data, GPR_SLICE_START_PTR(buffer), length); + } grpc_slice_unref(buffer); grpc_shutdown(); LLVMFuzzerTestOneInput(static_cast(data), length); diff --git a/test/core/util/ubsan_suppressions.txt b/test/core/util/ubsan_suppressions.txt index 015f4b89d78..37001c05a77 100644 --- a/test/core/util/ubsan_suppressions.txt +++ b/test/core/util/ubsan_suppressions.txt @@ -26,3 +26,8 @@ enum:api_fuzzer # https://github.com/GoogleCloudPlatform/layer-definitions/issues/531 is # addressed. alignment:grpc_core::XdsPriorityListUpdate::* +# Benign pointer-overflow error in boringssl +# (ref: https://github.com/openssl/openssl/issues/16816) +pointer-overflow:mem_ctrl +# https://github.com/protocolbuffers/upb/issues/452 +pointer-overflow:_upb_extreg_add diff --git a/tools/bazel.rc b/tools/bazel.rc index 9733eafb978..13e3fcb47bd 100644 --- a/tools/bazel.rc +++ b/tools/bazel.rc @@ -80,16 +80,24 @@ build:tsan --test_env=TSAN_OPTIONS=report_atomic_races=0 build:tsan --action_env=TSAN_OPTIONS=suppressions=test/core/util/tsan_suppressions.txt:halt_on_error=1:second_deadlock_stack=1 build:ubsan --strip=never +build:ubsan --copt=-fsanitize=undefined build:ubsan --copt=-fsanitize-link-c++-runtime build:ubsan --copt=-fno-omit-frame-pointer build:ubsan --copt=-DGRPC_UBSAN build:ubsan --copt=-DNDEBUG build:ubsan --copt=-fno-sanitize=function,vptr +build:ubsan --linkopt=-fsanitize=undefined +# avoid ubsan build error with int128 by linking against libc++ +# see b/200667821 build:ubsan --linkopt=-fsanitize-link-c++-runtime +build:ubsan --linkopt=-lc++ +build:ubsan --linkopt=-lc++abi +build:ubsan --linkopt=-lm build:ubsan --action_env=UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1:suppressions=test/core/util/ubsan_suppressions.txt -# For some reasons, these two stopped being propagated, so, redeclaring them here. -# That's a hack that needs to be removed once we understand what's going on. -build:ubsan --copt=-DGRPC_PORT_ISOLATED_RUNTIME=1 +# if you want to run ubsan locally, uncomment following three lines +# build:ubsan --linkopt=-lgcc_s +# build:ubsan --linkopt=-lubsan +# build:ubsan --linkopt=--rtlib=compiler-rt build:basicprof --strip=never build:basicprof --copt=-DNDEBUG diff --git a/tools/remote_build/rbe_common.bazelrc b/tools/remote_build/rbe_common.bazelrc index f8369ecf5a7..3d03f4c5812 100644 --- a/tools/remote_build/rbe_common.bazelrc +++ b/tools/remote_build/rbe_common.bazelrc @@ -79,7 +79,6 @@ build:tsan --copt=-gmlt # tests tend to be slower build:tsan --test_timeout=60,600,1800,3600 build:tsan --test_tag_filters=-no_linux,-notsan,-qps_json_driver -build:tsan --extra_execution_platforms=@rbe_default//config:platform # undefined behavior sanitizer: most settings are already in %workspace%/.bazelrc # we only need a few additional ones that are Foundry specific @@ -87,3 +86,12 @@ build:ubsan --copt=-gmlt # use double the default value for "moderate" and "long" timeout as sanitizer # tests tend to be slower build:ubsan --test_timeout=60,600,1800,3600 +# RBE docker image only has a msan specific version of libc++, so to be able to link against it, +# we need to involuntarily enable msan as well to be able to build. See b/200667821 +# TODO(jtattermusch): disable memory sanitizer for ubsan build as soon as possible. +build:ubsan --copt=-fsanitize=memory +build:ubsan --copt=-fsanitize-recover=memory +build:ubsan --linkopt=-fsanitize=memory +build:ubsan --cxxopt=--stdlib=libc++ +build:ubsan --action_env=MSAN_OPTIONS=halt_on_error=0 +build:ubsan --test_tag_filters=-no_linux,-noubsan