net: handle undefined parent in _unrefTimer and _destroy

The fix and approach are from #64491 by Shivay-98; this reopens it to
get it landed, since the original stalled awaiting requested changes.
`Socket.prototype._unrefTimer` and `Socket.prototype._destroy` both walk
the `_parent` chain with a strict `!== null` check. During connection
teardown a socket's `_parent` can be left `undefined` (for example a TLS
socket layered over another stream), so the loop steps onto `undefined`
and reads a property off it, throwing a TypeError:
Cannot read properties of undefined (reading 'Symbol(timeout)')
from an uncaught I/O callback and crashing the process.
Using a nullish (`!= null`) check terminates the walk on both `null` and
`undefined`.
[petter@hightouch.io: apply the same fix to the identical loop in
`_destroy`, which the original regression test already exercised via
`destroy()`; add direct unit coverage for both paths.]
Fixes: #64490
Refs: #64491
Signed-off-by: Petter Häggholm <petter@hightouch.io>
PR-URL: #64644Fixes: #64490
Refs: #64491
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Author: Shivay-98

lib/net.js

@@ -736,7 +736,9 @@ ObjectSetPrototypeOf(Socket, stream.Duplex);
 
 // Refresh existing timeouts.
 Socket.prototype._unrefTimer=function_unrefTimer(){
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 if(s[kTimeout])
 s[kTimeout].refresh();
 }
@@ -1097,7 +1099,9 @@ Socket.prototype._destroy = function(exception, cb) {
 
 this.connecting=false;
 
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 clearTimeout(s[kTimeout]);
 }
 

test/parallel/test-net-socket-unref-timer-parent-chain.js

@@ -0,0 +1,25 @@
+'use strict';
+constcommon=require('../common');
+
+// Walking the `_parent` chain must stop on a nullish link, not only strict
+// `null`. During connection teardown a socket's `_parent` can be left
+// `undefined`, which previously caused `_unrefTimer()` and `_destroy()` to read
+// a property off `undefined` and throw.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constassert=require('assert');
+constnet=require('net');
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket._unrefTimer();
+}
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket.on('error',common.mustNotCall());
+socket.destroy();
+assert.strictEqual(socket.destroyed,true);
+}

test/parallel/test-net-unref-timer-parent-undefined.js

@@ -0,0 +1,39 @@
+'use strict';
+
+constcommon=require('../common');
+
+if(!common.hasCrypto)
+common.skip('missing crypto');
+
+consttls=require('tls');
+constfixtures=require('../common/fixtures');
+
+// A TLS socket whose `_parent` is left `undefined` during teardown must not
+// crash when reads land on it (`onStreamRead` -> `_unrefTimer`) or when it is
+// destroyed (`_destroy`). Both walk the `_parent` chain.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constoptions={
+key: fixtures.readKey('agent1-key.pem'),
+cert: fixtures.readKey('agent1-cert.pem'),
+};
+
+constserver=tls.createServer(options,common.mustCall((conn)=>{
+setTimeout(()=>conn.write('x'),50);
+}));
+
+server.listen(0,common.mustCall(()=>{
+constclient=tls.connect({
+port: server.address().port,
+rejectUnauthorized: false,
+},common.mustCall(()=>{
+client._parent=undefined;
+}));
+
+client.on('data',common.mustCall(()=>{
+server.close();
+client.destroy();
+}));
+
+client.on('error',common.mustNotCall());
+}));

lib/net.js

@@ -736,7 +736,9 @@ ObjectSetPrototypeOf(Socket, stream.Duplex);
 
 // Refresh existing timeouts.
 Socket.prototype._unrefTimer=function_unrefTimer(){
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 if(s[kTimeout])
 s[kTimeout].refresh();
 }
@@ -1097,7 +1099,9 @@ Socket.prototype._destroy = function(exception, cb) {
 
 this.connecting=false;
 
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 clearTimeout(s[kTimeout]);
 }
 

test/parallel/test-net-socket-unref-timer-parent-chain.js

@@ -0,0 +1,25 @@
+'use strict';
+constcommon=require('../common');
+
+// Walking the `_parent` chain must stop on a nullish link, not only strict
+// `null`. During connection teardown a socket's `_parent` can be left
+// `undefined`, which previously caused `_unrefTimer()` and `_destroy()` to read
+// a property off `undefined` and throw.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constassert=require('assert');
+constnet=require('net');
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket._unrefTimer();
+}
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket.on('error',common.mustNotCall());
+socket.destroy();
+assert.strictEqual(socket.destroyed,true);
+}

test/parallel/test-net-unref-timer-parent-undefined.js

@@ -0,0 +1,39 @@
+'use strict';
+
+constcommon=require('../common');
+
+if(!common.hasCrypto)
+common.skip('missing crypto');
+
+consttls=require('tls');
+constfixtures=require('../common/fixtures');
+
+// A TLS socket whose `_parent` is left `undefined` during teardown must not
+// crash when reads land on it (`onStreamRead` -> `_unrefTimer`) or when it is
+// destroyed (`_destroy`). Both walk the `_parent` chain.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constoptions={
+key: fixtures.readKey('agent1-key.pem'),
+cert: fixtures.readKey('agent1-cert.pem'),
+};
+
+constserver=tls.createServer(options,common.mustCall((conn)=>{
+setTimeout(()=>conn.write('x'),50);
+}));
+
+server.listen(0,common.mustCall(()=>{
+constclient=tls.connect({
+port: server.address().port,
+rejectUnauthorized: false,
+},common.mustCall(()=>{
+client._parent=undefined;
+}));
+
+client.on('data',common.mustCall(()=>{
+server.close();
+client.destroy();
+}));
+
+client.on('error',common.mustNotCall());
+}));

lib/net.js

@@ -736,7 +736,9 @@ ObjectSetPrototypeOf(Socket, stream.Duplex);
 
 // Refresh existing timeouts.
 Socket.prototype._unrefTimer=function_unrefTimer(){
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 if(s[kTimeout])
 s[kTimeout].refresh();
 }
@@ -1097,7 +1099,9 @@ Socket.prototype._destroy = function(exception, cb) {
 
 this.connecting=false;
 
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 clearTimeout(s[kTimeout]);
 }
 

test/parallel/test-net-socket-unref-timer-parent-chain.js

@@ -0,0 +1,25 @@
+'use strict';
+constcommon=require('../common');
+
+// Walking the `_parent` chain must stop on a nullish link, not only strict
+// `null`. During connection teardown a socket's `_parent` can be left
+// `undefined`, which previously caused `_unrefTimer()` and `_destroy()` to read
+// a property off `undefined` and throw.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constassert=require('assert');
+constnet=require('net');
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket._unrefTimer();
+}
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket.on('error',common.mustNotCall());
+socket.destroy();
+assert.strictEqual(socket.destroyed,true);
+}

test/parallel/test-net-unref-timer-parent-undefined.js

@@ -0,0 +1,39 @@
+'use strict';
+
+constcommon=require('../common');
+
+if(!common.hasCrypto)
+common.skip('missing crypto');
+
+consttls=require('tls');
+constfixtures=require('../common/fixtures');
+
+// A TLS socket whose `_parent` is left `undefined` during teardown must not
+// crash when reads land on it (`onStreamRead` -> `_unrefTimer`) or when it is
+// destroyed (`_destroy`). Both walk the `_parent` chain.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constoptions={
+key: fixtures.readKey('agent1-key.pem'),
+cert: fixtures.readKey('agent1-cert.pem'),
+};
+
+constserver=tls.createServer(options,common.mustCall((conn)=>{
+setTimeout(()=>conn.write('x'),50);
+}));
+
+server.listen(0,common.mustCall(()=>{
+constclient=tls.connect({
+port: server.address().port,
+rejectUnauthorized: false,
+},common.mustCall(()=>{
+client._parent=undefined;
+}));
+
+client.on('data',common.mustCall(()=>{
+server.close();
+client.destroy();
+}));
+
+client.on('error',common.mustNotCall());
+}));

