New issue

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

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

Already on GitHub? Sign in to your account

inspector: Simplify buffer management #8257

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@eugeneo
Contributor

eugeneo commented Aug 24, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

This only touches the protocol implementation for the V8 inspector.

Description of change

This change simplifies buffer management to address a number of issues
that original implementation had.

Original implementation was trying to reduce the number of allocations
by providing regions of the internal buffer to libuv IO code. This
introduced some potential use after free issues if the buffer grows
(or shrinks) while there's a pending read. It also had some confusing
math that resulted in issues on Windows version of the libuv.

Fixes: #8155
CC: @ofrobots

@joaocgreis

This comment has been minimized.

Show comment
Hide comment
@joaocgreis

joaocgreis Aug 25, 2016

Member

I confirm this fixes #8155 for me under Windows 2012.

Member

joaocgreis commented Aug 25, 2016

I confirm this fixes #8155 for me under Windows 2012.

@@ -48,16 +49,17 @@ static void dump_hex(const char* buf, size_t len) {
}
#endif
static void remove_from_beginning(std::vector<char>* buffer, size_t count) {

This comment has been minimized.

@addaleax

addaleax Aug 25, 2016

Member

nit: I’m fine with having this the way it is, but to me using pointers to STL containers always seems a bit unidiomatic – this seems like a very okay place to use references?

@addaleax

addaleax Aug 25, 2016

Member

nit: I’m fine with having this the way it is, but to me using pointers to STL containers always seems a bit unidiomatic – this seems like a very okay place to use references?

This comment has been minimized.

@eugeneo

eugeneo Aug 25, 2016

Contributor

I used a pointer here to indicate that the parameter will be modified... Should I use a non-const ref?

@eugeneo

eugeneo Aug 25, 2016

Contributor

I used a pointer here to indicate that the parameter will be modified... Should I use a non-const ref?

This comment has been minimized.

@addaleax

addaleax Aug 25, 2016

Member

I don’t feel too strongly about it. I’d personally have opted for a non-const reference, but if you or anyone else prefers it this way, that’s definitely okay too.

@addaleax

addaleax Aug 25, 2016

Member

I don’t feel too strongly about it. I’d personally have opted for a non-const reference, but if you or anyone else prefers it this way, that’s definitely okay too.

This comment has been minimized.

@ofrobots

ofrobots Aug 26, 2016

Contributor

I think @eugeneo is following this rationale which I subscribe to as well (I might be biased though :).

@ofrobots

ofrobots Aug 26, 2016

Contributor

I think @eugeneo is following this rationale which I subscribe to as well (I might be biased though :).

This comment has been minimized.

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

The house style in node.js is pointers when arguments are modified. References should always be const.

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

The house style in node.js is pointers when arguments are modified. References should always be const.

This comment has been minimized.

@addaleax

addaleax Aug 27, 2016

Member

Thanks for clarifying, both of you :)

@addaleax

addaleax Aug 27, 2016

Member

Thanks for clarifying, both of you :)

@addaleax

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_socket.cc
return FRAME_INCOMPLETE;
const char* p = buffer_begin;
const char* buffer_end = p + data_length;
std::vector<char>::const_iterator p = buffer.begin();

This comment has been minimized.

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

I'd be okay with writing this as auto p = buffer.cbegin() (but maybe call it it to make it clear it's an iterator, not a pointer.)

EDIT: Or auto p = buffer.begin(), seeing that buffer is already const.

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

I'd be okay with writing this as auto p = buffer.cbegin() (but maybe call it it to make it clear it's an iterator, not a pointer.)

EDIT: Or auto p = buffer.begin(), seeing that buffer is already const.

This comment has been minimized.

@eugeneo

eugeneo Aug 26, 2016

Contributor

Done

@eugeneo

eugeneo Aug 26, 2016

Contributor

Done

@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_socket.cc
*bytes_consumed = pos;
return closed ? FRAME_CLOSE : FRAME_OK;
}
inspector_socket_t* inspector_from_stream(void* client) {
return node::ContainerOf(&inspector_socket_t::client,
reinterpret_cast<uv_tcp_t*>(client));

This comment has been minimized.

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

Can you line up the arguments?

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

Can you line up the arguments?

This comment has been minimized.

@eugeneo

eugeneo Aug 26, 2016

Contributor

Done

@eugeneo

eugeneo Aug 26, 2016

Contributor

Done

@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_socket.cc
std::vector<char>& buffer = inspector->buffer;
size_t old_size = buffer.size();
buffer.resize(buffer.size() + read);
std::copy(buf->base, buf->base + read, buffer.begin() + old_size);

This comment has been minimized.

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

buffer.insert()?

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

buffer.insert()?

This comment has been minimized.

@eugeneo

eugeneo Aug 26, 2016

Contributor

Right... Thanks!

@eugeneo

eugeneo Aug 26, 2016

Contributor

Right... Thanks!

@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_socket.cc
if (nread < 0 || nread == UV_EOF) {
close_and_report_handshake_failure(inspector);
} else {
http_parsing_state_s* state = inspector->http_parsing_state;
http_parsing_state_s* state =
inspector->http_parsing_state;

This comment has been minimized.

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

Ditto.

@bnoordhuis

This comment has been minimized.

@eugeneo

eugeneo Aug 26, 2016

Contributor

Done

@eugeneo

eugeneo Aug 26, 2016

Contributor

Done

@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_socket.cc
@@ -482,7 +475,7 @@ static int message_complete_cb(http_parser* parser) {
inspector_socket_t* inspector =
reinterpret_cast<inspector_socket_t*>(parser->data);
struct http_parsing_state_s* state =
(struct http_parsing_state_s*) inspector->http_parsing_state;
inspector->http_parsing_state;

This comment has been minimized.

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

This probably fits on one line now?

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

This probably fits on one line now?

This comment has been minimized.

@eugeneo

eugeneo Aug 26, 2016

Contributor

Done

@eugeneo

eugeneo Aug 26, 2016

Contributor

Done

@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_socket.h
@@ -64,12 +62,15 @@ int inspector_accept(uv_stream_t* server, struct inspector_socket_s* inspector,
void inspector_close(struct inspector_socket_s* inspector,
inspector_cb callback);
// Callbacks will receive handles that has inspector in data field...
// Callbacks will receive stream handles, use inspector_from_stream to get
// inspector

This comment has been minimized.

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

Can you punctuate the comment?

@bnoordhuis

bnoordhuis Aug 26, 2016

Member

Can you punctuate the comment?

This comment has been minimized.

@eugeneo

eugeneo Aug 26, 2016

Contributor

Done

@eugeneo

eugeneo Aug 26, 2016

Contributor

Done

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 26, 2016

Contributor

I addressed the comments and uploaded a new version. Thank you for the review. Please take another look.

Contributor

eugeneo commented Aug 26, 2016

I addressed the comments and uploaded a new version. Thank you for the review. Please take another look.

@bnoordhuis

View changes

Show outdated Hide outdated src/inspector_socket.cc
inspector_socket_t* inspector_from_stream(void* client) {
return node::ContainerOf(&inspector_socket_t::client,
reinterpret_cast<uv_tcp_t*>(client));
}

This comment has been minimized.

@bnoordhuis

bnoordhuis Aug 27, 2016

Member

This could be made a little more type-safe by having overloads for uv_handle_t and uv_stream_t:

inspector_socket_t* inspector_from_tcp(uv_tcp_t* client) {
  return node::ContainerOf(&inspector_socket_t::client, client);
}
inspector_socket_t* inspector_from_stream(uv_stream_t* client) {
  return inspector_from_tcp(reinterpret_cast<uv_tcp_t*>(client));
}
inspector_socket_t* inspector_from_handle(uv_handle_t* client) {
  return inspector_from_tcp(reinterpret_cast<uv_tcp_t*>(client));
}
@bnoordhuis

bnoordhuis Aug 27, 2016

Member

This could be made a little more type-safe by having overloads for uv_handle_t and uv_stream_t:

inspector_socket_t* inspector_from_tcp(uv_tcp_t* client) {
  return node::ContainerOf(&inspector_socket_t::client, client);
}
inspector_socket_t* inspector_from_stream(uv_stream_t* client) {
  return inspector_from_tcp(reinterpret_cast<uv_tcp_t*>(client));
}
inspector_socket_t* inspector_from_handle(uv_handle_t* client) {
  return inspector_from_tcp(reinterpret_cast<uv_tcp_t*>(client));
}
@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 27, 2016

Member

LGTM with a suggestion.

Member

bnoordhuis commented Aug 27, 2016

LGTM with a suggestion.

Eugene Ostroukhov
inspector: Simplify buffer management
This change simplifies buffer management to address a number of issues
that original implementation had.

Original implementation was trying to reduce the number of allocations
by providing regions of the internal buffer to libuv IO code. This
introduced some potential use after free issues if the buffer grows
(or shrinks) while there's a pending read. It also had some confusing
math that resulted in issues on Windows version of the libuv.

Fixes: #8155
@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 29, 2016

Contributor

Thank you for the review. I've implemented the suggestion. Please take another look.

Contributor

eugeneo commented Aug 29, 2016

Thank you for the review. I've implemented the suggestion. Please take another look.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 30, 2016

Member

LGTM but s/Simplify/simplify/ in the commit log status line (it's normally all lower case.)

Member

bnoordhuis commented Aug 30, 2016

LGTM but s/Simplify/simplify/ in the commit log status line (it's normally all lower case.)

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 30, 2016

Member

LGTM

Member

bnoordhuis commented Aug 30, 2016

LGTM

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 30, 2016

Member

CI is red, looks like a build bot failure.

Member

jasnell commented Aug 30, 2016

CI is red, looks like a build bot failure.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Aug 31, 2016

Contributor

ARM fanned builds seemed to be having issues yesterday. Let's hit the CI one more time: https://ci.nodejs.org/job/node-test-pull-request/3904/

Contributor

ofrobots commented Aug 31, 2016

ARM fanned builds seemed to be having issues yesterday. Let's hit the CI one more time: https://ci.nodejs.org/job/node-test-pull-request/3904/

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Aug 31, 2016

Contributor

New CI has a failure on the new ubuntu1204-clang-3.4.1 bot (expected, see #8343). The timeout on the mac build looks like a flake. I will land this shortly.

Contributor

ofrobots commented Aug 31, 2016

New CI has a failure on the new ubuntu1204-clang-3.4.1 bot (expected, see #8343). The timeout on the mac build looks like a flake. I will land this shortly.

ofrobots added a commit that referenced this pull request Aug 31, 2016

inspector: simplify buffer management
This change simplifies buffer management to address a number of issues
that original implementation had.

Original implementation was trying to reduce the number of allocations
by providing regions of the internal buffer to libuv IO code. This
introduced some potential use after free issues if the buffer grows
(or shrinks) while there's a pending read. It also had some confusing
math that resulted in issues on Windows version of the libuv.

PR-URL: #8257
Fixes: #8155
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Aug 31, 2016

Contributor

Landed as b6db963.

Contributor

ofrobots commented Aug 31, 2016

Landed as b6db963.

@ofrobots ofrobots closed this Aug 31, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

inspector: simplify buffer management
This change simplifies buffer management to address a number of issues
that original implementation had.

Original implementation was trying to reduce the number of allocations
by providing regions of the internal buffer to libuv IO code. This
introduced some potential use after free issues if the buffer grows
(or shrinks) while there's a pending read. It also had some confusing
math that resulted in issues on Windows version of the libuv.

PR-URL: nodejs#8257
Fixes: nodejs#8155
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

inspector: simplify buffer management
This change simplifies buffer management to address a number of issues
that original implementation had.

Original implementation was trying to reduce the number of allocations
by providing regions of the internal buffer to libuv IO code. This
introduced some potential use after free issues if the buffer grows
(or shrinks) while there's a pending read. It also had some confusing
math that resulted in issues on Windows version of the libuv.

PR-URL: #8257
Fixes: #8155
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

@eugeneo eugeneo deleted the eugeneo:buffer branch Sep 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment