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

<=v4: process.hrtime() segfaults on arrays with error-throwing accessors #7902

Closed
bengl opened this issue Jul 27, 2016 · 3 comments
Closed
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem.

Comments

@bengl
Copy link
Member

bengl commented Jul 27, 2016

In latest node v4.x and below, when an array with an accessor that throws is passed into process.hrtime(), it will produce a segmentation fault.

Example

const badArray = []
Object.defineProperty(badArray, 0, {
  get: () => {
    throw Error('I am a bad accessor')
  }
})
process.hrtime(badArray)

On node v5.x and v6.x this will just throw, which is expected. On node v4.x and below, this produces a segmentation fault.

I think this was fixed on v5.x+ by this commit fc143da.

Though this particular case is really edgy, it's my understanding that node APIs probably shouldn't segfault ever.

/cc @mlfbrown @deian who found this.

@bengl bengl changed the title on v4 and below, process.hrtime() segfaults on arrays with error-throwing accessors <=v4: process.hrtime() segfaults on arrays with error-throwing accessors Jul 27, 2016
@mscdex mscdex added the process Issues and PRs related to the process subsystem. label Jul 28, 2016
@bnoordhuis
Copy link
Member

bnoordhuis commented Jul 28, 2016

process.hrtime() is certainly not alone. Almost any C++ API function that accepts arrays or objects is vulnerable, e.g.:

$ ./out/Release/node -e '
  let a = [];
  Object.defineProperty(a, 0, {get() { throw 42 }});
  process.setgroups(a);
'
Segmentation fault (core dumped)

EDIT: Moving everything over to v8::Maybe and v8::MaybeLocal should fix most of them but that won't happen overnight.

@ChALkeR ChALkeR added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 28, 2016
@imyller
Copy link
Member

imyller commented Aug 2, 2016

Is a PR for known_issues test for this segfault necessary?

@addaleax
Copy link
Member

I think this has been addressed in #10764, feel free to re-open if I’m mistaken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants