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: https://github.com/nodejs/node/pull/7418
PR-URL: https://github.com/nodejs/node/pull/7450
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

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;

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;
 }