[trafficserver] branch 7.1.x updated: Fix crash in H2 priority tree cleanup.

[zw...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

zwoop pushed a commit to branch 7.1.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git


The following commit(s) were added to refs/heads/7.1.x by this push:
     new 87f8b46  Fix crash in H2 priority tree cleanup.
87f8b46 is described below

commit 87f8b46a5abc403d29b1ae4e598c8ba9a11996c5
Author: Susan Hinrichs <sh...@apache.org>
AuthorDate: Tue Nov 7 00:08:36 2017 +0000

    Fix crash in H2 priority tree cleanup.
    
    (cherry picked from commit 4e5f024ebc0ea439371aa079f9a90a4bc419170e)
---
 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 8337615..10e20c6 100644
--- a/proxy/http2/Http2ConnectionState.cc
+++ b/proxy/http2/Http2ConnectionState.cc
@@ -1100,6 +1100,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)) {
@@ -1115,7 +1116,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_CLOSED) {
@@ -1141,7 +1144,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 f0a5692..39b2165 100644
--- a/proxy/http2/Http2DependencyTree.h
+++ b/proxy/http2/Http2DependencyTree.h
@@ -118,6 +118,7 @@ public:
   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:
@@ -201,6 +202,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
 }
 
 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)
 {
@@ -241,6 +264,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 8ba5e19..7028704 100644
--- a/proxy/http2/Http2Stream.cc
+++ b/proxy/http2/Http2Stream.cc
@@ -666,21 +666,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

-- 
To stop receiving notification emails like this one, please contact
['"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>'].