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

test: add known issue test for child_process.spawnSync segfault #8543

Closed
wants to merge 1 commit into from

Conversation

imyller
Copy link
Member

@imyller imyller commented Sep 15, 2016

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

test

Description of change

child_process.spawnSync segfaults when given an array/object for the file argument with a throwing toString definition.

This PR adds a known_issues test with expected crash result.

Ref: #8539

child_process.spawnSync segfaults when given an array/object
for the file argument with a throwing toString definition.

Refs: nodejs#8539

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 15, 2016
@bnoordhuis
Copy link
Member

I don't see the point. You can crash node.js through almost any native function that accepts non-primitive values, see #7902; it's certainly not exclusive to spawnSync().

@imyller
Copy link
Member Author

imyller commented Sep 15, 2016

@bnoordhuis Agreed when that is the case.

From end-user standpoint issues #8537, #8538 and #8539 fall in to same category and should be explained and resolved gracefully.

I opened this PR to match #8539 to allow easier testing if per-case solution for core lib segfaults is later added.

@bnoordhuis
Copy link
Member

From end-user standpoint issues #8537, #8538 and #8539 fall in to same category and should be explained and resolved gracefully.

#7902 (comment) is the explanation including how to fix it. But as I said there, changing everything to v8::MaybeLocal<T> is not going to happen overnight; it's on my for-when-I-have-a-spare-week list.

@imyller
Copy link
Member Author

imyller commented Sep 15, 2016

@bnoordhuis Thank you for the explanation.

I'll close this PR as it does not add any additional value to the already well known issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants