Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib: make safe primordials safe to iterate #36391

Closed
wants to merge 11 commits into from
Closed

lib: make safe primordials safe to iterate #36391

wants to merge 11 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 4, 2020 •
edited
Loading

MapIterator and SetIterator are user-mutable, I think it makes sense to be able to iterate over SafeMap and SafeSet without calling possibly-user-mutated code.

Note that patterns such as for (const [key, value] of safeMap) would still be potentially "unsafe" as it executes %ArrayIteratorPrototype%.next when destructuring, which may have been mutated in user-land.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 5, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 5, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 6, 2020
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 6, 2020

My original implementation didn't take into account that GeneratorFunction.prototype.next is user-mutable – and therefore wasn't any safer. I took another approach which is hopefully safe this time, but I might have missed something else, please review.

Note that I'm using ._iterator instead of .#iterator based on #36326 (comment); the SafeIterator class is internal-only anyway and should never be accessible to user-land, I don't think the actual visibility of the field matters at all. We could use a Symbol if anyone prefers.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 8, 2020

@addaleax This change affects code running during Worker termination, you may want to review this. I tried to be extra careful, but I may have missed something somewhere.

@addaleax
Copy link
Member

addaleax commented Dec 8, 2020

@addaleax This change affects code running during Worker termination, you may want to review this. I tried to be extra careful, but I may have missed something somewhere.

@aduh95 Thanks for the ping :) How does it affect that?

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 8, 2020

How does it affect that?

git blame points at you for this warning added in f17e414:

node/lib/internal/source_map/source_map_cache.js

Lines 183 to 186 in cf53748

// WARNING: The `sourceMapCacheToObject` and `appendCJSCache` run during
// shutdown. In particular, they also run when Workers are terminated, making
// it important that they do not call out to any user-provided code, including
// built-in prototypes that might have been tampered with.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2020
@Trott
Copy link
Member

Trott commented Dec 11, 2020

Can we add a test for the edge case this fixes?

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Dec 12, 2020
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 14, 2020

Can we add a test for the edge case this fixes?

@Trott Good call, the added test highlighted a bug which provides a way for workers to get out of worker.terminate().

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 15, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 15, 2020
@github-actions
Copy link
Contributor

Landed in 7c80817...0a5969c

@github-actions github-actions bot closed this Dec 15, 2020
nodejs-github-bot pushed a commit that referenced this pull request Dec 15, 2020
@aduh95 @nodejs-github-bot
lib: make safe primordials safe to iterate
0a5969c 
PR-URL: #36391
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@aduh95 aduh95 deleted the safe-primordials-iterable branch December 15, 2020 13:28
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 15, 2020

Backporting this to v12.x would be nice as it fixes a potential bug with the Worker implementation, but backporters would have to remove use of null coalescing operator.
I'm not sure for v10.x, but it's probably not worth it as the worker implementation is not stable anyway.

targos pushed a commit that referenced this pull request Dec 21, 2020
@aduh95 @targos
lib: make safe primordials safe to iterate
728f512 
PR-URL: #36391
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Dec 22, 2020
Merged
@targos
Copy link
Member

targos commented May 16, 2021

This lands cleanly on v14.x but then it fails to build.

@aduh95 aduh95 mentioned this pull request May 16, 2021
Closed
aduh95 added a commit to aduh95/node that referenced this pull request May 16, 2021
@aduh95
lib: make safe primordials safe to iterate
7f16442 
PR-URL: nodejs#36391
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 25, 2021
@aduh95 @targos
lib: make safe primordials safe to iterate
e108be0 
PR-URL: #36391
Backport-PR-URL: #38703
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jun 5, 2021
@aduh95 @targos
lib: make safe primordials safe to iterate
a07bc82 
PR-URL: #36391
Backport-PR-URL: #38703
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
@aduh95 @targos
lib: make safe primordials safe to iterate
7ebc18f 
PR-URL: #36391
Backport-PR-URL: #38703
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ExE-Boss ExE-Boss ExE-Boss left review comments

@addaleax addaleax addaleax left review comments

@jasnell jasnell jasnell approved these changes

@targos targos Awaiting requested review from targos

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@aduh95 @nodejs-github-bot @addaleax @Trott @targos @jasnell @ExE-Boss @richardlau