lib/net.js

@@ -736,7 +736,9 @@ ObjectSetPrototypeOf(Socket, stream.Duplex);
 
 // Refresh existing timeouts.
 Socket.prototype._unrefTimer=function_unrefTimer(){
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 if(s[kTimeout])
 s[kTimeout].refresh();
 }
@@ -1097,7 +1099,9 @@ Socket.prototype._destroy = function(exception, cb) {
 
 this.connecting=false;
 
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 clearTimeout(s[kTimeout]);
 }
 

test/parallel/test-net-socket-unref-timer-parent-chain.js

@@ -0,0 +1,25 @@
+'use strict';
+constcommon=require('../common');
+
+// Walking the `_parent` chain must stop on a nullish link, not only strict
+// `null`. During connection teardown a socket's `_parent` can be left
+// `undefined`, which previously caused `_unrefTimer()` and `_destroy()` to read
+// a property off `undefined` and throw.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constassert=require('assert');
+constnet=require('net');
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket._unrefTimer();
+}
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket.on('error',common.mustNotCall());
+socket.destroy();
+assert.strictEqual(socket.destroyed,true);
+}

test/parallel/test-net-unref-timer-parent-undefined.js

@@ -0,0 +1,39 @@
+'use strict';
+
+constcommon=require('../common');
+
+if(!common.hasCrypto)
+common.skip('missing crypto');
+
+consttls=require('tls');
+constfixtures=require('../common/fixtures');
+
+// A TLS socket whose `_parent` is left `undefined` during teardown must not
+// crash when reads land on it (`onStreamRead` -> `_unrefTimer`) or when it is
+// destroyed (`_destroy`). Both walk the `_parent` chain.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constoptions={
+key: fixtures.readKey('agent1-key.pem'),
+cert: fixtures.readKey('agent1-cert.pem'),
+};
+
+constserver=tls.createServer(options,common.mustCall((conn)=>{
+setTimeout(()=>conn.write('x'),50);
+}));
+
+server.listen(0,common.mustCall(()=>{
+constclient=tls.connect({
+port: server.address().port,
+rejectUnauthorized: false,
+},common.mustCall(()=>{
+client._parent=undefined;
+}));
+
+client.on('data',common.mustCall(()=>{
+server.close();
+client.destroy();
+}));
+
+client.on('error',common.mustNotCall());
+}));

lib/net.js

@@ -736,7 +736,9 @@ ObjectSetPrototypeOf(Socket, stream.Duplex);
 
 // Refresh existing timeouts.
 Socket.prototype._unrefTimer=function_unrefTimer(){
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 if(s[kTimeout])
 s[kTimeout].refresh();
 }
@@ -1097,7 +1099,9 @@ Socket.prototype._destroy = function(exception, cb) {
 
 this.connecting=false;
 
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 clearTimeout(s[kTimeout]);
 }
 

test/parallel/test-net-socket-unref-timer-parent-chain.js

@@ -0,0 +1,25 @@
+'use strict';
+constcommon=require('../common');
+
+// Walking the `_parent` chain must stop on a nullish link, not only strict
+// `null`. During connection teardown a socket's `_parent` can be left
+// `undefined`, which previously caused `_unrefTimer()` and `_destroy()` to read
+// a property off `undefined` and throw.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constassert=require('assert');
+constnet=require('net');
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket._unrefTimer();
+}
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket.on('error',common.mustNotCall());
+socket.destroy();
+assert.strictEqual(socket.destroyed,true);
+}

test/parallel/test-net-unref-timer-parent-undefined.js

