From b3b017950a8956d86305a51f3978dd2ea0b27234 Mon Sep 17 00:00:00 2001
From: Karl Wiberg <kwiberg@webrtc.org>
Date: Wed, 10 Oct 2018 12:44:12 +0200
Subject: [PATCH] Fix backwards logic in rtc::Buffer::OnMovedFrom()

The logic in rtc::Buffer::OnMovedFrom was backwards w.r.t.
RTC_DCHECK_IS_ON. We intended to provoke bugs when DCHECKs are on and
play it safe when DCHECKs are off, but actually we did the reverse.
This CL fixes that.

It also adds a death test that would have caught the bug.

Bug: webrtc:9856
Change-Id: Ib6a4b07d12732e5a66e93b36b885abdab93e55c7
Reviewed-on: https://webrtc-review.googlesource.com/c/105040
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25091}
---
 rtc_base/buffer.h           | 11 +++++------
 rtc_base/buffer_unittest.cc | 13 +++++++++++++
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/rtc_base/buffer.h b/rtc_base/buffer.h
index 22f89372de..9bf96cd146 100644
--- a/rtc_base/buffer.h
+++ b/rtc_base/buffer.h
@@ -149,7 +149,6 @@ class BufferT {
   }
 
   BufferT& operator=(BufferT&& buf) {
-    RTC_DCHECK(IsConsistent());
     RTC_DCHECK(buf.IsConsistent());
     size_ = buf.size_;
     capacity_ = buf.capacity_;
@@ -389,7 +388,7 @@ class BufferT {
     ExplicitZeroMemory(data_.get() + size_, count * sizeof(T));
   }
 
-  // Precondition for all methods except Clear and the destructor.
+  // Precondition for all methods except Clear, operator= and the destructor.
   // Postcondition for all methods except move construction and move
   // assignment, which leave the moved-from object in a possibly inconsistent
   // state.
@@ -401,14 +400,14 @@ class BufferT {
   // can mutate the state slightly to help subsequent sanity checks catch bugs.
   void OnMovedFrom() {
 #if RTC_DCHECK_IS_ON
+    // Ensure that *this is always inconsistent, to provoke bugs.
+    size_ = 1;
+    capacity_ = 0;
+#else
     // Make *this consistent and empty. Shouldn't be necessary, but better safe
     // than sorry.
     size_ = 0;
     capacity_ = 0;
-#else
-    // Ensure that *this is always inconsistent, to provoke bugs.
-    size_ = 1;
-    capacity_ = 0;
 #endif
   }
 
diff --git a/rtc_base/buffer_unittest.cc b/rtc_base/buffer_unittest.cc
index ae976f1394..1c3abfd08f 100644
--- a/rtc_base/buffer_unittest.cc
+++ b/rtc_base/buffer_unittest.cc
@@ -434,6 +434,19 @@ TEST(BufferTest, TestStruct) {
   EXPECT_EQ(kObsidian, buf[2].stone);
 }
 
+TEST(BufferTest, DieOnUseAfterMove) {
+  Buffer buf(17);
+  Buffer buf2 = std::move(buf);
+  EXPECT_EQ(buf2.size(), 17u);
+#if RTC_DCHECK_IS_ON
+#if GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
+  EXPECT_DEATH(buf.empty(), "");
+#endif
+#else
+  EXPECT_TRUE(buf.empty());
+#endif
+}
+
 TEST(ZeroOnFreeBufferTest, TestZeroOnSetData) {
   ZeroOnFreeBuffer<uint8_t> buf(kTestData, 7);
   const uint8_t* old_data = buf.data();