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

crash in node_buffer.cc on ECONNRESET when using net.Socket onread #34346

Closed
robey opened this issue Jul 14, 2020 · 2 comments
Closed

crash in node_buffer.cc on ECONNRESET when using net.Socket onread #34346

robey opened this issue Jul 14, 2020 · 2 comments
Labels
net Issues and PRs related to the net subsystem.

Comments

@robey
Copy link
Contributor

robey commented Jul 14, 2020

  • Version: 14.4.0
  • Platform: Linux 4.19
  • Subsystem: net

occasionally, we see nodejs 14.4.0 drop core with a message like:

E 2020-07-01T21:38:27.737670958Z node[1]: ../src/node_buffer.cc:242:char* node::Buffer::Data(v8::Local<v8::Value>): Assertion `val->IsArrayBufferView()' failed.
I 2020-07-01T21:38:27.737769249Z [20200701-21:38:27.737] TRA conclave-5c84d47bd8-5b4pj (03e741ad-68e6-4f01-939c-922b53fb6004) @52: Socket error: read ECONNRESET
E 2020-07-01T21:38:27.738570272Z  1: 0xa2b020 node::Abort() [node]
E 2020-07-01T21:38:27.739024375Z  2: 0xa2b09e  [node]
E 2020-07-01T21:38:27.739592654Z  3: 0xa037aa node::Buffer::Data(v8::Local<v8::Value>) [node]
E 2020-07-01T21:38:27.740180478Z  4: 0xaf6481 node::CustomBufferJSListener::OnStreamRead(long, uv_buf_t const&) [node]
E 2020-07-01T21:38:27.740705062Z  5: 0xb02058 node::LibuvStreamWrap::OnUvRead(long, uv_buf_t const*) [node]
E 2020-07-01T21:38:27.741487768Z  6: 0x132fb41  [node]
E 2020-07-01T21:38:27.742341825Z  7: 0x1330310  [node]
E 2020-07-01T21:38:27.743122216Z  8: 0x1336c50  [node]
E 2020-07-01T21:38:27.743991594Z  9: 0x13250f8 uv_run [node]
E 2020-07-01T21:38:27.744573560Z 10: 0xa6b9b4 node::NodeMainInstance::Run() [node]
E 2020-07-01T21:38:27.745148797Z 11: 0x9fae81 node::Start(int, char**) [node]
E 2020-07-01T21:38:27.745172061Z 12: 0x7f290cdd22e1 __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
E 2020-07-01T21:38:27.745732198Z 13: 0x99529c  [node]

it seems to be always the same location, and always in response to an ECONNRESET on the socket. it only happens when using the "onread" hook in net.Socket.

the good news: after some trial & error, and digging through the nodejs code, i think i figured out why:

when we set "onread", the Socket init sets kBuffer and kBufferCb (net.js:306). then initSocketHandle will call useUserBuffer on the handle (net.js:254). this sets the uv listener to be a CustomBufferJSListener (stream_base.cc:59). so far so good.

when new data arrives, OnStreamRead calls CallJSOnreadMethod (stream_base.cc:535). if the JS method returns a value, it's assumed to be a new buffer to use next time.

the JS method is onStreamRead (stream_base_commons.js:163). if we got data, it returns nothing, unless kBufferGen is set, in which case it fetches a new buffer and returns that. cool.

if we got a non-EOF error, it instead destroys the socket and returns (line 205). unfortunately it uses a shortcut and returns whatever value destroy returns. destroy (destroy.js:5) always returns this. so the Socket gets returned and treated as a new buffer until it fails the assertion in Buffer::Data and crashes.

i think the easiest fix for this is to replace net.js line 205 to not return the socket:

    stream.destroy(errnoException(nread, 'read'));
    return;

i tested this by monkey-patching the socket destroy function to return nothing:

    const old_destroy = socket.destroy;
    socket.destroy = function (...args) { old_destroy.apply(this, args); };

this has been running for 6 days now without a crash, so i think it proves the fix.

@lpinca lpinca added the net Issues and PRs related to the net subsystem. label Jul 14, 2020
@lpinca
Copy link
Member

lpinca commented Jul 14, 2020

@mscdex

@robey
Copy link
Contributor Author

robey commented Jul 18, 2020

i think i managed to make a test script which will reliably crash under linux:

const net = require("net");

const delay = n => new Promise(resolve => setTimeout(resolve, n));

const server = net.createServer({ pauseOnConnect: true }, async c => {
  c.on("error", error => console.log(`s: error ${JSON.stringify(error)}`));
  console.log(`s: new connection`);
  c.write(Buffer.from([ 1 ]));
  await delay(100);
  c.destroy();
});

server.listen(async () => {
  console.log(`s: started at ${server.address().port}`);

  const client = net.connect({
    port: server.address().port,
    host: server.address().address,
    onread: {
      buffer: Buffer.alloc(1),
      callback: (n, data) => console.log(`c: onread ${n} ${data.toString("hex")}`),
    },
  });
  client.on("error", error => console.log(`c: error ${JSON.stringify(error)}`));
  client.write(Buffer.from([ 2 ]));
});

which gives

> node --version
v14.5.0

> node reset.js
s: started at 43557
s: new connection
c: onread 1 01
c: error {"errno":-104,"code":"ECONNRESET","syscall":"read"}
node[25872]: ../src/node_buffer.cc:243:char* node::Buffer::Data(v8::Local<v8::Value>): Assertion `val->IsArrayBufferView()' failed.
 1: 0xa3ac30 node::Abort() [node]
 2: 0xa3acae  [node]
 3: 0xa139ba node::Buffer::Data(v8::Local<v8::Value>) [node]
 4: 0xb04cf1 node::CustomBufferJSListener::OnStreamRead(long, uv_buf_t const&) [node]
 5: 0xb10a98 node::LibuvStreamWrap::OnUvRead(long, uv_buf_t const*) [node]
 6: 0x137aa81  [node]
 7: 0x137b250  [node]
 8: 0x1381ae0  [node]
 9: 0x136ff58 uv_run [node]
10: 0xa7ab94 node::NodeMainInstance::Run() [node]
11: 0xa0a861 node::Start(int, char**) [node]
12: 0x7f6baf7c11e3 __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
13: 0x9a526c  [node]
fish: “node reset.js” terminated by signal SIGABRT (Abort)

addaleax pushed a commit that referenced this issue 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 <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
codebytere pushed a commit that referenced this issue 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 <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit that referenced this issue 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 <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit that referenced this issue 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 <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants