Add a check that sanitizers cause a fatal failure

webrtc/rtc_tools/sanitizers_unittest.cc

+/*
+ *  Copyright (c) 2017 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 <stddef.h>
+
+#include "webrtc/rtc_base/checks.h"
+#include "webrtc/rtc_base/nullsocketserver.h"
+#include "webrtc/rtc_base/thread.h"
+#include "webrtc/test/gtest.h"
+
+namespace rtc {
+
+namespace {
+
+#if defined(MEMORY_SANITIZER)

[kwiberg-webrtc, 2017/09/13 11:59:23]

There are preprocessor symbols here
[https://chromium.googlesource.com/external/webrtc/+/HEAD/webrtc/rtc_base/sanitizer.h#16]
that are set to 0 or 1. Are the ones you're using here better somehow?

[oprypin_webrtc, 2017/09/13 14:48:15]

I didn't know about these. I got my example from
https://codereview.webrtc.org/3009123002/patch/160001/170010
But there are 4 kinds of sanitizer macros, while the ones you're suggesting are
only 2.

[kwiberg-webrtc, 2017/09/14 02:15:02]

OK, FOO_SANITIZER seems to be set by Chromium's GN files. Using our own macros
from sanitizer.h seems generally safer and cleaner. It'd be great if you could
add the missing ones to sanitizer.h. (Alternatively, just keep using Chromium's
macros here, since it looks like I'll have to sweep the tree clean of all of
these anyway...)

+void UseOfUninitializedValue() {
+  int* buf = new int[2];
+  if (buf[1]) {
+    ++buf[0];
+  }
+  delete[] buf;
+}
+
+TEST(SanitizersDeathTest, MemorySanitizer) {
+  UseOfUninitializedValue();

[kwiberg-webrtc, 2017/09/13 11:59:23]

Hmm. Why don't we expect to die here?

[oprypin_webrtc, 2017/09/13 14:48:15]

Hmm indeed. It was incorrect. Had to rework this.

+}
+#endif
+
+#if defined(ADDRESS_SANITIZER)
+void HeapUseAfterFree() {
+  char *buf = new char[2];
+  delete[] buf;
+  buf[0] = buf[1];
+}
+
+TEST(SanitizersDeathTest, AddressSanitizer) {
+  EXPECT_DEATH(HeapUseAfterFree(), "heap-use-after-free");
+}
+#endif
+
+#if defined(UNDEFINED_SANITIZER)
+// For ubsan:
+void SignedIntegerOverflow() {
+  int32_t x = 1234567890;
+  x *= 2;
+}
+
+// For ubsan_vptr:
+struct Base {
+  virtual void f() = 0;
+  virtual ~Base() {}
+};
+struct Derived : public Base {
+  virtual void f() {}
+};

[kwiberg-webrtc, 2017/09/14 02:15:02]

IIUC, you don't need the subclass or a virtual destructor for ubsan_vptr to
trigger. You should just need to call virtual function f() on a deleted object.

Alternatively---or additionally---keep the current hierarchy, and do something
like

  Base b;
  auto* d = static_cast<Derived*>(&b);  // bad down cast
  d->f();  // virtual function call with object of wrong dynamic type

 ("-fsanitize=vptr: Use of an object whose vptr indicates that it is of the
wrong dynamic type, or that its lifetime has not begun or has ended. ")

[oprypin_webrtc, 2017/09/14 06:48:33]

OK, I used the alternative implementation. It's nicer not to rely on a null
pointer.

[kwiberg-webrtc, 2017/09/14 08:23:22]

Acknowledged.

+
+void InvalidVptr() {
+  Base* ptr = new Derived;
+  delete ptr;
+  ptr->f();
+}
+
+TEST(SanitizersDeathTest, UndefinedSanitizer) {
+  EXPECT_DEATH({ SignedIntegerOverflow(); InvalidVptr(); },

[kwiberg-webrtc, 2017/09/13 11:59:23]

Why are you doing two lethal things here?

[oprypin_webrtc, 2017/09/13 14:48:15]

ubsan fails on only one of them, ubsan_vptr only fails on the other one, and I
can't discern which one is currently running.

[kwiberg-webrtc, 2017/09/14 02:15:02]

Hmm, yeah, I see. And it looks like it'll be messy to make GN tell us if it's on
or not. Problem is, this test only proves that both ubsan and ubsan_vptr fail on
at least one of the tests. :-(

+               "runtime error");
+}
+#endif
+
+#if defined(THREAD_SANITIZER)
+class IncrementThread : public Thread {
+ public:
+  explicit IncrementThread(int* value)
+      : Thread(std::unique_ptr<SocketServer>(new NullSocketServer())),

[kwiberg-webrtc, 2017/09/13 11:59:23]

rtc::MakeUnique<NullSocketServer>()

[oprypin_webrtc, 2017/09/13 14:48:15]

Done.

+        value_(value) {}
+
+  void Run() override {
+    ++*value_;
+    Thread::Current()->SleepMs(100);
+  }
+
+  // Un-protect Thread::Join for the test.
+  void Join() {
+    Thread::Join();
+  }
+
+ private:
+  int* value_;
+
+  RTC_DISALLOW_COPY_AND_ASSIGN(IncrementThread);
+};
+
+void DataRace() {
+  int value = 0;
+  IncrementThread thread1(&value);
+  IncrementThread thread2(&value);
+  thread1.Start();
+  thread2.Start();
+  thread1.Join();
+  thread2.Join();
+  // TSan seems to mess with gtest's death detection.
+  // Fail intentionally, and rely on detecting the error message.
+  RTC_CHECK(false);
+}
+
+TEST(SanitizersDeathTest, ThreadSanitizer) {
+  EXPECT_DEATH(DataRace(), "data race");
+}
+#endif
+
+}  // namespace
+
+}  // namespace rtc