Skip to content

timers: fix arbitrary object clearImmediate errors - #37824

Merged
aduh95 merged 1 commit into
nodejs:masterfrom
Linkgoron:timers-fix-null-check
Mar 28, 2021
Merged

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

Conversation

@Linkgoron

@LinkgoronLinkgoron 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-botnodejs-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
Linkgoronforce-pushed the timers-fix-null-check branch from ea16354 to f0f059fCompareMarch 20, 2021 00:00
@nodejs-github-bot

nodejs-github-bot commented Mar 20, 2021

Copy link
Copy Markdown
Collaborator

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

@addaleax

addaleax commented Mar 20, 2021

Copy link
Copy Markdown
Member

Can you add regression tests for these?

@Linkgoron

Linkgoron commented Mar 20, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

nodejs-github-bot commented Mar 20, 2021

Copy link
Copy Markdown
Collaborator

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

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

@BridgeARBridgeARMar 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.

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

@LinkgoronLinkgoronMar 20, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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 undefinedasync_id was given to emitDestroy, and this caused an error in emitDestroyScript (as the check there only checks <= 0 and undefined is not <= 0).

@LinkgoronLinkgoronMar 20, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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
aduh95 reviewed Mar 20, 2021
View reviewed changes
Comment threadlib/internal/timers.js Outdated
@LinkgoronLinkgoron added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 20, 2021
@nodejs-github-bot

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

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

nodejs-github-bot commented Mar 23, 2021

Copy link
Copy Markdown
Collaborator

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

@Linkgoron@aduh95
timers: fix arbitrary object clearImmediate errors
fcc934f
Fix errors that are caused by invoking clearImmediate
with arbitrary objects.
fixes: nodejs#37806
PR-URL: nodejs#37824Fixes: 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
aduh95force-pushed the timers-fix-null-check branch from f444985 to fcc934fCompareMarch 28, 2021 17:21
@aduh95
aduh95 merged commit fcc934f into nodejs:masterMar 28, 2021
@aduh95

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
@Linkgoron@ruyadorno
timers: fix arbitrary object clearImmediate errors
28f59e5
Fix errors that are caused by invoking clearImmediate
with arbitrary objects.
fixes: #37806
PR-URL: #37824Fixes: #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
@Linkgoron@ruyadorno
timers: fix arbitrary object clearImmediate errors
d44b268
Fix errors that are caused by invoking clearImmediate
with arbitrary objects.
fixes: #37806
PR-URL: #37824Fixes: #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>
@ruyadornoruyadorno mentioned this pull request Mar 30, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@Linkgoron@targos
timers: fix arbitrary object clearImmediate errors
e4d1042
Fix errors that are caused by invoking clearImmediate
with arbitrary objects.
fixes: #37806
PR-URL: #37824Fixes: #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>
@danielleadamsdanielleadams mentioned this pull request May 3, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeARBridgeARBridgeAR approved these changes
@TrottTrottTrott approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
@aduh95aduh95aduh95 approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.timersIssues 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

@Linkgoron@nodejs-github-bot@addaleax@aduh95@Trott@benjamingr@BridgeAR