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.

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

fs: avoid out-of-bounds arguments index access #11115

Closed
wants to merge 1 commit into from

Conversation

tjhosford
Copy link

This updates fs to prevent accessing out-of-range indices on the arguments object, which is known to cause V8 optimization bailout.

Related to issues discussed here: #10323

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

This updates fs to prevent accessing out-of-range indices on the arguments object, which is known to cause V8 optimization bailout.

Related to issues discussed here: nodejs#10323
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 2, 2017
@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Feb 2, 2017
@Trott
Copy link
Member

Trott commented Feb 2, 2017

I changed dur from 5 to 1 in benchmark/fs/readfile.js (solely so that I could do 60 runs in 5 minutes rather than 25 minutes, as it's getting late over here) and then did this:

$ node benchmark/compare.js --new ./node-tomhoss --old ./node-master --filter readfile.js fs 1>out.csv 
[00:04:41|% 100| 1/1 files | 60/60 runs | 4/4 configs]: Done
$ cat out.csv | Rscript benchmark/compare.R 
                                                 improvement confidence   p.value
 fs/readfile.js concurrent=1 len=1024 dur=1           1.32 %            0.1169608
 fs/readfile.js concurrent=1 len=16777216 dur=1      -0.12 %            0.6766730
 fs/readfile.js concurrent=10 len=1024 dur=1         -0.28 %            0.7146591
 fs/readfile.js concurrent=10 len=16777216 dur=1     -0.13 %            0.8874545
$

The results don't raise any concerns. However, someone should probably run a more comprehensive benchmark comparison. (For example, I didn't do writes here.)

@jasnell
Copy link
Member

jasnell commented Feb 2, 2017

LGTM but I'd like @trevnorris or @addaleax to take a look also

@addaleax
Copy link
Member

addaleax commented Feb 2, 2017

I don’t think any of these functions would actually ever be called without arguments; and when they are, they already have the overhead of failing very loudly anyways…

@vsemozhetbyt
Copy link
Contributor

@tomhoss , @Trott , @jasnell , @addaleax
So, maybe, this could be safely closed without merging?

@jasnell
Copy link
Member

jasnell commented Feb 8, 2017

Closing without action. Can reopen later if necessary

@jasnell jasnell closed this Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants