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

fs: fix writev empty array error behavior #41919

Merged
merged 1 commit into from Feb 12, 2022

Conversation

@benjamingr
Copy link
Member

@benjamingr benjamingr commented Feb 10, 2022

Fixes: #41910

Alternatively - we can error on an empty array but I like this behavior better.

cc @nodejs/fs @jasnell

aduh95
aduh95 approved these changes Feb 10, 2022
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Feb 10, 2022

ronag
ronag approved these changes Feb 10, 2022
@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 10, 2022

Are fs.writev and fs.writevSync affected?

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Feb 10, 2022

They throw a catchable error which is a lot less bad but the behavior for empty arrays should probably be adjusted there too (probably in a separate PR). I think we need to think how we do argument handling because it's currently very easy to make Node abort by passing a problematic argument.

That is - a lot more than just writeev is affected and I'd guess most methods break on problematic input (subclasses or types or proxies)

@nodejs-github-bot nodejs-github-bot merged commit a137eca into nodejs:master Feb 12, 2022
60 checks passed
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Feb 12, 2022

Landed in a137eca

@benjamingr benjamingr deleted the fix-ev-error branch Feb 12, 2022
bengl added a commit to bengl/node that referenced this issue Feb 21, 2022
PR-URL: nodejs#41919
Fixes: nodejs#41910
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
bengl added a commit to bengl/node that referenced this issue Feb 21, 2022
PR-URL: nodejs#41919
Fixes: nodejs#41910
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@bengl bengl mentioned this pull request Feb 21, 2022
bengl added a commit that referenced this issue Feb 22, 2022
PR-URL: #41919
Fixes: #41910
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants