Refactor LinkNode and add CHECKs for unsafe conditions

Calling base::LinkNode::Insert{Before,After} is invalid if it's
already in a list and can easily lead to undefined behavior. This
extracts them into a non-templated base and adds CHECKs.

Bug: None
Change-Id: Ib00fcd9ba513181336c4b02e846fa935fe5f46cf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2405709
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809092}

base/BUILD.gn

@@ -207,6 +207,7 @@ component("base") {
     "containers/id_map.h",
     "containers/intrusive_heap.cc",
     "containers/intrusive_heap.h",
+    "containers/linked_list.cc",
     "containers/linked_list.h",
     "containers/mru_cache.h",
     "containers/small_map.h",

base/containers/linked_list.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/containers/linked_list.h"
+
+#include "base/check_op.h"
+
+namespace base {
+
+namespace internal {
+
+LinkNodeBase::LinkNodeBase() = default;
+
+LinkNodeBase::LinkNodeBase(LinkNodeBase* previous, LinkNodeBase* next)
+    : previous_(previous), next_(next) {}
+
+LinkNodeBase::LinkNodeBase(LinkNodeBase&& rhs) {
+  next_ = rhs.next_;
+  rhs.next_ = nullptr;
+  previous_ = rhs.previous_;
+  rhs.previous_ = nullptr;
+
+  // If the node belongs to a list, next_ and previous_ are both non-null.
+  // Otherwise, they are both null.
+  if (next_) {
+    next_->previous_ = this;
+    previous_->next_ = this;
+  }
+}
+
+void LinkNodeBase::RemoveFromList() {
+  previous_->next_ = next_;
+  next_->previous_ = previous_;
+  // next() and previous() return non-null if and only this node is not in any
+  // list.
+  next_ = nullptr;
+  previous_ = nullptr;
+}
+
+void LinkNodeBase::InsertBeforeBase(LinkNodeBase* e) {
+  CHECK_EQ(previous_, nullptr);
+  CHECK_EQ(next_, nullptr);
+  next_ = e;
+  previous_ = e->previous_;
+  e->previous_->next_ = this;
+  e->previous_ = this;
+}
+
+void LinkNodeBase::InsertAfterBase(LinkNodeBase* e) {
+  CHECK_EQ(previous_, nullptr);
+  CHECK_EQ(next_, nullptr);
+  next_ = e->next_;
+  previous_ = e;
+  e->next_->previous_ = this;
+  e->next_ = this;
+}
+
+}  // namespace internal
+
+}  // namespace base

base/containers/linked_list.h

@@ -5,6 +5,8 @@
 #ifndef BASE_CONTAINERS_LINKED_LIST_H_
 #define BASE_CONTAINERS_LINKED_LIST_H_
 
+#include "base/check_op.h"
+
 // Simple LinkedList type. (See the Q&A section to understand how this
 // differs from std::list).
 //
@@ -28,7 +30,7 @@
 //
 //   list.Append(n1);
 //   list.Append(n3);
-//   n3->InsertBefore(n3);
+//   n2->InsertBefore(n3);
 //
 // Lastly, to iterate through the linked list forwards:
 //
@@ -79,63 +81,63 @@
 
 namespace base {
 
+namespace internal {
+
+// Base class for LinkNode<T> type
+class BASE_EXPORT LinkNodeBase {
+ public:
+  void RemoveFromList();
+
+ protected:
+  LinkNodeBase();
+  LinkNodeBase(LinkNodeBase* previous, LinkNodeBase* next);
+  LinkNodeBase(LinkNodeBase&& rhs);
+  LinkNodeBase(const LinkNodeBase&) = delete;
+  ~LinkNodeBase() = default;
+
+  LinkNodeBase& operator=(const LinkNodeBase&) = delete;
+
+  // Calling these with |e| as a different LinkNode type as |this| is
+  // unsafe. These are protected and only called from LinkNode<T> to
+  // ensure safety.
+  void InsertBeforeBase(LinkNodeBase* e);
+  void InsertAfterBase(LinkNodeBase* e);
+
+  LinkNodeBase* previous_base() const { return previous_; }
+  LinkNodeBase* next_base() const { return next_; }
+
+ private:
+  LinkNodeBase* previous_ = nullptr;
+  LinkNodeBase* next_ = nullptr;
+};
+
+}  // namespace internal
+
 template <typename T>
-class LinkNode {
+class LinkNode : public internal::LinkNodeBase {
  public:
-  LinkNode() : previous_(nullptr), next_(nullptr) {}
+  LinkNode() = default;
   LinkNode(LinkNode<T>* previous, LinkNode<T>* next)
-      : previous_(previous), next_(next) {}
-
-  LinkNode(LinkNode<T>&& rhs) {
-    next_ = rhs.next_;
-    rhs.next_ = nullptr;
-    previous_ = rhs.previous_;
-    rhs.previous_ = nullptr;
-
-    // If the node belongs to a list, next_ and previous_ are both non-null.
-    // Otherwise, they are both null.
-    if (next_) {
-      next_->previous_ = this;
-      previous_->next_ = this;
-    }
-  }
+      : internal::LinkNodeBase(previous, next) {}
+
+  LinkNode(LinkNode<T>&&) = default;
 
   LinkNode(const LinkNode&) = delete;
   LinkNode& operator=(const LinkNode&) = delete;
 
-  // Insert |this| into the linked list, before |e|.
-  void InsertBefore(LinkNode<T>* e) {
-    this->next_ = e;
-    this->previous_ = e->previous_;
-    e->previous_->next_ = this;
-    e->previous_ = this;
-  }
-
-  // Insert |this| into the linked list, after |e|.
-  void InsertAfter(LinkNode<T>* e) {
-    this->next_ = e->next_;
-    this->previous_ = e;
-    e->next_->previous_ = this;
-    e->next_ = this;
-  }
+  // Insert |this| into the linked list, before |e|. |this| must not
+  // already be in a list.
+  void InsertBefore(LinkNode<T>* e) { InsertBeforeBase(e); }
 
-  // Remove |this| from the linked list.
-  void RemoveFromList() {
-    this->previous_->next_ = this->next_;
-    this->next_->previous_ = this->previous_;
-    // next() and previous() return non-null if and only this node is not in any
-    // list.
-    this->next_ = nullptr;
-    this->previous_ = nullptr;
-  }
+  // Insert |this| into the linked list, after |e|. |this| must not
+  // already be in a list.
+  void InsertAfter(LinkNode<T>* e) { InsertAfterBase(e); }
 
   LinkNode<T>* previous() const {
-    return previous_;
+    return static_cast<LinkNode<T>*>(previous_base());
   }
 
-  LinkNode<T>* next() const {
-    return next_;
-  }
+  LinkNode<T>* next() const { return static_cast<LinkNode<T>*>(next_base()); }
 
   // Cast from the node-type to the value type.
   const T* value() const {
@@ -145,10 +147,6 @@ class LinkNode {
   T* value() {
     return static_cast<T*>(this);
   }
-
- private:
-  LinkNode<T>* previous_;
-  LinkNode<T>* next_;
 };
 
 template <typename T>

base/containers/linked_list_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "base/containers/linked_list.h"
 #include "base/stl_util.h"
+#include "base/test/gtest_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
@@ -345,5 +346,37 @@ TEST(LinkedList, NodeMoveConstructor) {
   EXPECT_EQ(2, n2_new.id());
 }
 
+TEST(LinkedListDeathTest, ChecksOnInsertBeforeWhenInList) {
+  LinkedList<Node> list1;
+  LinkedList<Node> list2;
+
+  Node n1(1);
+  Node n2(2);
+  Node n3(3);
+
+  list1.Append(&n1);
+
+  list2.Append(&n2);
+  list2.Append(&n3);
+
+  EXPECT_CHECK_DEATH(n1.InsertBefore(&n3));
+}
+
+TEST(LinkedListDeathTest, ChecksOnInsertAfterWhenInList) {
+  LinkedList<Node> list1;
+  LinkedList<Node> list2;
+
+  Node n1(1);
+  Node n2(2);
+  Node n3(3);
+
+  list1.Append(&n1);
+
+  list2.Append(&n2);
+  list2.Append(&n3);
+
+  EXPECT_CHECK_DEATH(n1.InsertAfter(&n2));
+}
+
 }  // namespace
 }  // namespace base