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, 53 insertions, 68 deletions

diff --git a/test/cctest/test_inspector_socket.cc b/test/cctest/test_inspector_socket.cc
index 335ae002a1..771f4022ac 100644
--- a/test/cctest/test_inspector_socket.cc
+++ b/test/cctest/test_inspector_socket.cc
@@ -26,9 +26,10 @@ static enum inspector_handshake_event last_event = kInspectorHandshakeHttpGet;
 static uv_loop_t loop;
 static uv_tcp_t server, client_socket;
 static inspector_socket_t inspector;
-static char last_path[100];
+static std::string last_path;
 static void (*handshake_delegate)(enum inspector_handshake_event state,
-                                  const char* path, bool* should_continue);
+                                  const std::string& path,
+                                  bool* should_continue);
 
 struct read_expects {
   const char* expected;
@@ -50,19 +51,19 @@ static void set_timeout_flag(uv_timer_t* timer) {
 }
 
 static void stop_if_stop_path(enum inspector_handshake_event state,
-                              const char* path, bool* cont) {
-  *cont = path == nullptr || strcmp(path, "/close") != 0;
+                              const std::string& path, bool* cont) {
+  *cont = path.empty() || path != "/close";
 }
 
 static bool connected_cb(inspector_socket_t* socket,
                          enum inspector_handshake_event state,
-                         const char* path) {
+                         const std::string& path) {
   inspector_ready = state == kInspectorHandshakeUpgraded;
   last_event = state;
-  if (!path) {
-    strcpy(last_path, "@@@ Nothing Recieved @@@");
+  if (path.empty()) {
+    last_path = "@@@ Nothing received @@@";
   } else {
-    strncpy(last_path, path, sizeof(last_path) - 1);
+    last_path = path;
   }
   handshake_events++;
   bool should_continue = true;
@@ -92,7 +93,7 @@ static void do_write(const char* data, int len) {
 }
 
 static void buffer_alloc_cb(uv_handle_t* stream, size_t len, uv_buf_t* buf) {
-  buf->base = static_cast<char*>(malloc(len));
+  buf->base = new char[len];
   buf->len = len;
 }
 
@@ -113,7 +114,7 @@ static void check_data_cb(read_expects* expectation, ssize_t nread,
     }
   }
   GTEST_ASSERT_EQ(i, nread);
-  free(buf->base);
+  delete[] buf->base;
   if (expectation->pos == expectation->expected_len) {
     expectation->read_expected = true;
     *retval = true;
@@ -169,7 +170,7 @@ static void expect_on_client(const char* data, size_t len) {
 }
 
 struct expectations {
-  char* actual_data;
+  std::string actual_data;
   size_t actual_offset;
   size_t actual_end;
   int err_code;
@@ -181,9 +182,8 @@ static void grow_expects_buffer(uv_handle_t* stream, size_t size, uv_buf_t* b) {
   size_t end = expects->actual_end;
   // Grow the buffer in chunks of 64k.
   size_t new_length = (end + size + 65535) & ~((size_t) 0xFFFF);
-  expects->actual_data =
-      static_cast<char*>(realloc(expects->actual_data, new_length));
-  *b = uv_buf_init(expects->actual_data + end, new_length - end);
+  expects->actual_data.resize(new_length);
+  *b = uv_buf_init(&expects->actual_data[end], new_length - end);
 }
 
 // static void dump_hex(const char* buf, size_t len) {
@@ -224,8 +224,7 @@ static void setup_inspector_expecting() {
   if (inspector.data) {
     return;
   }
-  expectations* expects = static_cast<expectations*>(malloc(sizeof(*expects)));
-  memset(expects, 0, sizeof(*expects));
+  expectations* expects = new expectations();
   inspector.data = expects;
   inspector_read_start(&inspector, grow_expects_buffer, save_read_data);
 }
@@ -246,8 +245,8 @@ static void expect_on_server(const char* data, size_t len) {
   }
   expects->actual_end -= expects->actual_offset;
   if (!expects->actual_end) {
-    memmove(expects->actual_data,
-            expects->actual_data + expects->actual_offset,
+    memmove(&expects->actual_data[0],
+            &expects->actual_data[expects->actual_offset],
             expects->actual_end);
   }
   expects->actual_offset = 0;
@@ -301,10 +300,11 @@ static void manual_inspector_socket_cleanup() {
   EXPECT_EQ(0, uv_is_active(
                    reinterpret_cast<uv_handle_t*>(&inspector.client)));
   really_close(reinterpret_cast<uv_handle_t*>(&inspector.client));
-  free(inspector.ws_state);
-  free(inspector.http_parsing_state);
-  free(inspector.buffer);
-  inspector.buffer = nullptr;
+  delete inspector.ws_state;
+  inspector.ws_state = nullptr;
+  delete inspector.http_parsing_state;
+  inspector.http_parsing_state = nullptr;
+  inspector.buffer.clear();
 }
 
 static void on_connection(uv_connect_t* connect, int status) {
@@ -321,9 +321,9 @@ protected:
     inspector_ready = false;
     last_event = kInspectorHandshakeHttpGet;
     uv_loop_init(&loop);
-    memset(&inspector, 0, sizeof(inspector));
-    memset(&server, 0, sizeof(server));
-    memset(&client_socket, 0, sizeof(client_socket));
+    inspector = inspector_socket_t();
+    server = uv_tcp_t();
+    client_socket = uv_tcp_t();
     server.data = &inspector;
     sockaddr_in addr;
     uv_timer_init(&loop, &timeout_timer);
@@ -346,13 +346,11 @@ protected:
   virtual void TearDown() {
     really_close(reinterpret_cast<uv_handle_t*>(&client_socket));
     really_close(reinterpret_cast<uv_handle_t*>(&timeout_timer));
-    EXPECT_EQ(nullptr, inspector.buffer);
+    EXPECT_TRUE(inspector.buffer.empty());
     expectations* expects = static_cast<expectations*>(inspector.data);
     if (expects != nullptr) {
       GTEST_ASSERT_EQ(expects->actual_end, expects->actual_offset);
-      free(expects->actual_data);
-      expects->actual_data = nullptr;
-      free(expects);
+      delete expects;
       inspector.data = nullptr;
     }
     const int err = uv_loop_close(&loop);
@@ -608,10 +606,10 @@ static void send_in_chunks(const char* data, size_t len) {
 static const char TEST_SUCCESS[] = "Test Success\n\n";
 
 static void ReportsHttpGet_handshake(enum inspector_handshake_event state,
-                                     const char* path, bool* cont) {
+                                     const std::string& path, bool* cont) {
   *cont = true;
   enum inspector_handshake_event expected_state = kInspectorHandshakeHttpGet;
-  const char* expected_path;
+  std::string expected_path;
   switch (handshake_events) {
   case 1:
     expected_path = "/some/path";
@@ -625,18 +623,16 @@ 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;
     break;
   default:
-    expected_path = nullptr;
     ASSERT_TRUE(false);
   }
   EXPECT_EQ(expected_state, state);
-  EXPECT_STREQ(expected_path, path);
+  EXPECT_EQ(expected_path, path);
 }
 
 TEST_F(InspectorSocketTest, ReportsHttpGet) {
@@ -672,15 +668,15 @@ TEST_F(InspectorSocketTest, ReportsHttpGet) {
 
 static void
 HandshakeCanBeCanceled_handshake(enum inspector_handshake_event state,
-                                 const char* path, bool* cont) {
+                                 const std::string& path, bool* cont) {
   switch (handshake_events - 1) {
   case 0:
     EXPECT_EQ(kInspectorHandshakeUpgrading, state);
-    EXPECT_STREQ("/ws/path", path);
+    EXPECT_EQ("/ws/path", path);
     break;
   case 1:
     EXPECT_EQ(kInspectorHandshakeFailed, state);
-    EXPECT_STREQ(nullptr, path);
+    EXPECT_TRUE(path.empty());
     break;
   default:
     EXPECT_TRUE(false);
@@ -699,9 +695,9 @@ TEST_F(InspectorSocketTest, HandshakeCanBeCanceled) {
 }
 
 static void GetThenHandshake_handshake(enum inspector_handshake_event state,
-                                       const char* path, bool* cont) {
+                                       const std::string& path, bool* cont) {
   *cont = true;
-  const char* expected_path = "/ws/path";
+  std::string expected_path = "/ws/path";
   switch (handshake_events - 1) {
   case 0:
     EXPECT_EQ(kInspectorHandshakeHttpGet, state);
@@ -718,7 +714,7 @@ static void GetThenHandshake_handshake(enum inspector_handshake_event state,
     EXPECT_TRUE(false);
     break;
   }
-  EXPECT_STREQ(expected_path, path);
+  EXPECT_EQ(expected_path, path);
 }
 
 TEST_F(InspectorSocketTest, GetThenHandshake) {
@@ -793,19 +789,17 @@ TEST_F(InspectorSocketTest, EOFBeforeHandshake) {
   SPIN_WHILE(last_event != kInspectorHandshakeFailed);
 }
 
-static void fill_message(char* buffer, size_t len) {
-  buffer[len - 1] = '\0';
-  for (size_t i = 0; i < len - 1; i++) {
-    buffer[i] = 'a' + (i % ('z' - 'a'));
+static void fill_message(std::string* buffer) {
+  for (size_t i = 0; i < buffer->size(); i += 1) {
+    (*buffer)[i] = 'a' + (i % ('z' - 'a'));
   }
 }
 
-static void mask_message(const char* message,
+static void mask_message(const std::string& message,
                          char* buffer, const char mask[]) {
   const size_t mask_len = 4;
-  int i = 0;
-  while (*message != '\0') {
-    *buffer++ = *message++ ^ mask[i++ % mask_len];
+  for (size_t i = 0; i < message.size(); i += 1) {
+    buffer[i] = message[i] ^ mask[i % mask_len];
   }
 }
 
@@ -816,25 +810,20 @@ TEST_F(InspectorSocketTest, Send1Mb) {
   SPIN_WHILE(!inspector_ready);
   expect_handshake();
 
-  const size_t message_len = 1000000;
-
   // 2. Brief exchange
-  char* message = static_cast<char*>(malloc(message_len + 1));
-  fill_message(message, message_len + 1);
+  std::string message(1000000, '\0');
+  fill_message(&message);
 
   // 1000000 is 0xF4240 hex
   const char EXPECTED_FRAME_HEADER[] = {
     '\x81', '\x7f', '\x00', '\x00', '\x00', '\x00', '\x00', '\x0F',
     '\x42', '\x40'
   };
-  char* expected =
-      static_cast<char*>(malloc(sizeof(EXPECTED_FRAME_HEADER) + message_len));
-
-  memcpy(expected, EXPECTED_FRAME_HEADER, sizeof(EXPECTED_FRAME_HEADER));
-  memcpy(expected + sizeof(EXPECTED_FRAME_HEADER), message, message_len);
+  std::string expected(EXPECTED_FRAME_HEADER, sizeof(EXPECTED_FRAME_HEADER));
+  expected.append(message);
 
-  inspector_write(&inspector, message, message_len);
-  expect_on_client(expected, sizeof(EXPECTED_FRAME_HEADER) + message_len);
+  inspector_write(&inspector, &message[0], message.size());
+  expect_on_client(&expected[0], expected.size());
 
   char MASK[4] = {'W', 'h', 'O', 'a'};
 
@@ -843,14 +832,13 @@ TEST_F(InspectorSocketTest, Send1Mb) {
     '\x42', '\x40', MASK[0], MASK[1], MASK[2], MASK[3]
   };
 
-  const size_t outgoing_len = sizeof(FRAME_TO_SERVER_HEADER) + message_len;
-  char* outgoing = static_cast<char*>(malloc(outgoing_len));
-  memcpy(outgoing, FRAME_TO_SERVER_HEADER, sizeof(FRAME_TO_SERVER_HEADER));
-  mask_message(message, outgoing + sizeof(FRAME_TO_SERVER_HEADER), MASK);
+  std::string outgoing(FRAME_TO_SERVER_HEADER, sizeof(FRAME_TO_SERVER_HEADER));
+  outgoing.resize(outgoing.size() + message.size());
+  mask_message(message, &outgoing[sizeof(FRAME_TO_SERVER_HEADER)], MASK);
 
   setup_inspector_expecting(); // Buffer on the client side.
-  do_write(outgoing, outgoing_len);
-  expect_on_server(message, message_len);
+  do_write(&outgoing[0], outgoing.size());
+  expect_on_server(&message[0], message.size());
 
   // 3. Close
   const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D',
@@ -860,9 +848,6 @@ TEST_F(InspectorSocketTest, Send1Mb) {
   expect_on_client(SERVER_CLOSE_FRAME, sizeof(SERVER_CLOSE_FRAME));
   GTEST_ASSERT_EQ(0, uv_is_active(
                          reinterpret_cast<uv_handle_t*>(&client_socket)));
-  free(outgoing);
-  free(expected);
-  free(message);
 }
 
 static ssize_t err;