Skip to content

inspector: handle socket close before close frame #12937

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 0 commits into from
May 12, 2017
Merged

inspector: handle socket close before close frame #12937

merged 0 commits into from
May 12, 2017

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented May 9, 2017

This change handles clients that respond to close request with a TCP
close instead of close response.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

inpsector: updated the protocol handler and added a test case.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol labels May 9, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented May 9, 2017

At least one popular frontend is not properly following WS protocol specification, this change protects from such clients.

@@ -335,6 +335,10 @@ static void websockets_data_cb(uv_stream_t* stream, ssize_t nread,
if (!inspector->shutting_down && inspector->ws_state->read_cb) {
inspector->ws_state->read_cb(stream, nread, nullptr);
}
if (inspector->ws_state->close_sent
&& !inspector->ws_state->received_close) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the && to the previous line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@eugeneo
Copy link
Contributor Author

eugeneo commented May 12, 2017

@cjihrig I have addressed the comment. Please take another look.

@eugeneo
Copy link
Contributor Author

eugeneo commented May 12, 2017

@eugeneo eugeneo closed this May 12, 2017
@eugeneo eugeneo deleted the broken_close branch May 12, 2017 23:07
@eugeneo eugeneo merged commit 7c3a23b into nodejs:master May 12, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented May 12, 2017

Landed as 7c3a23b

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
This change handles clients that respond to close request with a TCP
close instead of close response.

PR-URL: nodejs#12937
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants