FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

timers: fix arbitrary object clearImmediate errors by Linkgoron · Pull Request #37824 · nodejs/node · GitHub

/ node Public

timers: fix arbitrary object clearImmediate errors - #37824

Merged
aduh95 merged 1 commit into
nodejs:masterfrom
Linkgoron:timers-fix-null-check
Mar 28, 2021
ConversationCommits (1)ChecksFiles changed
Merged

timers: fix arbitrary object clearImmediate errors#37824
aduh95 merged 1 commit into
nodejs:masterfrom
Linkgoron:timers-fix-null-check

Conversation

Linkgoron commented Mar 19, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fix errors that are caused by invoking clearImmediate with arbitrary objects. (e.g. clearImmediate({}))

  • Fix in timers.js fixes the REPL crash.
  • Fix in internal/timers.js fixes another error that gets thrown.

Both of the above already exist in the clearTimeout path.

Fixes: #37806

nodejs-github-bot added needs-ci PRs that need a full CI run. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Mar 19, 2021
Linkgoron force-pushed the timers-fix-null-check branch from ea16354 to f0f059f Compare March 20, 2021 00:00

nodejs-github-bot commented Mar 20, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36833/

addaleax commented Mar 20, 2021

Copy link
Copy Markdown
Member

Can you add regression tests for these?

Linkgoron commented Mar 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

nodejs-github-bot commented Mar 20, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36835/

BridgeAR reviewed Mar 20, 2021
View reviewed changes
Comment thread test/parallel/test-repl-clear-immediate-crash.js Outdated

BridgeAR Mar 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This test is not really required, since it was just caused by the crash but it does not hurt either.

Linkgoron Mar 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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).

Linkgoron Mar 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

aduh95 reviewed Mar 20, 2021
View reviewed changes
Comment thread lib/internal/timers.js Outdated
Linkgoron added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 20, 2021

nodejs-github-bot commented Mar 22, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36911/

nodejs-github-bot commented Mar 22, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36917/

nodejs-github-bot commented Mar 23, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36923/

timers: fix arbitrary object clearImmediate errors
fcc934f 
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>
aduh95 force-pushed the timers-fix-null-check branch from f444985 to fcc934f Compare March 28, 2021 17:21
aduh95 merged commit fcc934f into nodejs:master Mar 28, 2021

aduh95 commented Mar 28, 2021

Copy link
Copy Markdown
Contributor

Landed in fcc934f

ruyadorno pushed a commit that referenced this pull request Mar 29, 2021
timers: fix arbitrary object clearImmediate errors
28f59e5 
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>
ruyadorno pushed a commit that referenced this pull request Mar 30, 2021
timers: fix arbitrary object clearImmediate errors
d44b268 
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>
ruyadorno mentioned this pull request Mar 30, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
timers: fix arbitrary object clearImmediate errors
e4d1042 
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>
danielleadams mentioned this pull request May 3, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BridgeAR BridgeAR approved these changes
Trott Trott approved these changes
benjamingr benjamingr approved these changes
aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

"clearImmediate" results in an abort

7 participants

Back | FazBrowse Home | New Git URL