pw_containers: Intrusive list item auto-removal

Previously, it was easy to accidentally have dangling use-after-free errors with intrusive list items. This patch changes the logic so that items remove themselves from their parent list during destruction. Additionally, this changes IntrusiveList<T>::Item's semantics so that instead of a nullptr to indicating an unlisted element, a self-cycle indicates unlisted. This leads to more elegant logic. Change-Id: I5de663f7c67112b4b9465373d64c0d57b05bd892 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/16260 Reviewed-by: Armando Montanez <[email protected]> Commit-Queue: Keir Mierle <[email protected]>
AlexPygin · Aug 19, 2020 · 69cd61d · 69cd61d
1 parent b5b75e5
commit 69cd61d
Show file tree

Hide file tree

Showing 4 changed files with 171 additions and 108 deletions.
diff --git a/pw_containers/BUILD.gn b/pw_containers/BUILD.gn
@@ -40,7 +40,7 @@ pw_source_set("intrusive_list") {
     "public/pw_containers/internal/intrusive_list_impl.h",
     "public/pw_containers/intrusive_list.h",
   ]
-  deps = [ "$dir_pw_assert" ]
+  deps = [ dir_pw_assert ]
   sources = [ "intrusive_list.cc" ]
 }
 

diff --git a/pw_containers/intrusive_list.cc b/pw_containers/intrusive_list.cc
@@ -18,28 +18,39 @@
 
 namespace pw::intrusive_list_impl {
 
+List::Item::~Item() { unlist(); }
+
+void List::Item::unlist(Item* prev) {
+  if (prev == nullptr) {
+    prev = previous();
+  }
+  // Skip over this.
+  prev->next_ = next_;
+
+  // Retain the invariant that unlisted items are self-cycles.
+  next_ = this;
+}
+
+List::Item* List::Item::previous() {
+  // Follow the cycle around to find the previous element; O(N).
+  Item* prev = next_;
+  while (prev->next_ != this) {
+    prev = prev->next_;
+  }
+  return prev;
+}
+
 void List::insert_after(Item* pos, Item& item) {
-  PW_CHECK_PTR_EQ(
-      item.next_,
-      nullptr,
+  PW_CHECK(
+      item.unlisted(),
       "Cannot add an item to a pw::IntrusiveList that is already in a list");
   item.next_ = pos->next_;
   pos->next_ = &item;
 }
 
-void List::erase_after(Item* pos) {
-  Item* const item_to_remove = pos->next_;
-  pos->next_ = item_to_remove->next_;
-  item_to_remove->next_ = nullptr;
-}
+void List::erase_after(Item* pos) { pos->next_->unlist(pos); }
 
-List::Item* List::before_end() noexcept {
-  Item* pos = before_begin();
-  while (pos->next_ != end()) {
-    pos = pos->next_;
-  }
-  return pos;
-}
+List::Item* List::before_end() noexcept { return before_begin()->previous(); }
 
 void List::clear() {
   while (!empty()) {
@@ -54,7 +65,6 @@ bool List::remove(const Item& item_to_remove) {
       return true;
     }
   }
-
   return false;
 }