Skip to content

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

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

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>
# for free to join this conversation on GitHub. Already have an account? # 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