From c8c1f96abe47b1699e1c9cd824eed7d4413f998a Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 28 Jul 2016 12:14:11 +0200 Subject: [PATCH] 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 Reviewed-By: James M Snell --- src/inspector_agent.cc | 31 ++-- src/inspector_socket.cc | 209 +++++++++------------------ src/inspector_socket.h | 28 +++- test/cctest/test_inspector_socket.cc | 121 +++++++--------- 4 files changed, 161 insertions(+), 228 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index fee459146c3e31..0446f462e92f7e 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -48,8 +48,8 @@ void PrintDebuggerReadyMessage(int port) { port, DEVTOOLS_HASH, port); } -bool AcceptsConnection(inspector_socket_t* socket, const char* path) { - return strncmp(DEVTOOLS_PATH, path, sizeof(DEVTOOLS_PATH)) == 0; +bool AcceptsConnection(inspector_socket_t* socket, const std::string& path) { + return 0 == path.compare(0, sizeof(DEVTOOLS_PATH) - 1, DEVTOOLS_PATH); } void DisposeInspector(inspector_socket_t* socket, int status) { @@ -63,10 +63,7 @@ void DisconnectAndDisposeIO(inspector_socket_t* socket) { } void OnBufferAlloc(uv_handle_t* handle, size_t len, uv_buf_t* buf) { - if (len > 0) { - buf->base = static_cast(malloc(len)); - CHECK_NE(buf->base, nullptr); - } + buf->base = new char[len]; buf->len = len; } @@ -133,18 +130,19 @@ void SendTargentsListResponse(inspector_socket_t* socket, int port) { SendHttpResponse(socket, buffer, len); } -bool RespondToGet(inspector_socket_t* socket, const char* path, int port) { +bool RespondToGet(inspector_socket_t* socket, const std::string& path, + int port) { const char PATH[] = "/json"; const char PATH_LIST[] = "/json/list"; const char PATH_VERSION[] = "/json/version"; const char PATH_ACTIVATE[] = "/json/activate/"; - if (!strncmp(PATH_VERSION, path, sizeof(PATH_VERSION))) { + if (0 == path.compare(0, sizeof(PATH_VERSION) - 1, PATH_VERSION)) { SendVersionResponse(socket); - } else if (!strncmp(PATH_LIST, path, sizeof(PATH_LIST)) || - !strncmp(PATH, path, sizeof(PATH))) { + } else if (0 == path.compare(0, sizeof(PATH_LIST) - 1, PATH_LIST) || + 0 == path.compare(0, sizeof(PATH) - 1, PATH)) { SendTargentsListResponse(socket, port); - } else if (!strncmp(path, PATH_ACTIVATE, sizeof(PATH_ACTIVATE) - 1) && - atoi(path + (sizeof(PATH_ACTIVATE) - 1)) == getpid()) { + } else if (0 == path.compare(0, sizeof(PATH_ACTIVATE) - 1, PATH_ACTIVATE) && + atoi(path.substr(sizeof(PATH_ACTIVATE) - 1).c_str()) == getpid()) { const char TARGET_ACTIVATED[] = "Target activated"; SendHttpResponse(socket, TARGET_ACTIVATED, sizeof(TARGET_ACTIVATED) - 1); } else { @@ -181,7 +179,7 @@ class AgentImpl { static void OnSocketConnectionIO(uv_stream_t* server, int status); static bool OnInspectorHandshakeIO(inspector_socket_t* socket, enum inspector_handshake_event state, - const char* path); + const std::string& path); static void WriteCbIO(uv_async_t* async); void WorkerRunIO(); @@ -388,7 +386,6 @@ void AgentImpl::ThreadCbIO(void* agent) { void AgentImpl::OnSocketConnectionIO(uv_stream_t* server, int status) { if (status == 0) { inspector_socket_t* socket = new inspector_socket_t(); - memset(socket, 0, sizeof(*socket)); socket->data = server->data; if (inspector_accept(server, socket, AgentImpl::OnInspectorHandshakeIO) != 0) { @@ -399,8 +396,8 @@ void AgentImpl::OnSocketConnectionIO(uv_stream_t* server, int status) { // static bool AgentImpl::OnInspectorHandshakeIO(inspector_socket_t* socket, - enum inspector_handshake_event state, - const char* path) { + enum inspector_handshake_event state, + const std::string& path) { AgentImpl* agent = static_cast(socket->data); switch (state) { case kInspectorHandshakeHttpGet: @@ -443,7 +440,7 @@ void AgentImpl::OnRemoteDataIO(inspector_socket_t* socket, DisconnectAndDisposeIO(socket); } if (buf) { - free(buf->base); + delete[] buf->base; } pause_cond_.Broadcast(scoped_lock); } diff --git a/src/inspector_socket.cc b/src/inspector_socket.cc index d3a8d8efffc57c..c360ef0db9545e 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(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 storage; + uv_write_t req; + uv_buf_t buf; +}; + // Cleanup static void write_request_cleanup(uv_write_t* req, int status) { - free((reinterpret_cast(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(malloc(sizeof(uv_buf_t))); - uv_write_t* req = reinterpret_cast(malloc(sizeof(uv_write_t))); - CHECK_NE(buf, nullptr); - CHECK_NE(req, nullptr); - memset(req, 0, sizeof(*req)); - buf->base = reinterpret_cast(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(&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(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 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(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(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(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(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(&input[0]), input.size(), + reinterpret_cast(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(parser->data))->http_parsing_state; + static const char SEC_WEBSOCKET_KEY_HEADER[] = "Sec-WebSocket-Key"; + auto inspector = static_cast(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(parser->data))->http_parsing_state; + auto inspector = static_cast(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(parser->data))->http_parsing_state; - append(&state->path, at, length); + auto inspector = static_cast(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(&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(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(&inspector->client); CHECK_NE(client, nullptr); int err = uv_tcp_init(server->loop, &inspector->client); diff --git a/src/inspector_socket.h b/src/inspector_socket.h index 3e52762e715de5..5d3477b98c1bb5 100644 --- a/src/inspector_socket.h +++ b/src/inspector_socket.h @@ -4,6 +4,9 @@ #include "http_parser.h" #include "uv.h" +#include +#include + enum inspector_handshake_event { kInspectorHandshakeUpgrading, kInspectorHandshakeUpgraded, @@ -19,17 +22,32 @@ typedef void (*inspector_cb)(struct inspector_socket_s*, int); // the connection. inspector_write can be used from the callback. typedef bool (*handshake_cb)(struct inspector_socket_s*, enum inspector_handshake_event state, - const char* path); + const std::string& path); + +struct http_parsing_state_s { + http_parser parser; + http_parser_settings parser_settings; + handshake_cb callback; + bool done; + bool parsing_value; + std::string ws_key; + std::string path; + std::string current_header; +}; -struct http_parsing_state_s; -struct ws_state_s; +struct ws_state_s { + uv_alloc_cb alloc_cb; + uv_read_cb read_cb; + inspector_cb close_cb; + bool close_sent; + bool received_close; +}; struct inspector_socket_s { void* data; struct http_parsing_state_s* http_parsing_state; struct ws_state_s* ws_state; - char* buffer; - size_t buffer_size; + std::vector buffer; size_t data_len; size_t last_read_end; uv_tcp_t client; diff --git a/test/cctest/test_inspector_socket.cc b/test/cctest/test_inspector_socket.cc index 335ae002a11659..771f4022acdf72 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(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(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(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(&inspector.client))); really_close(reinterpret_cast(&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 @@ class InspectorSocketTest : public ::testing::Test { 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 @@ class InspectorSocketTest : public ::testing::Test { virtual void TearDown() { really_close(reinterpret_cast(&client_socket)); really_close(reinterpret_cast(&timeout_timer)); - EXPECT_EQ(nullptr, inspector.buffer); + EXPECT_TRUE(inspector.buffer.empty()); expectations* expects = static_cast(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(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(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(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(&client_socket))); - free(outgoing); - free(expected); - free(message); } static ssize_t err;