inspector: Do cleanups before notifying callback

Inspector socket implementation was notifying handshake callback before performing the cleanups, which meant that callback could not reclaim resources allocated by the client. New implementation will free all resource not allocated by the client before calling the callback, allowing the client to complete the cleanup. Fixes: #7418 PR-URL: #7450 Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
nodejs · Jul 5, 2016 · c4aaf47 · c4aaf47
1 parent d857e9a
commit c4aaf47
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 23 deletions.
diff --git a/src/inspector_socket.cc b/src/inspector_socket.cc
@@ -22,6 +22,7 @@ struct http_parsing_state_s {
   http_parser parser;
   http_parser_settings parser_settings;
   handshake_cb callback;
+  bool done;
   bool parsing_value;
   char* ws_key;
   char* path;
@@ -482,24 +483,42 @@ static void handshake_complete(inspector_socket_t* inspector) {
            inspector->http_parsing_state->path);
 }
 
-static void cleanup_http_parsing_state(struct http_parsing_state_s* state) {
+static void cleanup_http_parsing_state(inspector_socket_t* inspector) {
+  struct http_parsing_state_s* state = inspector->http_parsing_state;
   free(state->current_header);
   free(state->path);
   free(state->ws_key);
   free(state);
+  inspector->http_parsing_state = nullptr;
+}
+
+static void report_handshake_failure_cb(uv_handle_t* handle) {
+  dispose_inspector(handle);
+  inspector_socket_t* inspector =
+      static_cast<inspector_socket_t*>(handle->data);
+  handshake_cb cb = inspector->http_parsing_state->callback;
+  cleanup_http_parsing_state(inspector);
+  cb(inspector, kInspectorHandshakeFailed, nullptr);
+}
+
+static void close_and_report_handshake_failure(inspector_socket_t* inspector) {
+  uv_handle_t* socket = reinterpret_cast<uv_handle_t*>(&inspector->client);
+  if (uv_is_closing(socket)) {
+    report_handshake_failure_cb(socket);
+  } else {
+    uv_read_stop(reinterpret_cast<uv_stream_t*>(socket));
+    uv_close(socket, report_handshake_failure_cb);
+  }
 }
 
 static void handshake_failed(inspector_socket_t* inspector) {
-  http_parsing_state_s* state = inspector->http_parsing_state;
   const char HANDSHAKE_FAILED_RESPONSE[] =
       "HTTP/1.0 400 Bad Request\r\n"
       "Content-Type: text/html; charset=UTF-8\r\n\r\n"
       "WebSockets request was expected\r\n";
   write_to_client(inspector, HANDSHAKE_FAILED_RESPONSE,
                   sizeof(HANDSHAKE_FAILED_RESPONSE) - 1);
-  close_connection(inspector);
-  inspector->http_parsing_state = nullptr;
-  state->callback(inspector, kInspectorHandshakeFailed, state->path);
+  close_and_report_handshake_failure(inspector);
 }
 
 // init_handshake references message_complete_cb
@@ -542,11 +561,10 @@ static int message_complete_cb(http_parser* parser) {
     int len = sizeof(accept_response);
     if (write_to_client(inspector, accept_response, len) >= 0) {
       handshake_complete(inspector);
+      inspector->http_parsing_state->done = true;
     } else {
-      state->callback(inspector, kInspectorHandshakeFailed, nullptr);
-      close_connection(inspector);
+      close_and_report_handshake_failure(inspector);
     }
-    inspector->http_parsing_state = nullptr;
   } else {
     handshake_failed(inspector);
   }
@@ -565,27 +583,21 @@ static void data_received_cb(uv_stream_s* client, ssize_t nread,
 #endif
   inspector_socket_t* inspector =
       reinterpret_cast<inspector_socket_t*>((client->data));
-  http_parsing_state_s* state = inspector->http_parsing_state;
   if (nread < 0 || nread == UV_EOF) {
-    inspector->http_parsing_state->callback(inspector,
-                                            kInspectorHandshakeFailed,
-                                            nullptr);
-    close_connection(inspector);
-    inspector->http_parsing_state = nullptr;
+    close_and_report_handshake_failure(inspector);
   } else {
+    http_parsing_state_s* state = inspector->http_parsing_state;
     http_parser* parser = &state->parser;
-    ssize_t parsed = http_parser_execute(parser, &state->parser_settings,
-                                         inspector->buffer,
-                                         nread);
-    if (parsed < nread) {
+    http_parser_execute(parser, &state->parser_settings, inspector->buffer,
+                        nread);
+    if (parser->http_errno != HPE_OK) {
       handshake_failed(inspector);
     }
+    if (inspector->http_parsing_state->done) {
+      cleanup_http_parsing_state(inspector);
+    }
     inspector->data_len = 0;
   }
-
-  if (inspector->http_parsing_state == nullptr) {
-    cleanup_http_parsing_state(state);
-  }
 }
 
 static void init_handshake(inspector_socket_t* inspector) {
@@ -600,6 +612,7 @@ static void init_handshake(inspector_socket_t* inspector) {
   if (state->path) {
     state->path[0] = '\0';
   }
+  state->done = false;
   http_parser_init(&state->parser, HTTP_REQUEST);
   state->parser.data = inspector;
   http_parser_settings* settings = &state->parser_settings;

diff --git a/test/cctest/test_inspector_socket.cc b/test/cctest/test_inspector_socket.cc
@@ -628,6 +628,8 @@ static void ReportsHttpGet_handshake(enum inspector_handshake_event state,
     break;
   case 5:
     expected_state = kInspectorHandshakeFailed;
+    expected_path = nullptr;
+    break;
   case 4:
     expected_path = "/close";
     *cont = false;
@@ -677,15 +679,16 @@ HandshakeCanBeCanceled_handshake(enum inspector_handshake_event state,
   switch (handshake_events - 1) {
   case 0:
     EXPECT_EQ(kInspectorHandshakeUpgrading, state);
+    EXPECT_STREQ("/ws/path", path);
     break;
   case 1:
     EXPECT_EQ(kInspectorHandshakeFailed, state);
+    EXPECT_STREQ(nullptr, path);
     break;
   default:
     EXPECT_TRUE(false);
     break;
   }
-  EXPECT_STREQ("/ws/path", path);
   *cont = false;
 }