GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — http2: refactor to avoid unsafe array iteration by aduh95 · Pull Request #36700 · nodejs/node

Conversation

@aduh95

@aduh95 aduh95 commented Dec 31, 2020

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji tniessen reacted with confused emoji
All reactions
  • 👍 1 reaction
  • 😕 1 reaction
@aduh95 aduh95 added http2 Issues and PRs related to the http2 subsystem. needs-benchmark-ci PRs that need a benchmark CI run. labels Dec 31, 2020
@PoojaDurgad PoojaDurgad added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 31, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 31, 2020
@RaisinTen RaisinTen mentioned this pull request Dec 31, 2020
request-ci not working #36708
Closed
jasnell
jasnell approved these changes Dec 31, 2020
View reviewed changes
rchougule
rchougule reviewed Jan 1, 2021
View reviewed changes
Comment thread lib/internal/http2/core.js Outdated

@rchougule rchougule Jan 1, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

for..of loop has higher performance as compared to forEach (correct me if I am wrong)...
So are we still fine going with forEach for this specific case here?

P.S. Just started with open-source reviews and was trying to learn from other PRs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Trott Trott Jan 2, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, for..of is more performant which is why it was chosen to begin with. It's also why this pull request will need a benchmark run before it can be merged. The problem with for..of is that it is vulnerable to prototype pollution attacks. If some third party module deep in your dependencies decides to inject some code in Array.prototype[Symbol.iterator], then for..of will happily run that code. That may or may not be a problem for the end user's code--there are legitimate use cases (like profiling) for this sort of so-called "monkeypatching", but we don't want Node.js core to run the altered code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rchougule reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@aduh95

This comment has been minimized.

Sign in to view
Trott
Trott approved these changes Jan 2, 2021
View reviewed changes

@Trott Trott left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM if benchmarks are OK

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@aduh95

This comment has been minimized.

Sign in to view
@aduh95

This comment has been minimized.

Sign in to view
tniessen
tniessen reviewed Jan 3, 2021
View reviewed changes
Comment thread lib/internal/http2/core.js Outdated

@tniessen tniessen Jan 3, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Isn't this also "vulnerable" to prototype pollution "attacks," if we really want to consider that "unsafe?" I mean, one could define a getter on the Array.prototype that returns something "malicious."

I'm really not sure if all this complexity and illegibility are worth the minimal benefit.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig and szmarczak reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@aduh95 aduh95 Jan 3, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Defining a 0 property on the Array.prototype would have effect only on empty arrays, right? args here is created by the spread operator, so I think this code would actually be safe even in case of Array.prototype pollution.

Object.defineProperty(Array.prototype, '0', {get(){return 9}, set(v){/* no op */}})
((t, ...args)=>args[0])(1, 2, 3) === 2 // true

I don't think we are concerned about code being "malicious" anyway, what we are trying to achieve with the move to primordials is to allow users to monkey-patch the built-in objects (for debugging purposes, for tinkering, etc.) without having errors thrown by Node.js internals.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tniessen tniessen Jan 5, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

so I think this code would actually be safe even in case of Array.prototype pollution.

Might be, I am not sure.

without having errors thrown by Node.js internals.

I understand the motivation, but I am not sure it's worth the cost to maintainability, readability etc.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

szmarczak reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@aduh95 aduh95 Jan 5, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

To address the maintainability concern, would you prefer if we added a linter rule to forbid array destructuring assignment?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@aduh95
aduh95 force-pushed the htt2-array-iteration branch from bf0fb18 to f261d70 Compare January 11, 2021 08:55
@aduh95

aduh95 commented Jan 11, 2021

Copy link
Copy Markdown
Contributor Author

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/887/ (queued)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ZYSzys

ZYSzys commented Jan 12, 2021

Copy link
Copy Markdown
Member

Benchmark CI: ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/887 (queued)

I'm a bit of -1 since benchmarks showed some regression.

21:22:36  http2/headers.js nheaders=0 n=1000                                                                              **     -3.60 %       ±2.48%  ±3.30%  ±4.30%
21:22:36  http2/headers.js nheaders=1000 n=1000                                                                            *     -2.02 %       ±1.59%  ±2.12%  ±2.76%
21:22:36  http2/headers.js nheaders=100 n=1000                                                                             *      4.27 %       ±3.56%  ±4.74%  ±6.18%
21:22:36  http2/headers.js nheaders=10 n=1000                                                                            ***     -5.31 %       ±2.11%  ±2.81%  ±3.66%
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Jan 12, 2021

Copy link
Copy Markdown
Contributor Author

Another benchmark run confirmed there's a perf regression for low values of nheaders, investigating…

                                       confidence improvement accuracy (*)   (**)  (***)
 http2/headers.js nheaders=0 n=1000           ***     -4.71 %       ±2.51% ±3.33% ±4.31%
 http2/headers.js nheaders=1000 n=1000                -1.72 %       ±2.24% ±2.97% ±3.84%
 http2/headers.js nheaders=100 n=1000                  1.01 %       ±3.03% ±4.01% ±5.18%
 http2/headers.js nheaders=10 n=1000          ***     -5.58 %       ±1.71% ±2.27% ±2.93%
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Jan 12, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Replacing <array>.forEach with for(;;) loop fixed the perf regressions. If no one objects, I'm planning to land this later this week, I think the perf looks OK now.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/889

                                       confidence improvement accuracy (*)   (**)  (***)
 http2/headers.js nheaders=0 n=1000                   -0.43 %       ±2.00% ±2.65% ±3.42%
 http2/headers.js nheaders=1000 n=1000        ***     -3.28 %       ±1.87% ±2.48% ±3.20%
 http2/headers.js nheaders=100 n=1000                  0.02 %       ±2.95% ±3.91% ±5.05%
 http2/headers.js nheaders=10 n=1000           **     -2.32 %       ±1.71% ±2.26% ±2.92%
rchougule reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Jan 12, 2021
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 12, 2021
@nodejs-github-bot

nodejs-github-bot commented Jan 12, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95 aduh95 removed the needs-benchmark-ci PRs that need a benchmark CI run. label Jan 12, 2021
@nodejs-github-bot

nodejs-github-bot commented Jan 12, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95
http2: refactor to avoid unsafe array iteration
0fd9bbb
PR-URL: nodejs#36700
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@aduh95
aduh95 force-pushed the htt2-array-iteration branch from 5cdcc12 to 0fd9bbb Compare January 15, 2021 18:24
@aduh95

aduh95 commented Jan 15, 2021

Copy link
Copy Markdown
Contributor Author

Landed in 0fd9bbb

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95
aduh95 merged commit 0fd9bbb into nodejs:master Jan 15, 2021
@aduh95
aduh95 deleted the htt2-array-iteration branch January 15, 2021 18:24
ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
@aduh95 @ruyadorno
http2: refactor to avoid unsafe array iteration
f5b8e7b
PR-URL: #36700
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Jan 22, 2021
v15.7.0 proposal #37020
Merged
@targos targos added the dont-land-on-v14.x label May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessen tniessen tniessen left review comments
@jasnell jasnell jasnell approved these changes
@Trott Trott Trott approved these changes
+1 more reviewer
@rchougule rchougule rchougule left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. http2 Issues and PRs related to the http2 subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@aduh95 @ZYSzys @nodejs-github-bot @jasnell @Trott @tniessen @rchougule @targos @PoojaDurgad
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.