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

src: avoid crash using uv.errname() binding #44401

Closed
wants to merge 1 commit into from

Conversation

juanarbol
Copy link
Member

Return undefined from uv binding when no args are provided and do the libuv
call with any arg (if provided) to the binding.

Fixes: #44400

This crash is being provoked by this assertion, using the binding directly is not guarantee that it will be invoked with the expected signature.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. libuv Issues and PRs related to the libuv dependency or the uv binding. needs-ci PRs that need a full CI run. labels Aug 25, 2022
Return undefined from uv binding when no args are provided and do the
libuv call with any arg (if provided) to the binding.

Fixes: nodejs#44400
@juanarbol juanarbol added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 25, 2022
@github-actions
Copy link
Contributor

Fast-track has been requested by @juanarbol. Please 👍 to approve.

@theanarkh
Copy link
Contributor

Do we need to check err < 0 ?

@juanarbol
Copy link
Member Author

Do we need to check err < 0 ?

Nope, that's why I removed that assertion and return undefined with no args

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

The added test is failing on asan builds because of a memory leak from https://github.com/libuv/libuv/blob/fb76f210eb6f093bc06a2f07646e56851818ccf2/src/uv-common.c#L170.

@juanarbol juanarbol removed the fast-track PRs that do not need to wait for 48 hours to land. label Aug 26, 2022
@theanarkh theanarkh mentioned this pull request Aug 27, 2022
4 tasks
src/uv.cc Show resolved Hide resolved
@juanarbol
Copy link
Member Author

I will close this; at this moment, it does not make any sense in the runtime, and #44421 made something better <3

@juanarbol juanarbol closed this Aug 29, 2022
nodejs-github-bot pushed a commit that referenced this pull request Aug 30, 2022
PR-URL: #44421
Refs: #44401
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Sep 5, 2022
PR-URL: #44421
Refs: #44401
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Sep 6, 2022
PR-URL: #44421
Refs: #44401
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Sep 7, 2022
PR-URL: #44421
Refs: #44401
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
PR-URL: nodejs#44421
Refs: nodejs#44401
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
targos pushed a commit that referenced this pull request Sep 16, 2022
PR-URL: #44421
Refs: #44401
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 10, 2022
PR-URL: #44421
Refs: #44401
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 11, 2022
PR-URL: #44421
Refs: #44401
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@juanarbol juanarbol deleted the juan/fix-uv-crash branch October 24, 2022 13:48
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
PR-URL: nodejs/node#44421
Refs: nodejs/node#44401
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
PR-URL: nodejs/node#44421
Refs: nodejs/node#44401
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
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++. libuv Issues and PRs related to the libuv dependency or the uv binding. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node.js crashes using process.binding('uv').errname(positiveNumberOrNullValue)
5 participants