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

[v14.x backport] fs: fix write methods param validation and docs #42603

Closed
wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Apr 4, 2022

Backport of #42588 and #41677. This fixes a bug that makes the process crash if a non-buffer object was passed to FileHandle.prototype.write:

$ node -e 'console.log(process.version);fs.promises.open("/dev/null").then(fh => fh.write({ toString() { return "str\n" } }))'
v14.19.1
/usr/local/opt/node@14/bin/node[13975]: ../src/string_bytes.cc:319:static size_t node::StringBytes::Write(v8::Isolate *, char *, size_t, Local<v8::Value>, enum encoding, int *): Assertion `val->IsString() == true' failed.
 1: 0x1000a6e4a node::Abort() [/usr/local/Cellar/node@14/14.19.1/bin/node]
 2: 0x1000a6cd3 node::AppendExceptionLine(node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Message>, node::ErrorHandlingMode) [/usr/local/Cellar/node@14/14.19.1/bin/node]
 3: 0x1001485c0 node::StringBytes::Write(v8::Isolate*, char*, unsigned long, v8::Local<v8::Value>, node::encoding, int*) [/usr/local/Cellar/node@14/14.19.1/bin/node]
 4: 0x1000b69f8 node::fs::WriteString(v8::FunctionCallbackInfo<v8::Value> const&) [/usr/local/Cellar/node@14/14.19.1/bin/node]
 5: 0x100205198 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [/usr/local/Cellar/node@14/14.19.1/bin/node]
 6: 0x1002048a1 v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/usr/local/Cellar/node@14/14.19.1/bin/node]
 7: 0x100204012 v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/usr/local/Cellar/node@14/14.19.1/bin/node]
 8: 0x1007ca639 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit [/usr/local/Cellar/node@14/14.19.1/bin/node]
 9: 0x1007610a2 Builtins_InterpreterEntryTrampoline [/usr/local/Cellar/node@14/14.19.1/bin/node]
10: 0x1007610a2 Builtins_InterpreterEntryTrampoline [/usr/local/Cellar/node@14/14.19.1/bin/node]
11: 0x10075afd9 Builtins_ArgumentsAdaptorTrampoline [/usr/local/Cellar/node@14/14.19.1/bin/node]
[1]    13975 abort      /usr/local/opt/node@14/bin/node -e
$ out/Release/node -e 'console.log(process.version);fs.promises.open("/dev/null").then(fh => fh.write({ toString() { return "str\n" } }))'
v14.19.2-pre
(node:16354) UnhandledPromiseRejectionWarning: TypeError [ERR_INVALID_ARG_TYPE]: The "buffer" argument must be of type string or an instance of Buffer, TypedArray, or DataView. Received an instance of Object
    at write (internal/fs/promises.js:464:3)
    at fsCall (internal/fs/promises.js:243:18)
    at FileHandle.write (internal/fs/promises.js:160:12)
    at [eval]:1:74
(Use `node --trace-warnings ...` to show where the warning was created)
(node:16354) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 3)
(node:16354) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
(node:16354) Warning: Closing file descriptor 23 on garbage collection
(node:16354) [DEP0137] DeprecationWarning: Closing a FileHandle object on garbage collection is deprecated. Please close FileHandle objects explicitly using FileHandle.prototype.close(). In the future, an error will be thrown if a file descriptor is closed during garbage collection.

PR-URL: nodejs#42588
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. v14.x labels Apr 4, 2022
The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: nodejs#34993
PR-URL: nodejs#41677
Refs: nodejs#41666
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95 aduh95 force-pushed the backport-fs-abort-fix branch from 02018d8 to 18ac4af Compare April 4, 2022 12:12
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2022
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 mentioned this pull request Apr 28, 2022
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 28, 2022

juanarbol pushed a commit that referenced this pull request May 1, 2022
PR-URL: #42588
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Backport-PR-URL: #42603
juanarbol pushed a commit that referenced this pull request May 1, 2022
The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: #34993
PR-URL: #41677
Refs: #41666
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: #42603
@juanarbol
Copy link
Member

Landed in 75302d3 and 77ba012 🎉

@juanarbol juanarbol closed this May 2, 2022
@aduh95 aduh95 deleted the backport-fs-abort-fix branch May 2, 2022 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants