[base] Check the arithmetic in `ReleaseImpl` as well as in `AddRefImpl`.

Bug: none
Change-Id: Iff7d64dd0ede989f260622c9249983530f55deb8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1622483Reviewed-by: Albert J. Wong <ajwong@chromium.org>
Commit-Queue: Chris Palmer <palmer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662933}

base/memory/ref_counted.cc

@@ -4,6 +4,9 @@
 
 #include "base/memory/ref_counted.h"
 
+#include <limits>
+#include <type_traits>
+
 #include "base/threading/thread_collision_warner.h"
 
 namespace base {
@@ -32,15 +35,35 @@ RefCountedThreadSafeBase::~RefCountedThreadSafeBase() {
 }
 #endif
 
-// This is a security check. In 32-bit-archs, an attacker would run out of
-// address space after allocating at most 2^32 scoped_refptrs. This replicates
-// that boundary for 64-bit-archs.
+// For security and correctness, we check the arithmetic on ref counts.
+//
+// In an attempt to avoid binary bloat (from inlining the `CHECK`), we define
+// these functions out-of-line. However, compilers are wily. Further testing may
+// show that `NOINLINE` helps or hurts.
+//
 #if defined(ARCH_CPU_64_BITS)
 void RefCountedBase::AddRefImpl() const {
-  // Check if |ref_count_| overflow only on 64 bit archs since the number of
-  // objects may exceed 2^32.
-  // To avoid the binary size bloat, use non-inline function here.
-  CHECK(++ref_count_ > 0);
+  // An attacker could induce use-after-free bugs, and potentially exploit them,
+  // by creating so many references to a ref-counted object that the reference
+  // count overflows. On 32-bit architectures, there is not enough address space
+  // to succeed. But on 64-bit architectures, it might indeed be possible.
+  // Therefore, we can elide the check for arithmetic overflow on 32-bit, but we
+  // must check on 64-bit.
+  //
+  // Make sure the addition didn't wrap back around to 0. This form of check
+  // works because we assert that `ref_count_` is an unsigned integer type.
+  CHECK(++ref_count_ != 0);
+}
+
+void RefCountedBase::ReleaseImpl() const {
+  // Make sure the subtraction didn't wrap back around from 0 to the max value.
+  // That could cause memory leaks, and may induce application-semantic
+  // correctness or safety bugs. (E.g. what if we really needed that object to
+  // be destroyed at the right time?)
+  //
+  // Note that unlike with overflow, underflow could also happen on 32-bit
+  // architectures. Arguably, we should do this check on32-bit machines too.
+  CHECK(--ref_count_ != std::numeric_limits<decltype(ref_count_)>::max());
 }
 #endif
 

base/memory/ref_counted.h

@@ -69,7 +69,7 @@ class BASE_EXPORT RefCountedBase {
 
   // Returns true if the object should self-delete.
   bool Release() const {
-    --ref_count_;
+    ReleaseImpl();
 
     // TODO(maruel): Add back once it doesn't assert 500 times/sec.
     // Current thread books the critical section "AddRelease"
@@ -126,8 +126,10 @@ class BASE_EXPORT RefCountedBase {
 
 #if defined(ARCH_CPU_64_BITS)
   void AddRefImpl() const;
+  void ReleaseImpl() const;
 #else
   void AddRefImpl() const { ++ref_count_; }
+  void ReleaseImpl() const { --ref_count_; }
 #endif
 
 #if DCHECK_IS_ON()
@@ -135,6 +137,8 @@ class BASE_EXPORT RefCountedBase {
 #endif
 
   mutable uint32_t ref_count_ = 0;
+  static_assert(std::is_unsigned<decltype(ref_count_)>::value,
+                "ref_count_ must be an unsigned type.");
 
 #if DCHECK_IS_ON()
   mutable bool needs_adopt_ref_ = false;