net: handle undefined parent in _unrefTimer and _destroy#64644
Conversation
|
Review requested:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64644 +/- ##
==========================================
- Coverage 92.01% 90.14% -1.87%
==========================================
Files 381 741 +360
Lines 170167 242137 +71970
Branches 26075 45568 +19493
==========================================
+ Hits 156585 218286 +61701
- Misses 13291 15368 +2077
- Partials 291 8483 +8192
🚀 New features to boost your workflow:
|
6b91d30 to
ae8a326
Compare
|
Cc. @aduh95 who was active in the original PR. Note: I've never contributed to Node and basically do not know what I am doing. |
Thanks for the PR @haggholm, no worries, this looks good to me and all going in the right direction. Very helpful picking up stalled work like this. I have a minor comment here that it'd be nice to clean up (you can just push another commit and they'll be squashed together at merge automatically) but otherwise everything looks great. Don't worry too much about the test-internet failure, I strongly suspect something flaky is going on there... Could be us, could be github actions, hard to say. |
The fix and approach are from nodejs#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: nodejs#64490 Refs: nodejs#64491 Signed-off-by: Petter Häggholm <petter@hightouch.io>
ae8a326 to
0fff075
Compare
More specifically, I see now it's been broken on main but will be fixed by #64624 imminently, we'll just need to wait until that's merged then rebase this. |
The fix and approach are from #64491 by @Shivay-98; opening from a fresh branch to land it, since that PR stalled awaiting the requested changes (unrelated commit dropped, failing test addressed,
Signed-off-byadded).Socket.prototype._unrefTimerandSocket.prototype._destroyboth walk the socket_parentchain with a strict!== nullcheck. When teardown leaves a socket's_parentasundefined(for example a TLS socket layered over another stream), the loop steps ontoundefinedand throwsfrom an uncaught I/O callback, crashing the process. Switching both loops to a nullish
!= nullcheck stops the walk onundefinedas well.The original PR fixed only
_unrefTimer; its regression test also callsdestroy(), which hits the same unpatched loop in_destroy, so that path is fixed and covered here too.Fixes: #64490
Refs: #64491
Checklist
make -j4 test(UNIX) passes