From 8dd48c9251d240b30af53cdbc7f8924b86baaded Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Wed, 1 Jun 2016 10:53:30 -0700 Subject: [PATCH] inspector: fix inspector connection cleanup In some cases close callback was called twice, while in some cases the memory was still not released at all. PR-URL: https://github.com/nodejs/node/pull/7268 Reviewed-By: bnoordhuis - Ben Noordhuis --- src/inspector_agent.cc | 18 +++---- src/inspector_socket.cc | 5 -- test/cctest/test_inspector_socket.cc | 78 ++++++++++++++++++---------- 3 files changed, 57 insertions(+), 44 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index d9823327904996..b0451db75f7dac 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -47,7 +47,7 @@ bool AcceptsConnection(inspector_socket_t* socket, const char* path) { } void DisposeInspector(inspector_socket_t* socket, int status) { - free(socket); + delete socket; } void DisconnectAndDisposeIO(inspector_socket_t* socket) { @@ -404,14 +404,12 @@ void AgentImpl::ThreadCbIO(void* agent) { // static void AgentImpl::OnSocketConnectionIO(uv_stream_t* server, int status) { if (status == 0) { - inspector_socket_t* socket = - static_cast(malloc(sizeof(*socket))); - ASSERT_NE(nullptr, socket); + 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) { - free(socket); + delete socket; } } } @@ -430,6 +428,7 @@ bool AgentImpl::OnInspectorHandshakeIO(inspector_socket_t* socket, agent->OnInspectorConnectionIO(socket); return true; case kInspectorHandshakeFailed: + delete socket; return false; default: UNREACHABLE(); @@ -461,12 +460,7 @@ void AgentImpl::OnRemoteDataIO(uv_stream_t* stream, agent->parent_env_->isolate() ->RequestInterrupt(InterruptCallback, agent); uv_async_send(&agent->data_written_); - } else if (read < 0) { - if (agent->client_socket_ == socket) { - agent->client_socket_ = nullptr; - } - DisconnectAndDisposeIO(socket); - } else { + } else if (read <= 0) { // EOF if (agent->client_socket_ == socket) { agent->client_socket_ = nullptr; @@ -474,6 +468,7 @@ void AgentImpl::OnRemoteDataIO(uv_stream_t* stream, new SetConnectedTask(agent, false)); uv_async_send(&agent->data_written_); } + DisconnectAndDisposeIO(socket); } uv_cond_broadcast(&agent->pause_cond_); } @@ -537,6 +532,7 @@ void AgentImpl::WorkerRunIO() { void AgentImpl::OnInspectorConnectionIO(inspector_socket_t* socket) { if (client_socket_) { + DisconnectAndDisposeIO(socket); return; } client_socket_ = socket; diff --git a/src/inspector_socket.cc b/src/inspector_socket.cc index fa2b6734e9e186..b860ef3bb740f7 100644 --- a/src/inspector_socket.cc +++ b/src/inspector_socket.cc @@ -88,8 +88,6 @@ static void close_connection(inspector_socket_t* inspector) { if (!uv_is_closing(socket)) { uv_read_stop(reinterpret_cast(socket)); uv_close(socket, dispose_inspector); - } else if (inspector->ws_state->close_cb) { - inspector->ws_state->close_cb(inspector, 0); } } @@ -276,9 +274,6 @@ static void invoke_read_callback(inspector_socket_t* inspector, } static void shutdown_complete(inspector_socket_t* inspector) { - if (inspector->ws_state->close_cb) { - inspector->ws_state->close_cb(inspector, 0); - } close_connection(inspector); } diff --git a/test/cctest/test_inspector_socket.cc b/test/cctest/test_inspector_socket.cc index ebe4215af5eca4..eecb35f824f13a 100644 --- a/test/cctest/test_inspector_socket.cc +++ b/test/cctest/test_inspector_socket.cc @@ -84,7 +84,7 @@ static void do_write(const char* data, int len) { bool done = false; req.data = &done; uv_buf_t buf[1]; - buf[0].base = const_cast(data); + buf[0].base = const_cast(data); buf[0].len = len; uv_write(&req, reinterpret_cast(&client_socket), buf, 1, write_done); @@ -92,7 +92,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 = static_cast(malloc(len)); buf->len = len; } @@ -369,7 +369,7 @@ class InspectorSocketTest : public ::testing::Test { TEST_F(InspectorSocketTest, ReadsAndWritesInspectorMessage) { ASSERT_TRUE(connected); ASSERT_FALSE(inspector_ready); - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); SPIN_WHILE(!inspector_ready); expect_handshake(); @@ -397,7 +397,7 @@ TEST_F(InspectorSocketTest, ReadsAndWritesInspectorMessage) { TEST_F(InspectorSocketTest, BufferEdgeCases) { - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake(); const char MULTIPLE_REQUESTS[] = { @@ -466,11 +466,11 @@ TEST_F(InspectorSocketTest, AcceptsRequestInSeveralWrites) { const int write1 = 95; const int write2 = 5; const int write3 = sizeof(HANDSHAKE_REQ) - write1 - write2 - 1; - do_write(const_cast(HANDSHAKE_REQ), write1); + do_write(const_cast(HANDSHAKE_REQ), write1); ASSERT_FALSE(inspector_ready); - do_write(const_cast(HANDSHAKE_REQ) + write1, write2); + do_write(const_cast(HANDSHAKE_REQ) + write1, write2); ASSERT_FALSE(inspector_ready); - do_write(const_cast(HANDSHAKE_REQ) + write1 + write2, write3); + do_write(const_cast(HANDSHAKE_REQ) + write1 + write2, write3); SPIN_WHILE(!inspector_ready); expect_handshake(); inspector_read_stop(&inspector); @@ -481,10 +481,10 @@ TEST_F(InspectorSocketTest, AcceptsRequestInSeveralWrites) { TEST_F(InspectorSocketTest, ExtraTextBeforeRequest) { last_event = kInspectorHandshakeUpgraded; char UNCOOL_BRO[] = "Uncool, bro: Text before the first req\r\n"; - do_write(const_cast(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1); + do_write(const_cast(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1); ASSERT_FALSE(inspector_ready); - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); SPIN_WHILE(last_event != kInspectorHandshakeFailed); expect_handshake_failure(); EXPECT_EQ(uv_is_active(reinterpret_cast(&client_socket)), 0); @@ -493,10 +493,10 @@ TEST_F(InspectorSocketTest, ExtraTextBeforeRequest) { TEST_F(InspectorSocketTest, ExtraLettersBeforeRequest) { char UNCOOL_BRO[] = "Uncool!!"; - do_write(const_cast(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1); + do_write(const_cast(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1); ASSERT_FALSE(inspector_ready); - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); SPIN_WHILE(last_event != kInspectorHandshakeFailed); expect_handshake_failure(); EXPECT_EQ(uv_is_active(reinterpret_cast(&client_socket)), 0); @@ -511,7 +511,7 @@ TEST_F(InspectorSocketTest, RequestWithoutKey) { "Sec-WebSocket-Version: 13\r\n\r\n"; ; - do_write(const_cast(BROKEN_REQUEST), sizeof(BROKEN_REQUEST) - 1); + do_write(const_cast(BROKEN_REQUEST), sizeof(BROKEN_REQUEST) - 1); SPIN_WHILE(last_event != kInspectorHandshakeFailed); expect_handshake_failure(); EXPECT_EQ(uv_is_active(reinterpret_cast(&client_socket)), 0); @@ -521,7 +521,7 @@ TEST_F(InspectorSocketTest, RequestWithoutKey) { TEST_F(InspectorSocketTest, KillsConnectionOnProtocolViolation) { ASSERT_TRUE(connected); ASSERT_FALSE(inspector_ready); - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); SPIN_WHILE(!inspector_ready); ASSERT_TRUE(inspector_ready); expect_handshake(); @@ -534,7 +534,7 @@ TEST_F(InspectorSocketTest, KillsConnectionOnProtocolViolation) { TEST_F(InspectorSocketTest, CanStopReadingFromInspector) { ASSERT_TRUE(connected); ASSERT_FALSE(inspector_ready); - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake(); ASSERT_TRUE(inspector_ready); @@ -552,15 +552,15 @@ TEST_F(InspectorSocketTest, CanStopReadingFromInspector) { manual_inspector_socket_cleanup(); } -static bool inspector_closed; +static int inspector_closed = 0; void inspector_closed_cb(inspector_socket_t *inspector, int code) { - inspector_closed = true; + inspector_closed++; } TEST_F(InspectorSocketTest, CloseDoesNotNotifyReadCallback) { - inspector_closed = false; - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + inspector_closed = 0; + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake(); int error_code = 0; @@ -570,27 +570,29 @@ TEST_F(InspectorSocketTest, CloseDoesNotNotifyReadCallback) { inspector_close(&inspector, inspector_closed_cb); char CLOSE_FRAME[] = {'\x88', '\x00'}; expect_on_client(CLOSE_FRAME, sizeof(CLOSE_FRAME)); - ASSERT_FALSE(inspector_closed); + EXPECT_EQ(0, inspector_closed); const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D', '\x0E', '\x1E', '\xFA'}; do_write(CLIENT_CLOSE_FRAME, sizeof(CLIENT_CLOSE_FRAME)); EXPECT_NE(UV_EOF, error_code); - SPIN_WHILE(!inspector_closed); + SPIN_WHILE(inspector_closed == 0); + EXPECT_EQ(1, inspector_closed); inspector.data = nullptr; } TEST_F(InspectorSocketTest, CloseWorksWithoutReadEnabled) { - inspector_closed = false; - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + inspector_closed = 0; + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake(); inspector_close(&inspector, inspector_closed_cb); char CLOSE_FRAME[] = {'\x88', '\x00'}; expect_on_client(CLOSE_FRAME, sizeof(CLOSE_FRAME)); - ASSERT_FALSE(inspector_closed); + EXPECT_EQ(0, inspector_closed); const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D', '\x0E', '\x1E', '\xFA'}; do_write(CLIENT_CLOSE_FRAME, sizeof(CLIENT_CLOSE_FRAME)); - SPIN_WHILE(!inspector_closed); + SPIN_WHILE(inspector_closed == 0); + EXPECT_EQ(1, inspector_closed); } // Make sure buffering works @@ -690,7 +692,7 @@ HandshakeCanBeCanceled_handshake(enum inspector_handshake_event state, TEST_F(InspectorSocketTest, HandshakeCanBeCanceled) { handshake_delegate = HandshakeCanBeCanceled_handshake; - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake_failure(); EXPECT_EQ(2, handshake_events); @@ -727,7 +729,7 @@ TEST_F(InspectorSocketTest, GetThenHandshake) { expect_on_client(TEST_SUCCESS, sizeof(TEST_SUCCESS) - 1); - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake(); EXPECT_EQ(3, handshake_events); manual_inspector_socket_cleanup(); @@ -766,7 +768,7 @@ static void CleanupSocketAfterEOF_read_cb(uv_stream_t* stream, ssize_t nread, } TEST_F(InspectorSocketTest, CleanupSocketAfterEOF) { - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake(); inspector_read_start(&inspector, buffer_alloc_cb, @@ -810,7 +812,7 @@ static void mask_message(const char* message, TEST_F(InspectorSocketTest, Send1Mb) { ASSERT_TRUE(connected); ASSERT_FALSE(inspector_ready); - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); SPIN_WHILE(!inspector_ready); expect_handshake(); @@ -862,3 +864,23 @@ TEST_F(InspectorSocketTest, Send1Mb) { free(expected); free(message); } + +static ssize_t err; + +void ErrorCleansUpTheSocket_cb(uv_stream_t* stream, ssize_t read, + const uv_buf_t* buf) { + err = read; +} + +TEST_F(InspectorSocketTest, ErrorCleansUpTheSocket) { + inspector_closed = 0; + do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); + expect_handshake(); + const char NOT_A_GOOD_FRAME[] = {'H', 'e', 'l', 'l', 'o'}; + err = 42; + inspector_read_start(&inspector, buffer_alloc_cb, + ErrorCleansUpTheSocket_cb); + do_write(NOT_A_GOOD_FRAME, sizeof(NOT_A_GOOD_FRAME)); + SPIN_WHILE(err > 0); + EXPECT_EQ(UV_EPROTO, err); +}