FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

node-api,test: fix test_reference_double_free crash by vmoroz · Pull Request #44927 · nodejs/node · GitHub

/ node Public

node-api,test: fix test_reference_double_free crash - #44927

Closed
vmoroz wants to merge 2 commits into
nodejs:mainfrom
vmoroz:PR/Fix_test_reference_double_free
ConversationCommits (2)ChecksFiles changed
Closed

node-api,test: fix test_reference_double_free crash#44927
vmoroz wants to merge 2 commits into
nodejs:mainfrom
vmoroz:PR/Fix_test_reference_double_free

Conversation

vmoroz commented Oct 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

The issue

The js-native-api\test_reference_double_free\test_wrap.js test always crashes when I use Visual Studio 2022.
The cause of the crash is that the deleteImmediately function callback is returning an invalid napi_value.
It happens because the DeleteImmediately function that implements the deleteImmediately callback returns void instead of napi_value. It causes it to return a random value kept in RAX register - in my case it was 0x40.

The fix

The return type of the DeleteImmediately function is changed to napi_value.
The related changes:

  • Changed use of macro NODE_API_CALL_RETURN_VOID to NODE_API_CALL because we changed the return type.
  • Added NODE_API_ASSERT to verify argument type - previously we retrieved the type, but never used it.
  • Return NULL from the function.

Notes

I did a quick search for void.*napi_callback_info in other tests to see if we have any other cases like this, but it found nothing.
It seems that it is the only place where we have this issue.

nodejs-github-bot added needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Oct 8, 2022
legendecas approved these changes Oct 9, 2022
View reviewed changes

legendecas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

👍

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2022

nodejs-github-bot commented Oct 9, 2022

Copy link
Copy Markdown
Collaborator

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

legendecas commented Oct 10, 2022

Copy link
Copy Markdown
Member

@nodejs/node-api please take a look, thank you

mhdawson approved these changes Oct 14, 2022
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2022

nodejs-github-bot commented Oct 14, 2022

Copy link
Copy Markdown
Collaborator

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

legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 18, 2022
nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 18, 2022

nodejs-github-bot commented Oct 18, 2022

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/44927
✔  Done loading data for nodejs/node/pull/44927
----------------------------------- PR info ------------------------------------
Title      node-api,test: fix test_reference_double_free crash (#44927)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     vmoroz:PR/Fix_test_reference_double_free -> nodejs:main
Labels     test, node-api, needs-ci
Commits    2
 - node-api,test: fix test_reference_double_free crash
 - return NULL instead of undefined
Committers 1
 - Vladimir Morozov 
PR-URL: https://github.com/nodejs/node/pull/44927
Reviewed-By: Chengzhong Wu 
Reviewed-By: Michael Dawson 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44927
Reviewed-By: Chengzhong Wu 
Reviewed-By: Michael Dawson 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 08 Oct 2022 20:39:41 GMT
   ✔  Approvals: 2
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/44927#pullrequestreview-1135308312
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/44927#pullrequestreview-1142647651
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-10-14T15:54:17Z: https://ci.nodejs.org/job/node-test-pull-request/47273/
- Querying data for job/node-test-pull-request/47273/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 44927
From https://github.com/nodejs/node
 * branch                  refs/pull/44927/merge -> FETCH_HEAD
✔  Fetched commits as 1a8f8ba6c6ca..ba3a1e5b291f
--------------------------------------------------------------------------------
[main db8a6f99c7] node-api,test: fix test_reference_double_free crash
 Author: Vladimir Morozov 
 Date: Sat Oct 8 13:17:01 2022 -0700
 1 file changed, 12 insertions(+), 9 deletions(-)
[main 2ed8da8b5d] return NULL instead of undefined
 Author: Vladimir Morozov 
 Date: Sat Oct 8 21:37:30 2022 -0700
 1 file changed, 1 insertion(+), 3 deletions(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

node-api,test: fix test_reference_double_free crash


PR-URL: #44927

Reviewed-By: Chengzhong Wu legendecas@gmail.com

Reviewed-By: Michael Dawson midawson@redhat.com


[detached HEAD b786d0193c] node-api,test: fix test_reference_double_free crash

Author: Vladimir Morozov vmoroz@users.noreply.github.com

Date: Sat Oct 8 13:17:01 2022 -0700

1 file changed, 12 insertions(+), 9 deletions(-)

Rebasing (3/4)

Rebasing (4/4)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

return NULL instead of undefined


PR-URL: #44927

Reviewed-By: Chengzhong Wu legendecas@gmail.com

Reviewed-By: Michael Dawson midawson@redhat.com


[detached HEAD 50f50832bb] return NULL instead of undefined

Author: Vladimir Morozov vmoroz@users.noreply.github.com

Date: Sat Oct 8 21:37:30 2022 -0700

1 file changed, 1 insertion(+), 3 deletions(-)


Successfully rebased and updated refs/heads/main.


ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.
https://github.com/nodejs/node/actions/runs/3272013058

vmoroz commented Oct 18, 2022

Copy link
Copy Markdown
Member Author

Commit Queue failed

I had changed my profile to make the primary e-mail public. I hope it should address the issue.

legendecas pushed a commit that referenced this pull request Oct 19, 2022
node-api,test: fix test_reference_double_free crash
5b316fe 
PR-URL: #44927
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

legendecas commented Oct 19, 2022

Copy link
Copy Markdown
Member

Landed in 5b316fe. Thank you for the work on this!

legendecas closed this Oct 19, 2022
vmoroz deleted the PR/Fix_test_reference_double_free branch October 19, 2022 00:15

vmoroz commented Oct 19, 2022

Copy link
Copy Markdown
Member Author

Landed in 5b316fe. Thank you for the work on this!

Thank you for landing it!

RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
node-api,test: fix test_reference_double_free crash
21ea55c 
PR-URL: #44927
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS mentioned this pull request Nov 1, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
node-api,test: fix test_reference_double_free crash
2105f09 
PR-URL: #44927
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
node-api,test: fix test_reference_double_free crash
3f454fa 
PR-URL: #44927
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
node-api,test: fix test_reference_double_free crash
b5811c4 
PR-URL: #44927
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
legendecas legendecas approved these changes
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

commit-queue-failed An error occurred while landing this pull request using GitHub Actions. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL