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.

Sign up for GitHub

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

Jump to bottom

node-api: make napi_get_buffer_info check if passed buffer is valid #51571

Merged
merged 1 commit into from
Feb 23, 2024
Merged

node-api: make napi_get_buffer_info check if passed buffer is valid #51571

merged 1 commit into from
Feb 23, 2024

Conversation

Janrupf
Copy link
Contributor

@Janrupf Janrupf commented Jan 26, 2024

Fixes #51570

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Jan 26, 2024
mhdawson
mhdawson reviewed Jan 26, 2024
View reviewed changes
src/node_api.cc Outdated Show resolved Hide resolved
@Janrupf Janrupf marked this pull request as ready for review January 26, 2024 19:17
@Janrupf Janrupf requested a review from mhdawson January 26, 2024 19:18
mhdawson
mhdawson approved these changes Jan 29, 2024
View reviewed changes
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 mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@toyobayashi toyobayashi mentioned this pull request Jan 30, 2024
Merged
@Janrupf Janrupf requested a review from mhdawson January 30, 2024 20:21
@Janrupf
Copy link
Contributor Author

Janrupf commented Jan 30, 2024

Fixed the linting errors, however, I have no idea why the test suddenly blew up on macOS. Possibly flaky?

vmoroz
vmoroz approved these changes Jan 30, 2024
View reviewed changes
Copy link
Member

@vmoroz vmoroz 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 mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2024
@mhdawson
Copy link
Member

@Janrupf kicked off another CI, I should have waited until the local ones had run before doing that earlier, we'll see what the results look like now.

@nodejs-github-bot
Copy link
Collaborator

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

@Janrupf
Copy link
Contributor Author

Janrupf commented Feb 1, 2024

Formatting should be good now, though the coverage test failed. I'm not sure if this is related to my changes (seems a bit unlikely)

mhdawson
mhdawson approved these changes Feb 2, 2024
View reviewed changes
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 mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 2, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 2, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

mertcanaltin
mertcanaltin approved these changes Feb 9, 2024
View reviewed changes
Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

LGTM

@Janrupf
Copy link
Contributor Author

Janrupf commented Feb 10, 2024

Jenkins still says its under security embargo, so I have no idea why the tests fail (they do pass locally...), any chance someone could take a look at this/make the logs available?

@nodejs-github-bot
Copy link
Collaborator

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

@Janrupf
Copy link
Contributor Author

Janrupf commented Feb 15, 2024

CI seems to be failing due to something unrelated again, @mhdawson mind taking a look?

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 15, 2024
@mhdawson
Copy link
Member

I think it's been too long to resume the existing ci, kicked off another one, expect it to need to be resumed a few times.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 15, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@Janrupf
Copy link
Contributor Author

Janrupf commented Feb 16, 2024

@mhdawson CI failed again (sorry for the pings...), this time another random check. I don't think this is related to my changes, but neither can I fully confirm it is not. Any idea if thats a flaky test or something really is broken?

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@mhdawson
Copy link
Member

Opened - #51813 for latest flaky test failure.

@nodejs-github-bot
Copy link
Collaborator

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

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 23, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 23, 2024
@nodejs-github-bot nodejs-github-bot merged commit 281c342 into nodejs:main Feb 23, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 281c342

marco-ippolito pushed a commit that referenced this pull request Feb 26, 2024
@Janrupf @marco-ippolito
node-api: make napi_get_buffer_info check if passed buffer is valid
d8351e4 
PR-URL: #51571
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Feb 26, 2024
@Janrupf @marco-ippolito
node-api: make napi_get_buffer_info check if passed buffer is valid
bab27c1 
PR-URL: #51571
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Feb 27, 2024
@Janrupf @marco-ippolito
node-api: make napi_get_buffer_info check if passed buffer is valid
bc6f33d 
PR-URL: #51571
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@Janrupf @richardlau
node-api: make napi_get_buffer_info check if passed buffer is valid
f6867f5 
PR-URL: #51571
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@Janrupf @richardlau
node-api: make napi_get_buffer_info check if passed buffer is valid
06a29f8 
PR-URL: #51571
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Merged
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
@Janrupf @rdw-msft
node-api: make napi_get_buffer_info check if passed buffer is valid
31ebed5 
PR-URL: nodejs#51571
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mertcanaltin mertcanaltin mertcanaltin approved these changes

@mhdawson mhdawson mhdawson approved these changes

@legendecas legendecas legendecas approved these changes

@vmoroz vmoroz vmoroz approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

napi_get_buffer_info crashes with an assertion error
6 participants
@Janrupf @nodejs-github-bot @mhdawson @vmoroz @legendecas @mertcanaltin