[trafficserver] 01/02: TS-4916 Add safety net to avoid H2-infinite-loop deadlock.

[bc...@apache.org]

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

bcall pushed a commit to branch 7.0.x
in repository https://git-dual.apache.org/repos/asf/trafficserver.git

commit 79cbf777e21ff38186b13c99d0084233ba39607d
Author: Gancho Tenev <gt...@gmail.com>
AuthorDate: Mon Oct 17 15:10:12 2016 -0700

    TS-4916 Add safety net to avoid H2-infinite-loop deadlock.
    
    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).
    Added 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).
    
    (cherry picked from commit a6f9337f61c980739e08bbefeb5f6409b7dcdac1)
---
 proxy/http2/Http2ConnectionState.cc | 17 ++++++++++++++---
 proxy/http2/Http2ConnectionState.h  |  2 +-
 proxy/http2/Http2Stream.cc          | 12 ++++++++++++
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc
index 1fd0f88..c14ebf3 100644
--- a/proxy/http2/Http2ConnectionState.cc
+++ b/proxy/http2/Http2ConnectionState.cc
@@ -908,6 +908,10 @@ Http2ConnectionState::create_stream(Http2StreamId new_id)
 
   Http2Stream *new_stream = THREAD_ALLOC_INIT(http2StreamAllocator, this_ethread());
   new_stream->init(new_id, client_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE));
+
+  ink_assert(NULL != new_stream);
+  ink_assert(!stream_list.in(new_stream));
+
   stream_list.push(new_stream);
   if (client_streamid) {
     latest_streamid_in = new_id;
@@ -938,6 +942,7 @@ Http2ConnectionState::find_stream(Http2StreamId id) const
     if (s->get_id() == id) {
       return s;
     }
+    ink_assert(s != s->link.next);
   }
   return NULL;
 }
@@ -952,6 +957,7 @@ Http2ConnectionState::restart_streams()
     if (!s->is_closed() && s->get_state() == HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE && min(this->client_rwnd, s->client_rwnd) > 0) {
       s->send_response_body();
     }
+    ink_assert(s != next);
     s = next;
   }
 }
@@ -963,9 +969,10 @@ Http2ConnectionState::cleanup_streams()
   while (s) {
     Http2Stream *next = s->link.next;
     this->delete_stream(s);
+    ink_assert(s != next);
     s = next;
   }
-  ink_assert(stream_list.head == NULL);
+  ink_assert(stream_list.empty());
 
   if (!is_state_closed()) {
     ua_session->get_netvc()->add_to_keep_alive_queue();
@@ -973,12 +980,14 @@ Http2ConnectionState::cleanup_streams()
   }
 }
 
-void
+bool
 Http2ConnectionState::delete_stream(Http2Stream *stream)
 {
+  ink_assert(NULL != stream);
+
   // If stream has already been removed from the list, just go on
   if (!stream_list.in(stream)) {
-    return;
+    return false;
   }
 
   DebugHttp2Stream(ua_session, stream->get_id(), "Delete stream");
@@ -999,6 +1008,8 @@ Http2ConnectionState::delete_stream(Http2Stream *stream)
 
   stream_list.remove(stream);
   stream->initiating_close();
+
+  return true;
 }
 
 void
diff --git a/proxy/http2/Http2ConnectionState.h b/proxy/http2/Http2ConnectionState.h
index eb6f930..7f52927 100644
--- a/proxy/http2/Http2ConnectionState.h
+++ b/proxy/http2/Http2ConnectionState.h
@@ -174,7 +174,7 @@ public:
   Http2Stream *create_stream(Http2StreamId new_id);
   Http2Stream *find_stream(Http2StreamId id) const;
   void restart_streams();
-  void delete_stream(Http2Stream *stream);
+  bool delete_stream(Http2Stream *stream);
   void release_stream(Http2Stream *stream);
   void cleanup_streams();
 
diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc
index 8b80d1b..4d3a07b 100644
--- a/proxy/http2/Http2Stream.cc
+++ b/proxy/http2/Http2Stream.cc
@@ -675,6 +675,18 @@ Http2Stream::destroy()
   }
   chunked_handler.clear();
   super::destroy();
+
+  // 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 (parent) {
+    if (static_cast<Http2ClientSession *>(parent)->connection_state.delete_stream(this)) {
+      Warning("Http2Stream was about to be deallocated without removing it from the active stream list");
+    }
+  }
+
   THREAD_FREE(this, http2StreamAllocator, this_ethread());
 }
 

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