Skip to content

Commit 8dd48c9

Browse files
Eugene OstroukhovFishrock123
Eugene Ostroukhov
authored and
Fishrock123
committed
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: #7268 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
1 parent 09ecd1f commit 8dd48c9

File tree

3 files changed

+57
-44
lines changed

3 files changed

+57
-44
lines changed

‎src/inspector_agent.cc‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ bool AcceptsConnection(inspector_socket_t* socket, const char* path) {
4747
}
4848

4949
void DisposeInspector(inspector_socket_t* socket, int status) {
50-
free(socket);
50+
delete socket;
5151
}
5252

5353
void DisconnectAndDisposeIO(inspector_socket_t* socket) {
@@ -404,14 +404,12 @@ void AgentImpl::ThreadCbIO(void* agent) {
404404
// static
405405
void AgentImpl::OnSocketConnectionIO(uv_stream_t* server, int status) {
406406
if (status == 0) {
407-
inspector_socket_t* socket =
408-
static_cast<inspector_socket_t*>(malloc(sizeof(*socket)));
409-
ASSERT_NE(nullptr, socket);
407+
inspector_socket_t* socket = new inspector_socket_t();
410408
memset(socket, 0, sizeof(*socket));
411409
socket->data = server->data;
412410
if (inspector_accept(server, socket,
413411
AgentImpl::OnInspectorHandshakeIO) != 0) {
414-
free(socket);
412+
delete socket;
415413
}
416414
}
417415
}
@@ -430,6 +428,7 @@ bool AgentImpl::OnInspectorHandshakeIO(inspector_socket_t* socket,
430428
agent->OnInspectorConnectionIO(socket);
431429
return true;
432430
case kInspectorHandshakeFailed:
431+
delete socket;
433432
return false;
434433
default:
435434
UNREACHABLE();
@@ -461,19 +460,15 @@ void AgentImpl::OnRemoteDataIO(uv_stream_t* stream,
461460
agent->parent_env_->isolate()
462461
->RequestInterrupt(InterruptCallback, agent);
463462
uv_async_send(&agent->data_written_);
464-
} else if (read < 0) {
465-
if (agent->client_socket_ == socket) {
466-
agent->client_socket_ = nullptr;
467-
}
468-
DisconnectAndDisposeIO(socket);
469-
} else {
463+
} else if (read <= 0) {
470464
// EOF
471465
if (agent->client_socket_ == socket) {
472466
agent->client_socket_ = nullptr;
473467
agent->platform_->CallOnForegroundThread(agent->parent_env_->isolate(),
474468
new SetConnectedTask(agent, false));
475469
uv_async_send(&agent->data_written_);
476470
}
471+
DisconnectAndDisposeIO(socket);
477472
}
478473
uv_cond_broadcast(&agent->pause_cond_);
479474
}
@@ -537,6 +532,7 @@ void AgentImpl::WorkerRunIO() {
537532

538533
void AgentImpl::OnInspectorConnectionIO(inspector_socket_t* socket) {
539534
if (client_socket_) {
535+
DisconnectAndDisposeIO(socket);
540536
return;
541537
}
542538
client_socket_ = socket;

‎src/inspector_socket.cc‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,6 @@ static void close_connection(inspector_socket_t* inspector) {
8888
if (!uv_is_closing(socket)) {
8989
uv_read_stop(reinterpret_cast<uv_stream_t*>(socket));
9090
uv_close(socket, dispose_inspector);
91-
} else if (inspector->ws_state->close_cb) {
92-
inspector->ws_state->close_cb(inspector, 0);
9391
}
9492
}
9593

@@ -276,9 +274,6 @@ static void invoke_read_callback(inspector_socket_t* inspector,
276274
}
277275

278276
static void shutdown_complete(inspector_socket_t* inspector) {
279-
if (inspector->ws_state->close_cb) {
280-
inspector->ws_state->close_cb(inspector, 0);
281-
}
282277
close_connection(inspector);
283278
}
284279

‎test/cctest/test_inspector_socket.cc‎

Lines changed: 50 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -84,15 +84,15 @@ static void do_write(const char* data, int len) {
8484
bool done = false;
8585
req.data = &done;
8686
uv_buf_t buf[1];
87-
buf[0].base = const_cast<char *>(data);
87+
buf[0].base = const_cast<char*>(data);
8888
buf[0].len = len;
8989
uv_write(&req, reinterpret_cast<uv_stream_t *>(&client_socket), buf, 1,
9090
write_done);
9191
SPIN_WHILE(req.data);
9292
}
9393

9494
static void buffer_alloc_cb(uv_handle_t* stream, size_t len, uv_buf_t* buf) {
95-
buf->base = static_cast<char *>(malloc(len));
95+
buf->base = static_cast<char*>(malloc(len));
9696
buf->len = len;
9797
}
9898

@@ -369,7 +369,7 @@ class InspectorSocketTest : public ::testing::Test {
369369
TEST_F(InspectorSocketTest, ReadsAndWritesInspectorMessage) {
370370
ASSERT_TRUE(connected);
371371
ASSERT_FALSE(inspector_ready);
372-
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
372+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
373373
SPIN_WHILE(!inspector_ready);
374374
expect_handshake();
375375

@@ -397,7 +397,7 @@ TEST_F(InspectorSocketTest, ReadsAndWritesInspectorMessage) {
397397

398398
TEST_F(InspectorSocketTest, BufferEdgeCases) {
399399

400-
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
400+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
401401
expect_handshake();
402402

403403
const char MULTIPLE_REQUESTS[] = {
@@ -466,11 +466,11 @@ TEST_F(InspectorSocketTest, AcceptsRequestInSeveralWrites) {
466466
const int write1 = 95;
467467
const int write2 = 5;
468468
const int write3 = sizeof(HANDSHAKE_REQ) - write1 - write2 - 1;
469-
do_write(const_cast<char *>(HANDSHAKE_REQ), write1);
469+
do_write(const_cast<char*>(HANDSHAKE_REQ), write1);
470470
ASSERT_FALSE(inspector_ready);
471-
do_write(const_cast<char *>(HANDSHAKE_REQ) + write1, write2);
471+
do_write(const_cast<char*>(HANDSHAKE_REQ) + write1, write2);
472472
ASSERT_FALSE(inspector_ready);
473-
do_write(const_cast<char *>(HANDSHAKE_REQ) + write1 + write2, write3);
473+
do_write(const_cast<char*>(HANDSHAKE_REQ) + write1 + write2, write3);
474474
SPIN_WHILE(!inspector_ready);
475475
expect_handshake();
476476
inspector_read_stop(&inspector);
@@ -481,10 +481,10 @@ TEST_F(InspectorSocketTest, AcceptsRequestInSeveralWrites) {
481481
TEST_F(InspectorSocketTest, ExtraTextBeforeRequest) {
482482
last_event = kInspectorHandshakeUpgraded;
483483
char UNCOOL_BRO[] = "Uncool, bro: Text before the first req\r\n";
484-
do_write(const_cast<char *>(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1);
484+
do_write(const_cast<char*>(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1);
485485

486486
ASSERT_FALSE(inspector_ready);
487-
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
487+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
488488
SPIN_WHILE(last_event != kInspectorHandshakeFailed);
489489
expect_handshake_failure();
490490
EXPECT_EQ(uv_is_active(reinterpret_cast<uv_handle_t*>(&client_socket)), 0);
@@ -493,10 +493,10 @@ TEST_F(InspectorSocketTest, ExtraTextBeforeRequest) {
493493

494494
TEST_F(InspectorSocketTest, ExtraLettersBeforeRequest) {
495495
char UNCOOL_BRO[] = "Uncool!!";
496-
do_write(const_cast<char *>(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1);
496+
do_write(const_cast<char*>(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1);
497497

498498
ASSERT_FALSE(inspector_ready);
499-
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
499+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
500500
SPIN_WHILE(last_event != kInspectorHandshakeFailed);
501501
expect_handshake_failure();
502502
EXPECT_EQ(uv_is_active(reinterpret_cast<uv_handle_t*>(&client_socket)), 0);
@@ -511,7 +511,7 @@ TEST_F(InspectorSocketTest, RequestWithoutKey) {
511511
"Sec-WebSocket-Version: 13\r\n\r\n";
512512
;
513513

514-
do_write(const_cast<char *>(BROKEN_REQUEST), sizeof(BROKEN_REQUEST) - 1);
514+
do_write(const_cast<char*>(BROKEN_REQUEST), sizeof(BROKEN_REQUEST) - 1);
515515
SPIN_WHILE(last_event != kInspectorHandshakeFailed);
516516
expect_handshake_failure();
517517
EXPECT_EQ(uv_is_active(reinterpret_cast<uv_handle_t*>(&client_socket)), 0);
@@ -521,7 +521,7 @@ TEST_F(InspectorSocketTest, RequestWithoutKey) {
521521
TEST_F(InspectorSocketTest, KillsConnectionOnProtocolViolation) {
522522
ASSERT_TRUE(connected);
523523
ASSERT_FALSE(inspector_ready);
524-
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
524+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
525525
SPIN_WHILE(!inspector_ready);
526526
ASSERT_TRUE(inspector_ready);
527527
expect_handshake();
@@ -534,7 +534,7 @@ TEST_F(InspectorSocketTest, KillsConnectionOnProtocolViolation) {
534534
TEST_F(InspectorSocketTest, CanStopReadingFromInspector) {
535535
ASSERT_TRUE(connected);
536536
ASSERT_FALSE(inspector_ready);
537-
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
537+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
538538
expect_handshake();
539539
ASSERT_TRUE(inspector_ready);
540540

@@ -552,15 +552,15 @@ TEST_F(InspectorSocketTest, CanStopReadingFromInspector) {
552552
manual_inspector_socket_cleanup();
553553
}
554554

555-
static bool inspector_closed;
555+
static int inspector_closed = 0;
556556

557557
void inspector_closed_cb(inspector_socket_t *inspector, int code) {
558-
inspector_closed = true;
558+
inspector_closed++;
559559
}
560560

561561
TEST_F(InspectorSocketTest, CloseDoesNotNotifyReadCallback) {
562-
inspector_closed = false;
563-
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
562+
inspector_closed = 0;
563+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
564564
expect_handshake();
565565

566566
int error_code = 0;
@@ -570,27 +570,29 @@ TEST_F(InspectorSocketTest, CloseDoesNotNotifyReadCallback) {
570570
inspector_close(&inspector, inspector_closed_cb);
571571
char CLOSE_FRAME[] = {'\x88', '\x00'};
572572
expect_on_client(CLOSE_FRAME, sizeof(CLOSE_FRAME));
573-
ASSERT_FALSE(inspector_closed);
573+
EXPECT_EQ(0, inspector_closed);
574574
const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D',
575575
'\x0E', '\x1E', '\xFA'};
576576
do_write(CLIENT_CLOSE_FRAME, sizeof(CLIENT_CLOSE_FRAME));
577577
EXPECT_NE(UV_EOF, error_code);
578-
SPIN_WHILE(!inspector_closed);
578+
SPIN_WHILE(inspector_closed == 0);
579+
EXPECT_EQ(1, inspector_closed);
579580
inspector.data = nullptr;
580581
}
581582

582583
TEST_F(InspectorSocketTest, CloseWorksWithoutReadEnabled) {
583-
inspector_closed = false;
584-
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
584+
inspector_closed = 0;
585+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
585586
expect_handshake();
586587
inspector_close(&inspector, inspector_closed_cb);
587588
char CLOSE_FRAME[] = {'\x88', '\x00'};
588589
expect_on_client(CLOSE_FRAME, sizeof(CLOSE_FRAME));
589-
ASSERT_FALSE(inspector_closed);
590+
EXPECT_EQ(0, inspector_closed);
590591
const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D',
591592
'\x0E', '\x1E', '\xFA'};
592593
do_write(CLIENT_CLOSE_FRAME, sizeof(CLIENT_CLOSE_FRAME));
593-
SPIN_WHILE(!inspector_closed);
594+
SPIN_WHILE(inspector_closed == 0);
595+
EXPECT_EQ(1, inspector_closed);
594596
}
595597

596598
// Make sure buffering works
@@ -690,7 +692,7 @@ HandshakeCanBeCanceled_handshake(enum inspector_handshake_event state,
690692
TEST_F(InspectorSocketTest, HandshakeCanBeCanceled) {
691693
handshake_delegate = HandshakeCanBeCanceled_handshake;
692694

693-
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
695+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
694696

695697
expect_handshake_failure();
696698
EXPECT_EQ(2, handshake_events);
@@ -727,7 +729,7 @@ TEST_F(InspectorSocketTest, GetThenHandshake) {
727729

728730
expect_on_client(TEST_SUCCESS, sizeof(TEST_SUCCESS) - 1);
729731

730-
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
732+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
731733
expect_handshake();
732734
EXPECT_EQ(3, handshake_events);
733735
manual_inspector_socket_cleanup();
@@ -766,7 +768,7 @@ static void CleanupSocketAfterEOF_read_cb(uv_stream_t* stream, ssize_t nread,
766768
}
767769

768770
TEST_F(InspectorSocketTest, CleanupSocketAfterEOF) {
769-
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
771+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
770772
expect_handshake();
771773

772774
inspector_read_start(&inspector, buffer_alloc_cb,
@@ -810,7 +812,7 @@ static void mask_message(const char* message,
810812
TEST_F(InspectorSocketTest, Send1Mb) {
811813
ASSERT_TRUE(connected);
812814
ASSERT_FALSE(inspector_ready);
813-
do_write(const_cast<char *>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
815+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
814816
SPIN_WHILE(!inspector_ready);
815817
expect_handshake();
816818

@@ -862,3 +864,23 @@ TEST_F(InspectorSocketTest, Send1Mb) {
862864
free(expected);
863865
free(message);
864866
}
867+
868+
static ssize_t err;
869+
870+
void ErrorCleansUpTheSocket_cb(uv_stream_t* stream, ssize_t read,
871+
const uv_buf_t* buf) {
872+
err = read;
873+
}
874+
875+
TEST_F(InspectorSocketTest, ErrorCleansUpTheSocket) {
876+
inspector_closed = 0;
877+
do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
878+
expect_handshake();
879+
const char NOT_A_GOOD_FRAME[] = {'H', 'e', 'l', 'l', 'o'};
880+
err = 42;
881+
inspector_read_start(&inspector, buffer_alloc_cb,
882+
ErrorCleansUpTheSocket_cb);
883+
do_write(NOT_A_GOOD_FRAME, sizeof(NOT_A_GOOD_FRAME));
884+
SPIN_WHILE(err > 0);
885+
EXPECT_EQ(UV_EPROTO, err);
886+
}

0 commit comments

Comments
 (0)