@@ -0,0 +1,39 @@
+'use strict';
+
+constcommon=require('../common');
+
+if(!common.hasCrypto)
+common.skip('missing crypto');
+
+consttls=require('tls');
+constfixtures=require('../common/fixtures');
+
+// A TLS socket whose `_parent` is left `undefined` during teardown must not
+// crash when reads land on it (`onStreamRead` -> `_unrefTimer`) or when it is
+// destroyed (`_destroy`). Both walk the `_parent` chain.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constoptions={
+key: fixtures.readKey('agent1-key.pem'),
+cert: fixtures.readKey('agent1-cert.pem'),
+};
+
+constserver=tls.createServer(options,common.mustCall((conn)=>{
+setTimeout(()=>conn.write('x'),50);
+}));
+
+server.listen(0,common.mustCall(()=>{
+constclient=tls.connect({
+port: server.address().port,
+rejectUnauthorized: false,
+},common.mustCall(()=>{
+client._parent=undefined;
+}));
+
+client.on('data',common.mustCall(()=>{
+server.close();
+client.destroy();
+}));
+
+client.on('error',common.mustNotCall());
+}));

lib/net.js

@@ -736,7 +736,9 @@ ObjectSetPrototypeOf(Socket, stream.Duplex);
 
 // Refresh existing timeouts.
 Socket.prototype._unrefTimer=function_unrefTimer(){
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 if(s[kTimeout])
 s[kTimeout].refresh();
 }
@@ -1097,7 +1099,9 @@ Socket.prototype._destroy = function(exception, cb) {
 
 this.connecting=false;
 
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 clearTimeout(s[kTimeout]);
 }
 

test/parallel/test-net-socket-unref-timer-parent-chain.js

@@ -0,0 +1,25 @@
+'use strict';
+constcommon=require('../common');
+
+// Walking the `_parent` chain must stop on a nullish link, not only strict
+// `null`. During connection teardown a socket's `_parent` can be left
+// `undefined`, which previously caused `_unrefTimer()` and `_destroy()` to read
+// a property off `undefined` and throw.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constassert=require('assert');
+constnet=require('net');
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket._unrefTimer();
+}
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket.on('error',common.mustNotCall());
+socket.destroy();
+assert.strictEqual(socket.destroyed,true);
+}

test/parallel/test-net-unref-timer-parent-undefined.js

@@ -0,0 +1,39 @@
+'use strict';
+
+constcommon=require('../common');
+
+if(!common.hasCrypto)
+common.skip('missing crypto');
+
+consttls=require('tls');
+constfixtures=require('../common/fixtures');
+
+// A TLS socket whose `_parent` is left `undefined` during teardown must not
+// crash when reads land on it (`onStreamRead` -> `_unrefTimer`) or when it is
+// destroyed (`_destroy`). Both walk the `_parent` chain.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constoptions={
+key: fixtures.readKey('agent1-key.pem'),
+cert: fixtures.readKey('agent1-cert.pem'),
+};
+
+constserver=tls.createServer(options,common.mustCall((conn)=>{
+setTimeout(()=>conn.write('x'),50);
+}));
+
+server.listen(0,common.mustCall(()=>{
+constclient=tls.connect({
+port: server.address().port,
+rejectUnauthorized: false,
+},common.mustCall(()=>{
+client._parent=undefined;
+}));
+
+client.on('data',common.mustCall(()=>{
+server.close();
+client.destroy();
+}));
+
+client.on('error',common.mustNotCall());
+}));

lib/net.js

@@ -736,7 +736,9 @@ ObjectSetPrototypeOf(Socket, stream.Duplex);
 
 // Refresh existing timeouts.
 Socket.prototype._unrefTimer=function_unrefTimer(){
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 if(s[kTimeout])
 s[kTimeout].refresh();
 }
@@ -1097,7 +1099,9 @@ Socket.prototype._destroy = function(exception, cb) {
 
 this.connecting=false;
 
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 clearTimeout(s[kTimeout]);
 }
 

test/parallel/test-net-socket-unref-timer-parent-chain.js

@@ -0,0 +1,25 @@
+'use strict';
+constcommon=require('../common');
+
+// Walking the `_parent` chain must stop on a nullish link, not only strict
+// `null`. During connection teardown a socket's `_parent` can be left
+// `undefined`, which previously caused `_unrefTimer()` and `_destroy()` to read
+// a property off `undefined` and throw.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constassert=require('assert');
+constnet=require('net');
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket._unrefTimer();
+}
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket.on('error',common.mustNotCall());
+socket.destroy();
+assert.strictEqual(socket.destroyed,true);
+}

