src: avoid manual memory management in inspector

Make the inspector code easier to reason about by restructuring it
to avoid manual memory allocation and copying as much as possible.

An amusing side effect is that it reduces the total amount of memory
used in the test suite.

Before:

    $ valgrind ./out/Release/cctest 2>&1 | grep 'total heap' | cut -c31-
    1,017 allocs, 1,017 frees, 21,695,456 allocated

After:

    $ valgrind ./out/Release/cctest 2>&1 | grep 'total heap' | cut -c31-
    869 allocs, 869 frees, 14,484,641 bytes allocated

PR-URL: https://github.com/nodejs/node/pull/7906
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

1 files changed, 71 insertions, 138 deletions

diff --git a/src/inspector_socket.cc b/src/inspector_socket.cc
index d3a8d8efff..c360ef0db9 100644
--- a/src/inspector_socket.cc
+++ b/src/inspector_socket.cc
@@ -18,25 +18,6 @@
 
 static const char CLOSE_FRAME[] = {'\x88', '\x00'};
 
-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;
-  char* current_header;
-};
-
-struct ws_state_s {
-  uv_alloc_cb alloc_cb;
-  uv_read_cb read_cb;
-  inspector_cb close_cb;
-  bool close_sent;
-  bool received_close;
-};
-
 enum ws_decode_result {
   FRAME_OK, FRAME_INCOMPLETE, FRAME_CLOSE, FRAME_ERROR
 };
