timers: fix arbitrary object clearImmediate errors #37824
Conversation
ea16354 to
f0f059f
Compare
|
Can you add regression tests for these? |
Done. I'm not 100% happy with the non-REPL test, as looking at the code even sending regular objects to clearImmediate looks like it has some side-effects which might corrupt some state, but it doesn't throw now. |
| proc.on('exit', common.mustCall((code) => { | ||
| assert.strictEqual(code, 0); | ||
| })); | ||
| proc.stdin.write('clearImmediate({});\n.exit\n'); |
There was a problem hiding this comment.
This test is not really required, since it was just caused by the crash but it does not hurt either.
There was a problem hiding this comment.
The issue, I think, was a bit different (if by "the crash" you mean that NRE for this._idleNext). The REPL crash was caused because an undefined async_id was given to emitDestroy, and this caused an error in emitDestroyScript (as the check there only checks <= 0 and undefined is not <= 0).
There was a problem hiding this comment.
Maybe a better fix would've been to actually change the code there (in async_hooks.js) to check for hasHooks(kDestroy) && asyncId > 0 instead of an early return - but that's probably out of scope for this minor issue, and maybe a crash there is better than "swallowing" bad async_ids which might cover errors elsewhere.
Fix errors that are caused by invoking clearImmediate with arbitrary objects. fixes: nodejs#37806 PR-URL: nodejs#37824 Fixes: nodejs#37806 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
f444985 to
fcc934f
Compare
|
Landed in fcc934f |
Fix errors that are caused by invoking clearImmediate with arbitrary objects. fixes: #37806 PR-URL: #37824 Fixes: #37806 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Fix errors that are caused by invoking clearImmediate with arbitrary objects. fixes: #37806 PR-URL: #37824 Fixes: #37806 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Fix errors that are caused by invoking clearImmediate with arbitrary objects. fixes: #37806 PR-URL: #37824 Fixes: #37806 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Fix errors that are caused by invoking clearImmediate with arbitrary objects. (e.g.
clearImmediate({}))timers.jsfixes the REPL crash.internal/timers.jsfixes another error that gets thrown.Both of the above already exist in the
clearTimeoutpath.Fixes: #37806