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

lib: make safe primordials safe to iterate by aduh95 · Pull Request #36391 · nodejs/node · GitHub

/ node Public

lib: make safe primordials safe to iterate - #36391

Closed
aduh95 wants to merge 11 commits into
nodejs:masterfrom
aduh95:safe-primordials-iterable
ConversationCommits (11)ChecksFiles changed
Closed

lib: make safe primordials safe to iterate#36391
aduh95 wants to merge 11 commits into
nodejs:masterfrom
aduh95:safe-primordials-iterable

Conversation

aduh95 commented Dec 4, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 5, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 5, 2020

This comment has been minimized.

Sign in to view
aduh95 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 6, 2020

aduh95 commented Dec 6, 2020

Copy link
Copy Markdown
Contributor Author

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 requested review from addaleax and targos December 6, 2020 11:26

aduh95 commented Dec 8, 2020

Copy link
Copy Markdown
Contributor Author

@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 commented Dec 8, 2020

Copy link
Copy Markdown
Member

@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 commented Dec 8, 2020

Copy link
Copy Markdown
Contributor Author

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.

addaleax reviewed Dec 8, 2020
View reviewed changes
Comment thread lib/internal/source_map/source_map_cache.js Outdated
Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2020

Trott commented Dec 11, 2020

Copy link
Copy Markdown
Member

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

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

This comment has been minimized.

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

This comment has been minimized.

Sign in to view
aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. review wanted PRs that need reviews. labels Dec 12, 2020

aduh95 commented Dec 14, 2020

Copy link
Copy Markdown
Contributor Author

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 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 15, 2020
github-actions Bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 15, 2020

github-actions Bot commented Dec 15, 2020

Copy link
Copy Markdown
Contributor

Landed in 7c80817...0a5969c

github-actions Bot closed this Dec 15, 2020
nodejs-github-bot pushed a commit that referenced this pull request Dec 15, 2020
lib: make safe primordials safe to iterate
0a5969c 
PR-URL: #36391
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 deleted the safe-primordials-iterable branch December 15, 2020 13:28

aduh95 commented Dec 15, 2020

Copy link
Copy Markdown
Contributor Author

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
lib: make safe primordials safe to iterate
728f512 
PR-URL: #36391
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos mentioned this pull request Dec 22, 2020
Merged

targos commented May 16, 2021

Copy link
Copy Markdown
Member

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

aduh95 mentioned this pull request May 16, 2021
Closed
aduh95 added a commit to aduh95/node that referenced this pull request May 16, 2021
lib: make safe primordials safe to iterate
7f16442 
PR-URL: nodejs#36391
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 25, 2021
lib: make safe primordials safe to iterate
e108be0 
PR-URL: #36391
Backport-PR-URL: #38703
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
lib: make safe primordials safe to iterate
a07bc82 
PR-URL: #36391
Backport-PR-URL: #38703
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
lib: make safe primordials safe to iterate
7ebc18f 
PR-URL: #36391
Backport-PR-URL: #38703
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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

addaleax addaleax left review comments
jasnell jasnell approved these changes
targos Awaiting requested review from targos
+1 more reviewer
ExE-Boss ExE-Boss left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL