Skip to content
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

net/streams: don't return the stream object from onStreamRead #34375

Closed
wants to merge 10 commits into from
Closed

net/streams: don't return the stream object from onStreamRead #34375

wants to merge 10 commits into from

Conversation

robey
Copy link
Contributor

@robey robey commented Jul 15, 2020

CallJSOnreadMethod expects the return value to be undefined or
a new buffer, so make sure to return nothing, even when an error
causes us to destroy the stream.

Fixes: #34346

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
    • (except test-benchmark-napi, which has been broken since I cloned)
  • commit message follows commit guidelines

@lpinca
Copy link
Member

lpinca commented Jul 15, 2020

Is it possible to add a regression test?

@robey
Copy link
Contributor Author

robey commented Jul 15, 2020

i think you would need to have some integration tests, sadly. it doesn't seem possible for a connection to be "reset" between two sockets on the same machine. (my understanding is that ECONNRESET is a response to unexpectedly receiving a TCP RST.)

another way you could do it is to fake up the ECONNRESET by mucking around in the internals somehow, but i'm not sure how you can trick libuv into doing that. would be cool tho!

@lpinca
Copy link
Member

lpinca commented Jul 15, 2020

I think net: is a better subsystem in commit title.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

@robey robey requested a review from a team July 18, 2020 00:22
@robey
Copy link
Contributor Author

robey commented Jul 18, 2020

good eye! i went ahead and fixed that one too, while i was in here.

@robey robey changed the title streams: don't return the stream object from onStreamRead net/streams: don't return the stream object from onStreamRead Jul 18, 2020
@mcollina
Copy link
Member

Can you please add a unit test?

@robey
Copy link
Contributor Author

robey commented Jul 18, 2020

as i said above, if you can think of a way to do it without having a cross-process integration test, i'm happy to try it.

@mcollina
Copy link
Member

would it be possible to use child processes? We have a few tests that use them for similar purposes.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I would probably also update the crash site in stream_base.cc to check for next_buf_v->IsArrayBufferView() instead of !next_buf_v->IsUndefined() (although that can be done as a separate fix)

@robey
Copy link
Contributor Author

robey commented Jul 19, 2020

i figured out some sample code that causes the crash (see bug), so i added that as a unit test. :)

@robey
Copy link
Contributor Author

robey commented Jul 19, 2020

I would probably also update the crash site in stream_base.cc to check for next_buf_v->IsArrayBufferView() instead of !next_buf_v->IsUndefined() (although that can be done as a separate fix)

sure, added that too. :)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@robey robey requested a review from ronag July 20, 2020 18:21
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Still LGTM :)

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 20, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member

jasnell commented Jul 23, 2020

Looks like there are a number of possibly relevant failures in CI

@jasnell jasnell added net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 23, 2020
@robey
Copy link
Contributor Author

robey commented Jul 23, 2020

Looks like there are a number of possibly relevant failures in CI

i assumed these were all spurious since "make test" passes cleanly (except the one that always fails). which test is failing for you and how can i repro it?

@lpinca
Copy link
Member

lpinca commented Jul 23, 2020

@robey here are a few examples:

node-test-commit-freebsd

21:31:45 not ok 1645 parallel/test-net-onread-static-buffer
21:31:45   ---
21:31:45   duration_ms: 0.441
21:31:45   severity: fail
21:31:45   exitcode: 1
21:31:45   stack: |-
21:31:45     Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
21:31:45         at Proxy.mustCall (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/common/index.js:331:10)
21:31:45         at Server.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-net-onread-static-buffer.js:203:29)
21:31:45         at Object.onceWrapper (events.js:420:28)
21:31:45         at Server.emit (events.js:314:20)
21:31:45         at emitListeningNT (net.js:1322:10)
21:31:45         at processTicksAndRejections (internal/process/task_queues.js:79:21)

node-test-commit-smartos

21:31:32 not ok 1470 parallel/test-net-onread-static-buffer
21:31:32   ---
21:31:32   duration_ms: 0.536
21:31:32   severity: fail
21:31:32   exitcode: 1
21:31:32   stack: |-
21:31:32     Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
21:31:32         at Proxy.mustCall (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos18-64/test/common/index.js:331:10)
21:31:32         at Server.<anonymous> (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos18-64/test/parallel/test-net-onread-static-buffer.js:203:29)
21:31:32         at Object.onceWrapper (events.js:420:28)
21:31:32         at Server.emit (events.js:314:20)
21:31:32         at emitListeningNT (net.js:1322:10)
21:31:32         at processTicksAndRejections (internal/process/task_queues.js:79:21)

