rtc::Optional: Tell sanitizers that unset values aren't OK to access

webrtc/base/optional.h

Index: webrtc/base/optional.h
diff --git a/webrtc/base/optional.h b/webrtc/base/optional.h
index e98ab2e3ce7e9765386bcb8cbb47c92f4a7b42fa..8a5ce99af3a2c10e9367128bf20eb5b0d0e8eaad 100644
--- a/webrtc/base/optional.h
+++ b/webrtc/base/optional.h
@@ -15,10 +15,34 @@
 #include <memory>
 #include <utility>
 
+#include "webrtc/base/array_view.h"
 #include "webrtc/base/checks.h"
+#include "webrtc/base/sanitizer.h"
 
 namespace rtc {
 
+namespace internal {
+
+#if RTC_HAS_ASAN
+
+// This is a non-inlined function. The optimizer can't see inside it.
+void* FunctionThatDoesNothingImpl(void*);

[ossu, 2016/09/05 08:27:21]

I'm a bit cautious about this one. Since we can't hide it properly, (i.e. put it
in an anonymous namespace in the .cc file), there's a chance it will actually
clash if we want to do this for another class.
Wouldn't it be better if you stuffed this inside Optional, as a private (static)
member function? That or shoving it into some helper header, I guess.

[kwiberg-webrtc, 2016/09/05 09:11:30]

No, I can't put it in rtc::Optional, because that's a template, and that doesn't
mesh well with this function's need to be non-inline.

Would it be OK if I instead changed the namespace to "optional_internal" or
something?

[ossu, 2016/09/08 08:49:03]

Ah, that's true. Yeah, optional_internal is fine by me!

+
+template <typename T>
+inline T* FunctionThatDoesNothing(T* x) {
+  return reinterpret_cast<T*>(
+      FunctionThatDoesNothingImpl(reinterpret_cast<void*>(x)));
+}
+
+#else
+
+template <typename T>
+inline T* FunctionThatDoesNothing(T* x) { return x; }
+
+#endif
+
+}  // namespace internal
+
 // Simple std::optional-wannabe. It either contains a T or not.
 //
 // A moved-from Optional<T> may only be destroyed, and assigned to if T allows
@@ -59,7 +83,9 @@ template <typename T>
 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_ ? *internal::FunctionThatDoesNothing(&value_)

[ossu, 2016/09/05 09:38:43]

If this is a problem for us here, how do we prevent it from being a problem in
the rest of the code?

I.e. if my function somewhere does if(opt) { return *opt; } or the (almost)
exact same thing as above: return opt ? *opt : "nothing!".
Is that safe or will it come back to bite us?

[kwiberg-webrtc, 2016/09/05 12:41:21]

Yeah, it isn't safe in the general case, in that that there are probably other
constructs that will cause problems. The problems will only ever be observed in
ASan builds, though, so the potential damage is small.

In the slightly longer term, I'd like us to steal Chromium's base::Optional, and
that one has an inner class for the specific purpose of holding the value; if we
guard its accessors like this, it ought to be foolproof. Doing the same for our
current Optional class would take a fair bit of refactoring, which I'm not keen
on doing; it would be like washing your car when you're just about to ditch it
and steal a new one.

[ossu, 2016/09/08 08:49:03]

Hmm... alright, let's put this in for now and revisit once we re-import Optional
from Chromium.

+                      : 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