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

Abort/core dump in fs.closeSync upon fd of greater than 32 bit #28980

Closed
coreyfarrell opened this issue Aug 5, 2019 · 1 comment
Closed

Abort/core dump in fs.closeSync upon fd of greater than 32 bit #28980

coreyfarrell opened this issue Aug 5, 2019 · 1 comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.

Comments

@coreyfarrell
Copy link
Member

  • Version: v12.7.0 installed with nvm
  • Platform: Linux lt2.cfware.com 5.1.20-300.fc30.x86_64 #1 SMP Fri Jul 26 15:03:11 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: fs

Tested in REPL:

const fs = require('fs')
fs.closeSync(4294967296)
fs.closeSync(4294967295)

fs.closeSync(4294967296) produces a JS exception:

Thrown:
RangeError [ERR_OUT_OF_RANGE]: The value of "fd" is out of range. It must be >= 0 && < 4294967296. Received 4294967296
    at Object.closeSync (fs.js:405:3)
    at repl:1:4
    at Script.runInThisContext (vm.js:126:20)
    at REPLServer.defaultEval (repl.js:384:29)
    at bound (domain.js:415:14)
    at REPLServer.runBound [as eval] (domain.js:428:12)
    at REPLServer.onLine (repl.js:700:10)
    at REPLServer.emit (events.js:208:15)
    at REPLServer.EventEmitter.emit (domain.js:471:20)
    at REPLServer.Interface._onLine (readline.js:316:10)

fs.closeSync(4294967295) trips a C++ assertion:

node[28006]: ../src/node_file.cc:802:void node::fs::Close(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `args[0]->IsInt32()' failed.
 1: 0x9afed0 node::Abort() [node]
 2: 0x9aff57  [node]
 3: 0x9ba25a node::fs::Close(v8::FunctionCallbackInfo<v8::Value> const&) [node]
 4: 0xb90726  [node]
 5: 0xb92647 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node]
 6: 0x1318979  [node]
Aborted (core dumped)

Obviously this is not a reasonable thing to do but an invalid fd it should produce a JS exception and not core dump.

@bnoordhuis bnoordhuis added c++ Issues and PRs that require attention from people who are familiar with C++. confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors. labels Aug 6, 2019
@bnoordhuis
Copy link
Member

bnoordhuis commented Aug 6, 2019

Can confirm. Added labels.

Close() in src/node_file.cc checks that it's an int32 whereas the binding.close() wrappers in lib/fs.js (close() and closeSync()) check that it's an uint32.

I think the logic in that last file should be updated to only accept integers in the range 0..2**31-1, i.e.:

validateInteger(fd, 'fd', 0, 0x7FFF_FFFF);

edit: missed Colin's PR somehow...

@targos targos removed the good first issue Issues that are suitable for first-time contributors. label Aug 6, 2019
@Trott Trott closed this as completed in c072a80 Aug 7, 2019
targos pushed a commit that referenced this issue Aug 19, 2019
This commit updates the JS layer's validation of file
descriptors to check for int32s >= 0 instead of uint32s.

PR-URL: #28984
Fixes: #28980
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Ayase-252 pushed a commit to Ayase-252/node that referenced this issue Apr 11, 2021
This commit updates the JS layer's validation of file
descriptors to check for int32s >= 0 instead of uint32s.

PR-URL: nodejs#28984
Fixes: nodejs#28980
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
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++. confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants