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

Fix segmentation fault when using setters or getters that are not defined #148

Closed
wants to merge 1 commit into from
Closed

Fix segmentation fault when using setters or getters that are not defined #148

wants to merge 1 commit into from

Conversation

rolftimmermans
Copy link
Contributor

Currently the following scenario aborts with a segmentation fault:

exports.Set("Text", DefineClass(env, "Test", {
    InstanceAccessor("readonly_prop", &Test::GetReadonlyProp, nullptr),
}));
new Test().readonly_prop = 1;

This PR fixes that scenario by checking for a null getter/setter and setting the property descriptor accordingly. Includes a test case for readonly properties.

I had to modify the test runner to correctly report SIGSEGV in order for it to actually fail the tests.

@mhdawson
Copy link
Member

mhdawson commented Oct 3, 2017

Sorry I've not had a chance to check this out yet. I'm out at Node Interactive this week but will try to get to it when I have spare time.

@mhdawson
Copy link
Member

Seems like this PR #159 is fixing the same/similar problem. That PR does not seem to have the change you needed to make sure the test failed. Can you take a look at that PR and then potentially revise this one to have what would still be needed after that ?

@rolftimmermans
Copy link
Contributor Author

I fixed the conflicts, added a write only test and fixed a segfault that was caused by a missing SuppressDestruct.

If the other PR is merged we can still update the segfault logic and fix of course.

@mhdawson
Copy link
Member

Ok, my plan is to land the other PR, ask you to rebase and then I can go ahead and land this one. Sorry for taking so long to review this one....

@mhdawson
Copy link
Member

Ok other PR has landed, if you can rebase that would be great.

@rolftimmermans
Copy link
Contributor Author

👍 done

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

mhdawson pushed a commit that referenced this pull request Oct 25, 2017
Fail test suite if child process receives a signal.
Fixes segfault in test.

PR-URL: #148
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@mhdawson
Copy link
Member

Landed as 22be609

@mhdawson mhdawson closed this Oct 25, 2017
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
Fail test suite if child process receives a signal.
Fixes segfault in test.

PR-URL: nodejs/node-addon-api#148
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
Fail test suite if child process receives a signal.
Fixes segfault in test.

PR-URL: nodejs/node-addon-api#148
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
Fail test suite if child process receives a signal.
Fixes segfault in test.

PR-URL: nodejs/node-addon-api#148
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
Fail test suite if child process receives a signal.
Fixes segfault in test.

PR-URL: nodejs/node-addon-api#148
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants