GeezerVison is showing github.com — net: handle undefined parent in _unrefTimer and _destroy · nodejs/node@05cc430
File tree
Expand file treeCollapse file tree
Open diff view settings
Expand file treeCollapse file tree
Open diff view settings
Collapse file
Copy file name to clipboardExpand all lines: lib/net.js +6-2Lines changed: 6 additions & 2 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -736,7 +736,9 @@ ObjectSetPrototypeOf(Socket, stream.Duplex); |
736 | 736 |
|
737 | 737 | // Refresh existing timeouts. |
738 | 738 | Socket.prototype._unrefTimer = function _unrefTimer() { |
739 | | - for (let s = this; s !== null; s = s._parent) { |
| 739 | + // `_parent` may be null; we use a loose `!= null` check in case external |
| 740 | + // code sets it to undefined. |
| 741 | + for (let s = this; s != null; s = s._parent) { |
740 | 742 | if (s[kTimeout]) |
741 | 743 | s[kTimeout].refresh(); |
742 | 744 | } |
@@ -1097,7 +1099,9 @@ Socket.prototype._destroy = function(exception, cb) { |
1097 | 1099 |
|
1098 | 1100 | this.connecting = false; |
1099 | 1101 |
|
1100 | | - for (let s = this; s !== null; s = s._parent) { |
| 1102 | + // `_parent` may be null; we use a loose `!= null` check in case external |
| 1103 | + // code sets it to undefined. |
| 1104 | + for (let s = this; s != null; s = s._parent) { |
1101 | 1105 | clearTimeout(s[kTimeout]); |
1102 | 1106 | } |
1103 | 1107 |
|
|
Collapse file
Copy file name to clipboard +25Lines changed: 25 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
|
| 1 | +'use strict'; |
| 2 | +const common = require('../common'); |
| 3 | + |
| 4 | +// Walking the `_parent` chain must stop on a nullish link, not only strict |
| 5 | +// `null`. During connection teardown a socket's `_parent` can be left |
| 6 | +// `undefined`, which previously caused `_unrefTimer()` and `_destroy()` to read |
| 7 | +// a property off `undefined` and throw. |
| 8 | +// Refs: https://github.com/nodejs/node/issues/64490 |
| 9 | + |
| 10 | +const assert = require('assert'); |
| 11 | +const net = require('net'); |
| 12 | + |
| 13 | +{ |
| 14 | + const socket = new net.Socket(); |
| 15 | + socket._parent = undefined; |
| 16 | + socket._unrefTimer(); |
| 17 | +} |
| 18 | + |
| 19 | +{ |
| 20 | + const socket = new net.Socket(); |
| 21 | + socket._parent = undefined; |
| 22 | + socket.on('error', common.mustNotCall()); |
| 23 | + socket.destroy(); |
| 24 | + assert.strictEqual(socket.destroyed, true); |
| 25 | +} |
Collapse file
Copy file name to clipboard +39Lines changed: 39 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
|
| 1 | +'use strict'; |
| 2 | + |
| 3 | +const common = require('../common'); |
| 4 | + |
| 5 | +if (!common.hasCrypto) |
| 6 | + common.skip('missing crypto'); |
| 7 | + |
| 8 | +const tls = require('tls'); |
| 9 | +const fixtures = require('../common/fixtures'); |
| 10 | + |
| 11 | +// A TLS socket whose `_parent` is left `undefined` during teardown must not |
| 12 | +// crash when reads land on it (`onStreamRead` -> `_unrefTimer`) or when it is |
| 13 | +// destroyed (`_destroy`). Both walk the `_parent` chain. |
| 14 | +// Refs: https://github.com/nodejs/node/issues/64490 |
| 15 | + |
| 16 | +const options = { |
| 17 | + key: fixtures.readKey('agent1-key.pem'), |
| 18 | + cert: fixtures.readKey('agent1-cert.pem'), |
| 19 | +}; |
| 20 | + |
| 21 | +const server = tls.createServer(options, common.mustCall((conn) => { |
| 22 | + setTimeout(() => conn.write('x'), 50); |
| 23 | +})); |
| 24 | + |
| 25 | +server.listen(0, common.mustCall(() => { |
| 26 | + const client = tls.connect({ |
| 27 | + port: server.address().port, |
| 28 | + rejectUnauthorized: false, |
| 29 | + }, common.mustCall(() => { |
| 30 | + client._parent = undefined; |
| 31 | + })); |
| 32 | + |
| 33 | + client.on('data', common.mustCall(() => { |
| 34 | + server.close(); |
| 35 | + client.destroy(); |
| 36 | + })); |
| 37 | + |
| 38 | + client.on('error', common.mustNotCall()); |
| 39 | +})); |