lukechilds
/
node
mirror of https://github.com/lukechilds/node.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

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 <info@bnoordhuis.nl>
v7.x
Eugene Ostroukhov 9 years ago
committed by Ali Ijaz Sheikh
parent
62c8e7b922
commit
9dfc8b9d8e
3 changed files with 57 additions and 44 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 18
      src/inspector_agent.cc
  2. 5
      src/inspector_socket.cc
  3. 78
      test/cctest/test_inspector_socket.cc

18
src/inspector_agent.cc
View File

@ -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<inspector_socket_t*>(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;

5
src/inspector_socket.cc
View File

@ -88,8 +88,6 @@ static void close_connection(inspector_socket_t* inspector) {
if (!uv_is_closing(socket)) {
uv_read_stop(reinterpret_cast<uv_stream_t*>(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);
}

78
test/cctest/test_inspector_socket.cc
View File

@ -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<char *>(data);
buf[0].base = const_cast<char*>(data);
buf[0].len = len;
uv_write(&req, reinterpret_cast<uv_stream_t *>(&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<char *>(malloc(len));
buf->base = static_cast<char*>(malloc(len));
buf->len = len;
}
@ -369,7 +369,7 @@ protected:
TEST_F(InspectorSocketTest, ReadsAndWritesInspectorMessage) {
ASSERT_TRUE(connected);
ASSERT_FALSE(inspector_ready);
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
do_write(const_cast<char*>(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<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
do_write(const_cast<char*>(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<char *>(HANDSHAKE_REQ), write1);
do_write(const_cast<char*>(HANDSHAKE_REQ), write1);
ASSERT_FALSE(inspector_ready);
do_write(const_cast<char *>(HANDSHAKE_REQ) + write1, write2);
do_write(const_cast<char*>(HANDSHAKE_REQ) + write1, write2);
ASSERT_FALSE(inspector_ready);
do_write(const_cast<char *>(HANDSHAKE_REQ) + write1 + write2, write3);
do_write(const_cast<char*>(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<char *>(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1);
do_write(const_cast<char*>(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1);
ASSERT_FALSE(inspector_ready);
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
SPIN_WHILE(last_event != kInspectorHandshakeFailed);
expect_handshake_failure();
EXPECT_EQ(uv_is_active(reinterpret_cast<uv_handle_t*>(&client_socket)), 0);
@ -493,10 +493,10 @@ TEST_F(InspectorSocketTest, ExtraTextBeforeRequest) {
TEST_F(InspectorSocketTest, ExtraLettersBeforeRequest) {
char UNCOOL_BRO[] = "Uncool!!";
do_write(const_cast<char *>(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1);
do_write(const_cast<char*>(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1);
ASSERT_FALSE(inspector_ready);
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
SPIN_WHILE(last_event != kInspectorHandshakeFailed);
expect_handshake_failure();
EXPECT_EQ(uv_is_active(reinterpret_cast<uv_handle_t*>(&client_socket)), 0);
@ -511,7 +511,7 @@ TEST_F(InspectorSocketTest, RequestWithoutKey) {
"Sec-WebSocket-Version: 13\r\n\r\n";
;
do_write(const_cast<char *>(BROKEN_REQUEST), sizeof(BROKEN_REQUEST) - 1);
do_write(const_cast<char*>(BROKEN_REQUEST), sizeof(BROKEN_REQUEST) - 1);
SPIN_WHILE(last_event != kInspectorHandshakeFailed);
expect_handshake_failure();
EXPECT_EQ(uv_is_active(reinterpret_cast<uv_handle_t*>(&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<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
do_write(const_cast<char*>(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<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
do_write(const_cast<char*>(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<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
inspector_closed = 0;
do_write(const_cast<char*>(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<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
inspector_closed = 0;
do_write(const_cast<char*>(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<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
do_write(const_cast<char*>(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<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
do_write(const_cast<char*>(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<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
do_write(const_cast<char*>(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<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
do_write(const_cast<char*>(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<char*>(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);
}

Write Preview
Loading…
Cancel
Save