Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 8dd48c9

Browse filesBrowse 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
Copy full SHA for 8dd48c9

File tree

Expand file treeCollapse file tree

3 files changed

+57
-44
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+57
-44
lines changed
Open diff view settings
Collapse file

‎src/inspector_agent.cc‎

Copy file name to clipboardExpand all lines: src/inspector_agent.cc
+7-11Lines 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;
Collapse file

‎src/inspector_socket.cc‎

Copy file name to clipboardExpand all lines: src/inspector_socket.cc
-5Lines 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

Collapse file

‎test/cctest/test_inspector_socket.cc‎

Copy file name to clipboardExpand all lines: test/cctest/test_inspector_socket.cc
+50-28Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.