@@ -72,11 +53,9 @@ static void dispose_inspector(uv_handle_t* handle) {
       reinterpret_cast<inspector_socket_t*>(handle->data);
   inspector_cb close =
       inspector->ws_mode ? inspector->ws_state->close_cb : nullptr;
-  free(inspector->buffer);
-  free(inspector->ws_state);
+  inspector->buffer.clear();
+  delete inspector->ws_state;
   inspector->ws_state = nullptr;
-  inspector->buffer = nullptr;
-  inspector->buffer_size = 0;
   inspector->data_len = 0;
   inspector->last_read_end = 0;
   if (close) {
@@ -92,11 +71,25 @@ static void close_connection(inspector_socket_t* inspector) {
   }
 }
 
+struct WriteRequest {
+  WriteRequest(inspector_socket_t* inspector, const char* data, size_t size)
+      : inspector(inspector)
+      , storage(data, data + size)
+      , buf(uv_buf_init(&storage[0], storage.size())) {}
+
+  static WriteRequest* from_write_req(uv_write_t* req) {
+    return node::ContainerOf(&WriteRequest::req, req);
+  }
+
+  inspector_socket_t* const inspector;
+  std::vector<char> storage;
+  uv_write_t req;
+  uv_buf_t buf;
+};
+
 // Cleanup
 static void write_request_cleanup(uv_write_t* req, int status) {
-  free((reinterpret_cast<uv_buf_t*>(req->data))->base);
-  free(req->data);
-  free(req);
+  delete WriteRequest::from_write_req(req);
 }
 
 static int write_to_client(inspector_socket_t* inspector,
@@ -109,21 +102,9 @@ static int write_to_client(inspector_socket_t* inspector,
 #endif
 
   // Freed in write_request_cleanup
-  uv_buf_t* buf = reinterpret_cast<uv_buf_t*>(malloc(sizeof(uv_buf_t)));
-  uv_write_t* req = reinterpret_cast<uv_write_t*>(malloc(sizeof(uv_write_t)));
-  CHECK_NE(buf, nullptr);
-  CHECK_NE(req, nullptr);
-  memset(req, 0, sizeof(*req));
-  buf->base = reinterpret_cast<char*>(malloc(len));
-
-  CHECK_NE(buf->base, nullptr);
-
-  memcpy(buf->base, msg, len);
-  buf->len = len;
-  req->data = buf;
-
+  WriteRequest* wr = new WriteRequest(inspector, msg, len);
   uv_stream_t* stream = reinterpret_cast<uv_stream_t*>(&inspector->client);
-  return uv_write(req, stream, buf, 1, write_cb) < 0;
+  return uv_write(&wr->req, stream, &wr->buf, 1, write_cb) < 0;
 }
 
 // Constants for hybi-10 frame format.
@@ -278,10 +259,10 @@ static void shutdown_complete(inspector_socket_t* inspector) {
   close_connection(inspector);
 }
 
-static void on_close_frame_written(uv_write_t* write, int status) {
-  inspector_socket_t* inspector =
-      reinterpret_cast<inspector_socket_t*>(write->handle->data);
-  write_request_cleanup(write, status);
+static void on_close_frame_written(uv_write_t* req, int status) {
+  WriteRequest* wr = WriteRequest::from_write_req(req);
+  inspector_socket_t* inspector = wr->inspector;
+  delete wr;
   inspector->ws_state->close_sent = true;
   if (inspector->ws_state->received_close) {
     shutdown_complete(inspector);
@@ -304,7 +285,7 @@ static int parse_ws_frames(inspector_socket_t* inspector, size_t len) {
   std::vector<char> output;
   bool compressed = false;
 
-  ws_decode_result r =  decode_frame_hybi17(inspector->buffer,
+  ws_decode_result r =  decode_frame_hybi17(&inspector->buffer[0],
                                             len, true /* client_frame */,
                                             &bytes_consumed, &output,
                                             &compressed);
@@ -334,16 +315,13 @@ static void prepare_buffer(uv_handle_t* stream, size_t len, uv_buf_t* buf) {
   inspector_socket_t* inspector =
       reinterpret_cast<inspector_socket_t*>(stream->data);
 
-  if (len > (inspector->buffer_size - inspector->data_len)) {
+  if (len > (inspector->buffer.size() - inspector->data_len)) {
     int new_size = (inspector->data_len + len + BUFFER_GROWTH_CHUNK_SIZE - 1) /
                    BUFFER_GROWTH_CHUNK_SIZE *
                    BUFFER_GROWTH_CHUNK_SIZE;
-    inspector->buffer_size = new_size;
-    inspector->buffer = reinterpret_cast<char*>(realloc(inspector->buffer,
-                                        inspector->buffer_size));
-    ASSERT_NE(inspector->buffer, nullptr);
+    inspector->buffer.resize(new_size);
   }
-  buf->base = inspector->buffer + inspector->data_len;
+  buf->base = &inspector->buffer[inspector->data_len];
   buf->len = len;
   inspector->data_len += len;
 }
@@ -366,10 +344,10 @@ static void websockets_data_cb(uv_stream_t* stream, ssize_t nread,
     #endif
     // 1. Move read bytes to continue the buffer
     // Should be same as this is supposedly last buffer
-    ASSERT_EQ(buf->base + buf->len, inspector->buffer + inspector->data_len);
+    ASSERT_EQ(buf->base + buf->len, &inspector->buffer[inspector->data_len]);
 
     // Should be noop...
-    memmove(inspector->buffer + inspector->last_read_end, buf->base, nread);
+    memmove(&inspector->buffer[inspector->last_read_end], buf->base, nread);
     inspector->last_read_end += nread;
 
     // 2. Parse.
@@ -378,8 +356,8 @@ static void websockets_data_cb(uv_stream_t* stream, ssize_t nread,
       processed = parse_ws_frames(inspector, inspector->last_read_end);
       // 3. Fix the buffer size & length
       if (processed > 0) {
-        memmove(inspector->buffer, inspector->buffer + processed,
-            inspector->last_read_end - processed);
+        memmove(&inspector->buffer[0], &inspector->buffer[processed],
+                inspector->last_read_end - processed);
         inspector->last_read_end -= processed;
         inspector->data_len = inspector->last_read_end;
       }
@@ -410,73 +388,53 @@ void inspector_read_stop(inspector_socket_t* inspector) {
   inspector->ws_state->read_cb = nullptr;
 }
 
-static void generate_accept_string(const char* client_key, char* buffer) {
+static void generate_accept_string(const std::string& client_key,
+                                   char (*buffer)[ACCEPT_KEY_LENGTH]) {
   // Magic string from websockets spec.
-  const char ws_magic[] = "258EAFA5-E914-47DA-95CA-C5AB0DC85B11";
-  size_t key_len = strlen(client_key);
-  size_t magic_len = sizeof(ws_magic) - 1;
-
-  char* buf = reinterpret_cast<char*>(malloc(key_len + magic_len));
-  CHECK_NE(buf, nullptr);
-  memcpy(buf, client_key, key_len);
-  memcpy(buf + key_len, ws_magic, magic_len);
-  char hash[20];
-  SHA1((unsigned char*) buf, key_len + magic_len, (unsigned char*) hash);
-  free(buf);
-  node::base64_encode(hash, 20, buffer, ACCEPT_KEY_LENGTH);
-  buffer[ACCEPT_KEY_LENGTH] = '\0';
-}
-
-static void append(char** value, const char* string, size_t length) {
-  const size_t INCREMENT = 500;  // There should never be more then 1 chunk...
-
-  int current_len = *value ? strlen(*value) : 0;
-  int new_len = current_len + length;
-  int adjusted = (new_len / INCREMENT + 1) * INCREMENT;
-  *value = reinterpret_cast<char*>(realloc(*value, adjusted));
-  memcpy(*value + current_len, string, length);
-  (*value)[new_len] = '\0';
+  static const char ws_magic[] = "258EAFA5-E914-47DA-95CA-C5AB0DC85B11";
+  std::string input(client_key + ws_magic);
+  char hash[SHA_DIGEST_LENGTH];
+  SHA1(reinterpret_cast<const unsigned char*>(&input[0]), input.size(),
+       reinterpret_cast<unsigned char*>(hash));
+  node::base64_encode(hash, sizeof(hash), *buffer, sizeof(*buffer));
 }
 
 static int header_value_cb(http_parser* parser, const char* at, size_t length) {
-  char SEC_WEBSOCKET_KEY_HEADER[] = "Sec-WebSocket-Key";
-  struct http_parsing_state_s* state = (struct http_parsing_state_s*)
-      (reinterpret_cast<inspector_socket_t*>(parser->data))->http_parsing_state;
+  static const char SEC_WEBSOCKET_KEY_HEADER[] = "Sec-WebSocket-Key";
+  auto inspector = static_cast<inspector_socket_t*>(parser->data);
+  auto state = inspector->http_parsing_state;
   state->parsing_value = true;
-  if (state->current_header &&
-      node::StringEqualNoCaseN(state->current_header,
+  if (state->current_header.size() == sizeof(SEC_WEBSOCKET_KEY_HEADER) - 1 &&
+      node::StringEqualNoCaseN(state->current_header.data(),
                                SEC_WEBSOCKET_KEY_HEADER,
-                               sizeof(SEC_WEBSOCKET_KEY_HEADER))) {
-    append(&state->ws_key, at, length);
+                               sizeof(SEC_WEBSOCKET_KEY_HEADER) - 1)) {
+    state->ws_key.append(at, length);
   }
   return 0;
 }
 
 static int header_field_cb(http_parser* parser, const char* at, size_t length) {
-  struct http_parsing_state_s* state = (struct http_parsing_state_s*)
-      (reinterpret_cast<inspector_socket_t*>(parser->data))->http_parsing_state;
+  auto inspector = static_cast<inspector_socket_t*>(parser->data);
+  auto state = inspector->http_parsing_state;
   if (state->parsing_value) {
     state->parsing_value = false;
-    if (state->current_header)
-      state->current_header[0] = '\0';
+    state->current_header.clear();
   }
-  append(&state->current_header, at, length);
+  state->current_header.append(at, length);
   return 0;
 }
 
 static int path_cb(http_parser* parser, const char* at, size_t length) {
-  struct http_parsing_state_s* state = (struct http_parsing_state_s*)
-    (reinterpret_cast<inspector_socket_t*>(parser->data))->http_parsing_state;
-  append(&state->path, at, length);
+  auto inspector = static_cast<inspector_socket_t*>(parser->data);
+  auto state = inspector->http_parsing_state;
+  state->path.append(at, length);
   return 0;
 }
 
 static void handshake_complete(inspector_socket_t* inspector) {
   uv_read_stop(reinterpret_cast<uv_stream_t*>(&inspector->client));
   handshake_cb callback = inspector->http_parsing_state->callback;
-  inspector->ws_state = (struct ws_state_s*) malloc(sizeof(struct ws_state_s));
-  ASSERT_NE(nullptr, inspector->ws_state);
-  memset(inspector->ws_state, 0, sizeof(struct ws_state_s));
+  inspector->ws_state = new ws_state_s();
   inspector->last_read_end = 0;
   inspector->ws_mode = true;
   callback(inspector, kInspectorHandshakeUpgraded,
@@ -484,11 +442,7 @@ static void handshake_complete(inspector_socket_t* inspector) {
 }
 
 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);
+  delete inspector->http_parsing_state;
   inspector->http_parsing_state = nullptr;
 }
 
@@ -498,7 +452,7 @@ static void report_handshake_failure_cb(uv_handle_t* handle) {
       static_cast<inspector_socket_t*>(handle->data);
   handshake_cb cb = inspector->http_parsing_state->callback;
   cleanup_http_parsing_state(inspector);
-  cb(inspector, kInspectorHandshakeFailed, nullptr);
+  cb(inspector, kInspectorHandshakeFailed, std::string());
 }
 
 static void close_and_report_handshake_failure(inspector_socket_t* inspector) {
@@ -537,29 +491,21 @@ static int message_complete_cb(http_parser* parser) {
     } else {
       handshake_failed(inspector);
     }
-  } else if (!state->ws_key) {
+  } else if (state->ws_key.empty()) {
     handshake_failed(inspector);
   } else if (state->callback(inspector, kInspectorHandshakeUpgrading,
                              state->path)) {
-    char accept_string[ACCEPT_KEY_LENGTH + 1];
-    generate_accept_string(state->ws_key, accept_string);
-
+    char accept_string[ACCEPT_KEY_LENGTH];
+    generate_accept_string(state->ws_key, &accept_string);
     const char accept_ws_prefix[] = "HTTP/1.1 101 Switching Protocols\r\n"
                                     "Upgrade: websocket\r\n"
                                     "Connection: Upgrade\r\n"
                                     "Sec-WebSocket-Accept: ";
     const char accept_ws_suffix[] = "\r\n\r\n";
-    // Format has two chars (%s) that are replaced with actual key
-    char accept_response[sizeof(accept_ws_prefix) - 1 +
-                         sizeof(accept_ws_suffix) - 1 +
-                         ACCEPT_KEY_LENGTH];
-    memcpy(accept_response, accept_ws_prefix, sizeof(accept_ws_prefix) - 1);
-    memcpy(accept_response + sizeof(accept_ws_prefix) - 1,
-        accept_string, ACCEPT_KEY_LENGTH);
-    memcpy(accept_response + sizeof(accept_ws_prefix) - 1 + ACCEPT_KEY_LENGTH,
-        accept_ws_suffix, sizeof(accept_ws_suffix) - 1);
-    int len = sizeof(accept_response);
-    if (write_to_client(inspector, accept_response, len) >= 0) {
+    std::string reply(accept_ws_prefix, sizeof(accept_ws_prefix) - 1);
+    reply.append(accept_string, sizeof(accept_string));
+    reply.append(accept_ws_suffix, sizeof(accept_ws_suffix) - 1);
+    if (write_to_client(inspector, &reply[0], reply.size()) >= 0) {
       handshake_complete(inspector);
       inspector->http_parsing_state->done = true;
     } else {
@@ -588,7 +534,7 @@ static void data_received_cb(uv_stream_s* client, ssize_t nread,
   } else {
     http_parsing_state_s* state = inspector->http_parsing_state;
     http_parser* parser = &state->parser;
-    http_parser_execute(parser, &state->parser_settings, inspector->buffer,
+    http_parser_execute(parser, &state->parser_settings, &inspector->buffer[0],
                         nread);
     if (parser->http_errno != HPE_OK) {
       handshake_failed(inspector);
@@ -603,15 +549,9 @@ static void data_received_cb(uv_stream_s* client, ssize_t nread,
 static void init_handshake(inspector_socket_t* inspector) {
   http_parsing_state_s* state = inspector->http_parsing_state;
   CHECK_NE(state, nullptr);
-  if (state->current_header) {
-    state->current_header[0] = '\0';
-  }
-  if (state->ws_key) {
-    state->ws_key[0] = '\0';
-  }
-  if (state->path) {
-    state->path[0] = '\0';
-  }
+  state->current_header.clear();
+  state->ws_key.clear();
+  state->path.clear();
   state->done = false;
   http_parser_init(&state->parser, HTTP_REQUEST);
   state->parser.data = inspector;
@@ -626,15 +566,8 @@ static void init_handshake(inspector_socket_t* inspector) {
 int inspector_accept(uv_stream_t* server, inspector_socket_t* inspector,
                      handshake_cb callback) {
   ASSERT_NE(callback, nullptr);
-  // The only field that users should care about.
-  void* data = inspector->data;
-  memset(inspector, 0, sizeof(*inspector));
-  inspector->data = data;
-
-  inspector->http_parsing_state = (struct http_parsing_state_s*)
-      malloc(sizeof(struct http_parsing_state_s));
-  ASSERT_NE(nullptr, inspector->http_parsing_state);
-  memset(inspector->http_parsing_state, 0, sizeof(struct http_parsing_state_s));
+  CHECK_EQ(inspector->http_parsing_state, nullptr);
+  inspector->http_parsing_state = new http_parsing_state_s();
   uv_stream_t* client = reinterpret_cast<uv_stream_t*>(&inspector->client);
   CHECK_NE(client, nullptr);
   int err = uv_tcp_init(server->loop, &inspector->client);