From 2b11fd2e4bcbd5240ee04110323309f1fa2d3251 Mon Sep 17 00:00:00 2001 From: kwiberg Date: Fri, 9 Sep 2016 03:35:24 -0700 Subject: [PATCH] rtc::Optional: Tell sanitizers that unset values aren't OK to access This is a sample use of the other three sanitizer functions introduced in https://codereview.webrtc.org/2293893002/. BUG=chromium:617124 Review-Url: https://codereview.webrtc.org/2289383002 Cr-Commit-Position: refs/heads/master@{#14157} --- webrtc/base/BUILD.gn | 1 + webrtc/base/base.gyp | 1 + webrtc/base/optional.cc | 23 ++++++++++++++++ webrtc/base/optional.h | 59 +++++++++++++++++++++++++++++++++++++++-- 4 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 webrtc/base/optional.cc diff --git a/webrtc/base/BUILD.gn b/webrtc/base/BUILD.gn index ec92cce600..2b1be29058 100644 --- a/webrtc/base/BUILD.gn +++ b/webrtc/base/BUILD.gn @@ -128,6 +128,7 @@ rtc_static_library("rtc_base_approved") { "md5digest.h", "mod_ops.h", "onetimeevent.h", + "optional.cc", "optional.h", "platform_file.cc", "platform_file.h", diff --git a/webrtc/base/base.gyp b/webrtc/base/base.gyp index 9baecd6802..ed8cd59362 100644 --- a/webrtc/base/base.gyp +++ b/webrtc/base/base.gyp @@ -65,6 +65,7 @@ 'md5digest.h', 'mod_ops.h', 'onetimeevent.h', + 'optional.cc', 'optional.h', 'platform_file.cc', 'platform_file.h', diff --git a/webrtc/base/optional.cc b/webrtc/base/optional.cc new file mode 100644 index 0000000000..6bebdd5a19 --- /dev/null +++ b/webrtc/base/optional.cc @@ -0,0 +1,23 @@ +/* + * Copyright 2016 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "webrtc/base/optional.h" + +namespace rtc { +namespace optional_internal { + +#if RTC_HAS_ASAN + +void* FunctionThatDoesNothingImpl(void* x) { return x; } + +#endif + +} // namespace optional_internal +} // namespace rtc diff --git a/webrtc/base/optional.h b/webrtc/base/optional.h index e98ab2e3ce..b047fabc42 100644 --- a/webrtc/base/optional.h +++ b/webrtc/base/optional.h @@ -15,10 +15,34 @@ #include #include +#include "webrtc/base/array_view.h" #include "webrtc/base/checks.h" +#include "webrtc/base/sanitizer.h" namespace rtc { +namespace optional_internal { + +#if RTC_HAS_ASAN + +// This is a non-inlined function. The optimizer can't see inside it. +void* FunctionThatDoesNothingImpl(void*); + +template +inline T* FunctionThatDoesNothing(T* x) { + return reinterpret_cast( + FunctionThatDoesNothingImpl(reinterpret_cast(x))); +} + +#else + +template +inline T* FunctionThatDoesNothing(T* x) { return x; } + +#endif + +} // namespace optional_internal + // Simple std::optional-wannabe. It either contains a T or not. // // A moved-from Optional may only be destroyed, and assigned to if T allows @@ -59,7 +83,9 @@ template class Optional final { public: // Construct an empty Optional. - Optional() : has_value_(false), empty_('\0') {} + Optional() : has_value_(false), empty_('\0') { + PoisonValue(); + } // Construct an Optional that contains a value. explicit Optional(const T& value) : has_value_(true) { @@ -73,6 +99,8 @@ class Optional final { Optional(const Optional& m) : has_value_(m.has_value_) { if (has_value_) new (&value_) T(m.value_); + else + PoisonValue(); } // Move constructor: if m has a value, moves the value from m, leaving m @@ -82,11 +110,15 @@ class Optional final { Optional(Optional&& m) : has_value_(m.has_value_) { if (has_value_) new (&value_) T(std::move(m.value_)); + else + PoisonValue(); } ~Optional() { if (has_value_) value_.~T(); + else + UnpoisonValue(); } // Copy assignment. Uses T's copy assignment if both sides have a value, T's @@ -96,12 +128,14 @@ class Optional final { if (has_value_) { value_ = m.value_; // T's copy assignment. } else { + UnpoisonValue(); new (&value_) T(m.value_); // T's copy constructor. has_value_ = true; } } else if (has_value_) { value_.~T(); has_value_ = false; + PoisonValue(); } return *this; } @@ -114,12 +148,14 @@ class Optional final { if (has_value_) { value_ = std::move(m.value_); // T's move assignment. } else { + UnpoisonValue(); new (&value_) T(std::move(m.value_)); // T's move constructor. has_value_ = true; } } else if (has_value_) { value_.~T(); has_value_ = false; + PoisonValue(); } return *this; } @@ -134,17 +170,21 @@ class Optional final { swap(m1.value_, m2.value_); } else { // Only m1 has a value: move it to m2. + m2.UnpoisonValue(); new (&m2.value_) T(std::move(m1.value_)); m1.value_.~T(); // Destroy the moved-from value. m1.has_value_ = false; m2.has_value_ = true; + m1.PoisonValue(); } } else if (m2.has_value_) { // Only m2 has a value: move it to m1. + m1.UnpoisonValue(); new (&m1.value_) T(std::move(m2.value_)); m2.value_.~T(); // Destroy the moved-from value. m1.has_value_ = true; m2.has_value_ = false; + m2.PoisonValue(); } } @@ -171,7 +211,11 @@ class Optional final { // Dereference with a default value in case we don't have a value. const T& value_or(const T& default_val) const { - return has_value_ ? value_ : default_val; + // The no-op call prevents the compiler from generating optimized code that + // reads value_ even if !has_value_, but only if FunctionThatDoesNothing is + // not completely inlined; see its declaration.). + return has_value_ ? *optional_internal::FunctionThatDoesNothing(&value_) + : default_val; } // Equality tests. Two Optionals are equal if they contain equivalent values, @@ -187,6 +231,17 @@ class Optional final { } private: + // Tell sanitizers that value_ shouldn't be touched. + void PoisonValue() { + rtc::AsanPoison(rtc::MakeArrayView(&value_, 1)); + rtc::MsanMarkUninitialized(rtc::MakeArrayView(&value_, 1)); + } + + // Tell sanitizers that value_ is OK to touch again. + void UnpoisonValue() { + rtc::AsanUnpoison(rtc::MakeArrayView(&value_, 1)); + } + bool has_value_; // True iff value_ contains a live value. union { // empty_ exists only to make it possible to initialize the union, even when