test/parallel/test-net-unref-timer-parent-undefined.js

@@ -0,0 +1,39 @@
+'use strict';
+
+constcommon=require('../common');
+
+if(!common.hasCrypto)
+common.skip('missing crypto');
+
+consttls=require('tls');
+constfixtures=require('../common/fixtures');
+
+// A TLS socket whose `_parent` is left `undefined` during teardown must not
+// crash when reads land on it (`onStreamRead` -> `_unrefTimer`) or when it is
+// destroyed (`_destroy`). Both walk the `_parent` chain.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constoptions={
+key: fixtures.readKey('agent1-key.pem'),
+cert: fixtures.readKey('agent1-cert.pem'),
+};
+
+constserver=tls.createServer(options,common.mustCall((conn)=>{
+setTimeout(()=>conn.write('x'),50);
+}));
+
+server.listen(0,common.mustCall(()=>{
+constclient=tls.connect({
+port: server.address().port,
+rejectUnauthorized: false,
+},common.mustCall(()=>{
+client._parent=undefined;
+}));
+
+client.on('data',common.mustCall(()=>{
+server.close();
+client.destroy();
+}));
+
+client.on('error',common.mustNotCall());
+}));

lib/net.js

@@ -736,7 +736,9 @@ ObjectSetPrototypeOf(Socket, stream.Duplex);
 
 // Refresh existing timeouts.
 Socket.prototype._unrefTimer=function_unrefTimer(){
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 if(s[kTimeout])
 s[kTimeout].refresh();
 }
@@ -1097,7 +1099,9 @@ Socket.prototype._destroy = function(exception, cb) {
 
 this.connecting=false;
 
-for(lets=this;s!==null;s=s._parent){
+// `_parent` may be null; we use a loose `!= null` check in case external
+// code sets it to undefined.
+for(lets=this;s!=null;s=s._parent){
 clearTimeout(s[kTimeout]);
 }
 

test/parallel/test-net-socket-unref-timer-parent-chain.js

@@ -0,0 +1,25 @@
+'use strict';
+constcommon=require('../common');
+
+// Walking the `_parent` chain must stop on a nullish link, not only strict
+// `null`. During connection teardown a socket's `_parent` can be left
+// `undefined`, which previously caused `_unrefTimer()` and `_destroy()` to read
+// a property off `undefined` and throw.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constassert=require('assert');
+constnet=require('net');
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket._unrefTimer();
+}
+
+{
+constsocket=newnet.Socket();
+socket._parent=undefined;
+socket.on('error',common.mustNotCall());
+socket.destroy();
+assert.strictEqual(socket.destroyed,true);
+}

test/parallel/test-net-unref-timer-parent-undefined.js

@@ -0,0 +1,39 @@
+'use strict';
+
+constcommon=require('../common');
+
+if(!common.hasCrypto)
+common.skip('missing crypto');
+
+consttls=require('tls');
+constfixtures=require('../common/fixtures');
+
+// A TLS socket whose `_parent` is left `undefined` during teardown must not
+// crash when reads land on it (`onStreamRead` -> `_unrefTimer`) or when it is
+// destroyed (`_destroy`). Both walk the `_parent` chain.
+// Refs: https://github.com/nodejs/node/issues/64490
+
+constoptions={
+key: fixtures.readKey('agent1-key.pem'),
+cert: fixtures.readKey('agent1-cert.pem'),
+};
+
+constserver=tls.createServer(options,common.mustCall((conn)=>{
+setTimeout(()=>conn.write('x'),50);
+}));
+
+server.listen(0,common.mustCall(()=>{
+constclient=tls.connect({
+port: server.address().port,
+rejectUnauthorized: false,
+},common.mustCall(()=>{
+client._parent=undefined;
+}));
+
+client.on('data',common.mustCall(()=>{
+server.close();
+client.destroy();
+}));
+
+client.on('error',common.mustNotCall());
+}));