From c4aaf47f4d85d43e2ee594efba1b0c87c797a8af Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Mon, 27 Jun 2016 16:39:26 -0700 Subject: [PATCH] inspector: Do cleanups before notifying callback Inspector socket implementation was notifying handshake callback before performing the cleanups, which meant that callback could not reclaim resources allocated by the client. New implementation will free all resource not allocated by the client before calling the callback, allowing the client to complete the cleanup. Fixes: https://github.com/nodejs/node/pull/7418 PR-URL: https://github.com/nodejs/node/pull/7450 Reviewed-By: bnoordhuis - Ben Noordhuis --- src/inspector_socket.cc | 57 +++++++++++++++++----------- test/cctest/test_inspector_socket.cc | 5 ++- 2 files changed, 39 insertions(+), 23 deletions(-) diff --git a/src/inspector_socket.cc b/src/inspector_socket.cc index b860ef3bb740f7..d3a8d8efffc57c 100644 --- a/src/inspector_socket.cc +++ b/src/inspector_socket.cc @@ -22,6 +22,7 @@ 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; @@ -482,24 +483,42 @@ static void handshake_complete(inspector_socket_t* inspector) { inspector->http_parsing_state->path); } -static void cleanup_http_parsing_state(struct http_parsing_state_s* state) { +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); + inspector->http_parsing_state = nullptr; +} + +static void report_handshake_failure_cb(uv_handle_t* handle) { + dispose_inspector(handle); + inspector_socket_t* inspector = + static_cast(handle->data); + handshake_cb cb = inspector->http_parsing_state->callback; + cleanup_http_parsing_state(inspector); + cb(inspector, kInspectorHandshakeFailed, nullptr); +} + +static void close_and_report_handshake_failure(inspector_socket_t* inspector) { + uv_handle_t* socket = reinterpret_cast(&inspector->client); + if (uv_is_closing(socket)) { + report_handshake_failure_cb(socket); + } else { + uv_read_stop(reinterpret_cast(socket)); + uv_close(socket, report_handshake_failure_cb); + } } static void handshake_failed(inspector_socket_t* inspector) { - http_parsing_state_s* state = inspector->http_parsing_state; const char HANDSHAKE_FAILED_RESPONSE[] = "HTTP/1.0 400 Bad Request\r\n" "Content-Type: text/html; charset=UTF-8\r\n\r\n" "WebSockets request was expected\r\n"; write_to_client(inspector, HANDSHAKE_FAILED_RESPONSE, sizeof(HANDSHAKE_FAILED_RESPONSE) - 1); - close_connection(inspector); - inspector->http_parsing_state = nullptr; - state->callback(inspector, kInspectorHandshakeFailed, state->path); + close_and_report_handshake_failure(inspector); } // init_handshake references message_complete_cb @@ -542,11 +561,10 @@ static int message_complete_cb(http_parser* parser) { int len = sizeof(accept_response); if (write_to_client(inspector, accept_response, len) >= 0) { handshake_complete(inspector); + inspector->http_parsing_state->done = true; } else { - state->callback(inspector, kInspectorHandshakeFailed, nullptr); - close_connection(inspector); + close_and_report_handshake_failure(inspector); } - inspector->http_parsing_state = nullptr; } else { handshake_failed(inspector); } @@ -565,27 +583,21 @@ static void data_received_cb(uv_stream_s* client, ssize_t nread, #endif inspector_socket_t* inspector = reinterpret_cast((client->data)); - http_parsing_state_s* state = inspector->http_parsing_state; if (nread < 0 || nread == UV_EOF) { - inspector->http_parsing_state->callback(inspector, - kInspectorHandshakeFailed, - nullptr); - close_connection(inspector); - inspector->http_parsing_state = nullptr; + close_and_report_handshake_failure(inspector); } else { + http_parsing_state_s* state = inspector->http_parsing_state; http_parser* parser = &state->parser; - ssize_t parsed = http_parser_execute(parser, &state->parser_settings, - inspector->buffer, - nread); - if (parsed < nread) { + http_parser_execute(parser, &state->parser_settings, inspector->buffer, + nread); + if (parser->http_errno != HPE_OK) { handshake_failed(inspector); } + if (inspector->http_parsing_state->done) { + cleanup_http_parsing_state(inspector); + } inspector->data_len = 0; } - - if (inspector->http_parsing_state == nullptr) { - cleanup_http_parsing_state(state); - } } static void init_handshake(inspector_socket_t* inspector) { @@ -600,6 +612,7 @@ static void init_handshake(inspector_socket_t* inspector) { if (state->path) { state->path[0] = '\0'; } + state->done = false; http_parser_init(&state->parser, HTTP_REQUEST); state->parser.data = inspector; http_parser_settings* settings = &state->parser_settings; diff --git a/test/cctest/test_inspector_socket.cc b/test/cctest/test_inspector_socket.cc index eecb35f824f13a..7290a6e975b958 100644 --- a/test/cctest/test_inspector_socket.cc +++ b/test/cctest/test_inspector_socket.cc @@ -628,6 +628,8 @@ 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; @@ -677,15 +679,16 @@ HandshakeCanBeCanceled_handshake(enum inspector_handshake_event state, switch (handshake_events - 1) { case 0: EXPECT_EQ(kInspectorHandshakeUpgrading, state); + EXPECT_STREQ("/ws/path", path); break; case 1: EXPECT_EQ(kInspectorHandshakeFailed, state); + EXPECT_STREQ(nullptr, path); break; default: EXPECT_TRUE(false); break; } - EXPECT_STREQ("/ws/path", path); *cont = false; }