node-test-commit-windows-fanned

21:26:37 not ok 409 parallel/test-net-onread-static-buffer
21:26:37   ---
21:26:37   duration_ms: 0.633
21:26:37   severity: fail
21:26:37   exitcode: 134
21:26:37   stack: |-
21:26:37     Administrator: Administrator: Windows PowerShell[8864]: C:\workspace\node-compile-windows\node\src\stream_base.cc:534: Assertion `(buf.base) == (buffer_.base)' failed.
21:26:37      1: 00007FF64E5CA33F v8::internal::Isolate::ArchiveSpacePerThread+4127
21:26:37      2: 00007FF64E56AC06 v8::internal::wasm::WasmCode::safepoint_table_offset+65046
21:26:37      3: 00007FF64E56AF81 v8::internal::wasm::WasmCode::safepoint_table_offset+65937
21:26:37      4: 00007FF64E4917D1 v8::internal::Isolate::isolate_root_bias+15889
21:26:37      5: 00007FF64E48C639 v8::internal::MicrotaskQueue::microtasks_policy+1257
21:26:37      6: 00007FF64E60939F uv_tty_set_vterm_state+8847
21:26:37      7: 00007FF64E61EE8C uv_loop_init+844
21:26:37      8: 00007FF64E61F19A uv_run+202
21:26:37      9: 00007FF64E523D25 v8::internal::OrderedHashTable<v8::internal::OrderedHashSet,1>::NumberOfBucketsOffset+10277
21:26:37     10: 00007FF64E598AC6 node::Start+262
21:26:37     11: 00007FF64E3F785C RC4_options+343900
21:26:37     12: 00007FF64F38B00C v8::internal::compiler::RepresentationChanger::Uint32OverflowOperatorFor+156156
21:26:37     13: 00007FFDFF637BD4 BaseThreadInitThunk+20
21:26:37     14: 00007FFE00ACCE51 RtlUserThreadStart+33

@@ -538,7 +538,8 @@ void CustomBufferJSListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
0,
StreamBase::SKIP_NREAD_CHECKS);
Local<Value> next_buf_v;
if (ret.ToLocal(&next_buf_v) && !next_buf_v->IsUndefined()) {
if (ret.ToLocal(&next_buf_v) && !next_buf_v->IsUndefined() &&
next_buf_v->IsArrayBufferView()) {
Copy link
Member

Choose a reason for hiding this comment

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

Might this be what's causing problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense... ok, i reverted that part.

CallJSOnreadMethod expects the return value to be undefined or
a new buffer, so make sure to return nothing, even when an error
causes us to destroy the stream.

Fixes: #34346
@robey
Copy link
Contributor Author

robey commented Aug 7, 2020

it doesn't seem like there's any interest in this patch, so i'll close it next week. we can run a fork of node internally. sorry it didn't work out.

@nodejs-github-bot
Copy link
Collaborator

@lpinca
Copy link
Member

lpinca commented Aug 7, 2020

CI is green. I think this can land.

@lpinca lpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 7, 2020
@addaleax
Copy link
Member

addaleax commented Aug 8, 2020

Landed in c0be31f, and sorry for the delay!

@addaleax addaleax closed this Aug 8, 2020
addaleax pushed a commit that referenced this pull request Aug 8, 2020
CallJSOnreadMethod expects the return value to be undefined or
a new buffer, so make sure to return nothing, even when an error
causes us to destroy the stream.

Fixes: #34346

PR-URL: #34375
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 8, 2020
CallJSOnreadMethod expects the return value to be undefined or
a new buffer, so make sure to return nothing, even when an error
causes us to destroy the stream.

Fixes: #34346

PR-URL: #34375
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@codebytere codebytere mentioned this pull request Aug 10, 2020
codebytere pushed a commit that referenced this pull request Aug 11, 2020
CallJSOnreadMethod expects the return value to be undefined or
a new buffer, so make sure to return nothing, even when an error
causes us to destroy the stream.

Fixes: #34346

PR-URL: #34375
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
CallJSOnreadMethod expects the return value to be undefined or
a new buffer, so make sure to return nothing, even when an error
causes us to destroy the stream.

Fixes: #34346

PR-URL: #34375
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
CallJSOnreadMethod expects the return value to be undefined or
a new buffer, so make sure to return nothing, even when an error
causes us to destroy the stream.

Fixes: #34346

PR-URL: #34375
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crash in node_buffer.cc on ECONNRESET when using net.Socket onread
7 participants