From 850feece61e58dd482cd07d8908b991467775f3a Mon Sep 17 00:00:00 2001
From: Susan Hinrichs <shinrich@apache.org>
Date: Tue, 7 Nov 2017 00:08:36 +0000
Subject: [PATCH] Fix crash in H2 priority tree cleanup.

---
 proxy/http2/Http2ConnectionState.cc |  4 +++-
 proxy/http2/Http2DependencyTree.h   | 25 +++++++++++++++++++++++++
 proxy/http2/Http2Stream.cc          | 17 +++++------------
 3 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc
index 6cfe32f017d..7f70822ecef 100644
--- a/proxy/http2/Http2ConnectionState.cc
+++ b/proxy/http2/Http2ConnectionState.cc
@@ -1105,6 +1105,7 @@ bool
 Http2ConnectionState::delete_stream(Http2Stream *stream)
 {
   ink_assert(nullptr != stream);
+  SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
 
   // If stream has already been removed from the list, just go on
   if (!stream_list.in(stream)) {
@@ -1120,7 +1121,9 @@ Http2ConnectionState::delete_stream(Http2Stream *stream)
         dependency_tree->deactivate(node, 0);
       }
       dependency_tree->remove(node);
+      // ink_release_assert(dependency_tree->find(stream->get_id()) == nullptr);
     }
+    stream->priority_node = nullptr;
   }
 
   if (stream->get_state() == Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL) {
@@ -1146,7 +1149,6 @@ Http2ConnectionState::release_stream(Http2Stream *stream)
       ink_assert(client_streams_out_count > 0);
       --client_streams_out_count;
     }
-    stream_list.remove(stream);
   }
 
   if (ua_session) {
diff --git a/proxy/http2/Http2DependencyTree.h b/proxy/http2/Http2DependencyTree.h
index d4b44ee7bab..3f3af9ba562 100644
--- a/proxy/http2/Http2DependencyTree.h
+++ b/proxy/http2/Http2DependencyTree.h
@@ -119,6 +119,7 @@ template <typename T> class Tree
   void activate(Node *node);
   void deactivate(Node *node, uint32_t sent);
   void update(Node *node, uint32_t sent);
+  bool in(Node *current, Node *node);
   uint32_t size() const;
 
 private:
@@ -202,6 +203,7 @@ Tree<T>::add(uint32_t parent_id, uint32_t id, uint32_t weight, bool exclusive, T
 
   parent->children.push(node);
   if (!node->queue->empty()) {
+    ink_release_assert(!node->queued);
     parent->queue->push(node->entry);
     node->queued = true;
   }
@@ -210,6 +212,27 @@ Tree<T>::add(uint32_t parent_id, uint32_t id, uint32_t weight, bool exclusive, T
   return node;
 }
 
+template <typename T>
+bool
+Tree<T>::in(Node *current, Node *node)
+{
+  bool retval = false;
+  if (current == nullptr)
+    current = _root;
+  if (current->queue->in(node->entry)) {
+    return true;
+  } else {
+    Node *child = current->children.head;
+    while (child) {
+      if (in(child, node)) {
+        return true;
+      }
+      child = child->link.next;
+    }
+  }
+  return retval;
+}
+
 template <typename T>
 void
 Tree<T>::remove(Node *node)
@@ -242,6 +265,8 @@ Tree<T>::remove(Node *node)
     remove(parent);
   }
 
+  // ink_release_assert(!this->in(nullptr, node));
+
   --_node_count;
   delete node;
 }
diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc
index 23eff855b44..38e55abd0d1 100644
--- a/proxy/http2/Http2Stream.cc
+++ b/proxy/http2/Http2Stream.cc
@@ -664,21 +664,14 @@ Http2Stream::destroy()
 
   // Safe to initiate SSN_CLOSE if this is the last stream
   if (parent) {
-    // release_stream and delete_stream indirectly call each other and seem to have a lot of commonality
-    // Should get resolved at somepoint.
     Http2ClientSession *h2_parent = static_cast<Http2ClientSession *>(parent);
     SCOPED_MUTEX_LOCK(lock, h2_parent->connection_state.mutex, this_ethread());
-    h2_parent->connection_state.release_stream(this);
+    // Make sure the stream is removed from the stream list and priority tree
+    // In many cases, this has been called earlier, so this call is a no-op
+    h2_parent->connection_state.delete_stream(this);
 
-    // Current Http2ConnectionState implementation uses a memory pool for instantiating streams and DLL<> stream_list for storing
-    // active streams. Destroying a stream before deleting it from stream_list and then creating a new one + reusing the same chunk
-    // from the memory pool right away always leads to destroying the DLL structure (deadlocks, inconsistencies).
-    // The following is meant as a safety net since the consequences are disastrous. Until the design/implementation changes it
-    // seems
-    // less error prone to (double) delete before destroying (noop if already deleted).
-    if (h2_parent->connection_state.delete_stream(this)) {
-      Warning("Http2Stream was about to be deallocated without removing it from the active stream list");
-    }
+    // Update session's stream counts, so it accurately goes into keep-alive state
+    h2_parent->connection_state.release_stream(this);
   }
 
   // Clean up the write VIO in case of inactivity timeout