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

webrtc/base/optional.h

 /*
  *  Copyright 2015 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.
  */
 
 #ifndef WEBRTC_BASE_OPTIONAL_H_
 #define WEBRTC_BASE_OPTIONAL_H_
 
 #include <algorithm>
 #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
 // being assigned to after having been moved from. Specifically, you may not
 // assume that it just doesn't contain a value anymore.
 //
 // Examples of good places to use Optional:
 //
 // - As a class or struct member, when the member doesn't always have a value:
 //     struct Prisoner {
@@ skipping 20 matching lines @@
 //   Optional<double> when parsing a single number as in the example above
 //   might make sense, but any larger parse job is probably going to need to
 //   tell the caller what the problem was, not just that there was one.
 //
 // TODO(kwiberg): Get rid of this class when the standard library has
 // std::optional (and we're allowed to use it).
 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) {
     new (&value_) T(value);
   }
   explicit Optional(T&& value) : has_value_(true) {
     new (&value_) T(std::move(value));
   }
 
   // Copy constructor: copies the value from m if it has one.
   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
   // still in a state where it has a value, but a moved-from one (the
   // properties of which depends on T; the only general guarantee is that we
   // can destroy m).
   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
   // copy constructor if only the right-hand side has a value.
   Optional& operator=(const Optional& m) {
     if (m.has_value_) {
       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;
   }
 
   // Move assignment. Uses T's move assignment if both sides have a value, T's
   // move constructor if only the right-hand side has a value. The state of m
   // after it's been moved from is as for the move constructor.
   Optional& operator=(Optional&& m) {
     if (m.has_value_) {
       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;
   }
 
   // Swap the values if both m1 and m2 have values; move the value if only one
   // of them has one.
   friend void swap(Optional& m1, Optional& m2) {
     if (m1.has_value_) {
       if (m2.has_value_) {
         // Both have values: swap.
         using std::swap;
         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();
     }
   }
 
   // Conversion to bool to test if we have a value.
   explicit operator bool() const { return has_value_; }
 
   // Dereferencing. Only allowed if we have a value.
   const T* operator->() const {
     RTC_DCHECK(has_value_);
     return &value_;
   }
   T* operator->() {
     RTC_DCHECK(has_value_);
     return &value_;
   }
   const T& operator*() const {
     RTC_DCHECK(has_value_);
     return value_;
   }
   T& operator*() {
     RTC_DCHECK(has_value_);
     return value_;
   }
 
   // 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,
   // or
   // if they're both empty.
   friend bool operator==(const Optional& m1, const Optional& m2) {
     return m1.has_value_ && m2.has_value_ ? m1.value_ == m2.value_
                                           : m1.has_value_ == m2.has_value_;
   }
   friend bool operator!=(const Optional& m1, const Optional& m2) {
     return m1.has_value_ && m2.has_value_ ? m1.value_ != m2.value_
                                           : m1.has_value_ != m2.has_value_;
   }
 
  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
     // it doesn't contain any data. If the union goes uninitialized, it may
     // trigger compiler warnings.
     char empty_;
     // By placing value_ in a union, we get to manage its construction and
     // destruction manually: the Optional constructors won't automatically
     // construct it, and the Optional destructor won't automatically destroy
     // it. Basically, this just allocates a properly sized and aligned block of
     // memory in which we can manually put a T with placement new.
     T value_;
   };
 };
 
 }  // namespace rtc
 
 #endif  // WEBRTC_BASE_OPTIONAL_H_