From 6705b48012bc5a2fa6cf017f43426c5b906b2e04 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Mon, 18 Dec 2023 17:06:45 -0500 Subject: [PATCH] fs: validate fd synchronously on c++ PR-URL: https://github.com/nodejs/node/pull/51027 Reviewed-By: Matteo Collina --- lib/fs.js | 27 +++------- src/node_file.cc | 40 ++++++++++----- src/util.cc | 85 +++++++++++++++++++++++++++++++ src/util.h | 5 ++ test/cctest/test_util.cc | 73 ++++++++++++++++++++++---- test/parallel/test-fs-fchmod.js | 22 ++++---- test/parallel/test-fs-fchown.js | 4 +- test/parallel/test-fs-truncate.js | 2 +- 8 files changed, 198 insertions(+), 60 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 81c53383357f7f..ecfc7c560a1a74 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -515,13 +515,12 @@ function defaultCloseCallback(err) { * @returns {void} */ function close(fd, callback = defaultCloseCallback) { - fd = getValidatedFd(fd); if (callback !== defaultCloseCallback) callback = makeCallback(callback); const req = new FSReqCallback(); req.oncomplete = callback; - binding.close(fd, req); + binding.close(getValidatedFd(fd), req); } /** @@ -530,9 +529,7 @@ function close(fd, callback = defaultCloseCallback) { * @returns {void} */ function closeSync(fd) { - fd = getValidatedFd(fd); - - binding.close(fd); + binding.close(getValidatedFd(fd)); } /** @@ -1106,7 +1103,6 @@ function ftruncate(fd, len = 0, callback) { callback = len; len = 0; } - fd = getValidatedFd(fd); validateInteger(len, 'len'); len = MathMax(0, len); callback = makeCallback(callback); @@ -1123,7 +1119,6 @@ function ftruncate(fd, len = 0, callback) { * @returns {void} */ function ftruncateSync(fd, len = 0) { - fd = getValidatedFd(fd); validateInteger(len, 'len'); len = MathMax(0, len); binding.ftruncate(fd, len); @@ -1275,7 +1270,6 @@ function rmSync(path, options) { * @returns {void} */ function fdatasync(fd, callback) { - fd = getValidatedFd(fd); const req = new FSReqCallback(); req.oncomplete = makeCallback(callback); binding.fdatasync(fd, req); @@ -1289,7 +1283,6 @@ function fdatasync(fd, callback) { * @returns {void} */ function fdatasyncSync(fd) { - fd = getValidatedFd(fd); binding.fdatasync(fd); } @@ -1301,7 +1294,6 @@ function fdatasyncSync(fd) { * @returns {void} */ function fsync(fd, callback) { - fd = getValidatedFd(fd); const req = new FSReqCallback(); req.oncomplete = makeCallback(callback); binding.fsync(fd, req); @@ -1314,7 +1306,6 @@ function fsync(fd, callback) { * @returns {void} */ function fsyncSync(fd) { - fd = getValidatedFd(fd); binding.fsync(fd); } @@ -1535,7 +1526,6 @@ function fstat(fd, options = { bigint: false }, callback) { callback = options; options = kEmptyObject; } - fd = getValidatedFd(fd); callback = makeStatsCallback(callback); const req = new FSReqCallback(options.bigint); @@ -1618,7 +1608,6 @@ function statfs(path, options = { bigint: false }, callback) { * @returns {Stats | undefined} */ function fstatSync(fd, options = { bigint: false }) { - fd = getValidatedFd(fd); const stats = binding.fstat(fd, options.bigint, undefined, false); if (stats === undefined) { return; @@ -1884,7 +1873,6 @@ function unlinkSync(path) { * @returns {void} */ function fchmod(fd, mode, callback) { - fd = getValidatedFd(fd); mode = parseFileMode(mode, 'mode'); callback = makeCallback(callback); @@ -1901,7 +1889,7 @@ function fchmod(fd, mode, callback) { */ function fchmodSync(fd, mode) { binding.fchmod( - getValidatedFd(fd), + fd, parseFileMode(mode, 'mode'), ); } @@ -2029,14 +2017,13 @@ function lchownSync(path, uid, gid) { * @returns {void} */ function fchown(fd, uid, gid, callback) { - fd = getValidatedFd(fd); validateInteger(uid, 'uid', -1, kMaxUserId); validateInteger(gid, 'gid', -1, kMaxUserId); callback = makeCallback(callback); const req = new FSReqCallback(); req.oncomplete = callback; - binding.fchown(fd, uid, gid, req); + binding.fchown(getValidatedFd(fd), uid, gid, req); } /** @@ -2047,11 +2034,10 @@ function fchown(fd, uid, gid, callback) { * @returns {void} */ function fchownSync(fd, uid, gid) { - fd = getValidatedFd(fd); validateInteger(uid, 'uid', -1, kMaxUserId); validateInteger(gid, 'gid', -1, kMaxUserId); - binding.fchown(fd, uid, gid); + binding.fchown(getValidatedFd(fd), uid, gid); } /** @@ -2141,7 +2127,6 @@ function utimesSync(path, atime, mtime) { * @returns {void} */ function futimes(fd, atime, mtime, callback) { - fd = getValidatedFd(fd); atime = toUnixTimestamp(atime, 'atime'); mtime = toUnixTimestamp(mtime, 'mtime'); callback = makeCallback(callback); @@ -2162,7 +2147,7 @@ function futimes(fd, atime, mtime, callback) { */ function futimesSync(fd, atime, mtime) { binding.futimes( - getValidatedFd(fd), + fd, toUnixTimestamp(atime, 'atime'), toUnixTimestamp(mtime, 'mtime'), ); diff --git a/src/node_file.cc b/src/node_file.cc index 6ad0391092b3ae..3f3de19dcd704a 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1242,8 +1242,10 @@ static void FStat(const FunctionCallbackInfo& args) { const int argc = args.Length(); CHECK_GE(argc, 2); - CHECK(args[0]->IsInt32()); - int fd = args[0].As()->Value(); + int fd; + if (!GetValidatedFd(env, args[0]).To(&fd)) { + return; + } bool use_bigint = args[1]->IsTrue(); if (!args[2]->IsUndefined()) { // fstat(fd, use_bigint, req) @@ -1493,18 +1495,20 @@ static void FTruncate(const FunctionCallbackInfo& args) { const int argc = args.Length(); CHECK_GE(argc, 2); - CHECK(args[0]->IsInt32()); - const int fd = args[0].As()->Value(); + int fd; + if (!GetValidatedFd(env, args[0]).To(&fd)) { + return; + } CHECK(IsSafeJsInt(args[1])); const int64_t len = args[1].As()->Value(); - if (argc > 2) { + if (argc > 2) { // ftruncate(fd, len, req) FSReqBase* req_wrap_async = GetReqWrap(args, 2); FS_ASYNC_TRACE_BEGIN0(UV_FS_FTRUNCATE, req_wrap_async) AsyncCall(env, req_wrap_async, args, "ftruncate", UTF8, AfterNoArgs, uv_fs_ftruncate, fd, len); - } else { + } else { // ftruncate(fd, len) FSReqWrapSync req_wrap_sync("ftruncate"); FS_SYNC_TRACE_BEGIN(ftruncate); SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_ftruncate, fd, len); @@ -1518,8 +1522,10 @@ static void Fdatasync(const FunctionCallbackInfo& args) { const int argc = args.Length(); CHECK_GE(argc, 1); - CHECK(args[0]->IsInt32()); - const int fd = args[0].As()->Value(); + int fd; + if (!GetValidatedFd(env, args[0]).To(&fd)) { + return; + } if (argc > 1) { // fdatasync(fd, req) FSReqBase* req_wrap_async = GetReqWrap(args, 1); @@ -1541,8 +1547,10 @@ static void Fsync(const FunctionCallbackInfo& args) { const int argc = args.Length(); CHECK_GE(argc, 1); - CHECK(args[0]->IsInt32()); - const int fd = args[0].As()->Value(); + int fd; + if (!GetValidatedFd(env, args[0]).To(&fd)) { + return; + } if (argc > 1) { FSReqBase* req_wrap_async = GetReqWrap(args, 1); @@ -2615,8 +2623,10 @@ static void FChmod(const FunctionCallbackInfo& args) { const int argc = args.Length(); CHECK_GE(argc, 2); - CHECK(args[0]->IsInt32()); - const int fd = args[0].As()->Value(); + int fd; + if (!GetValidatedFd(env, args[0]).To(&fd)) { + return; + } CHECK(args[1]->IsInt32()); const int mode = args[1].As()->Value(); @@ -2771,8 +2781,10 @@ static void FUTimes(const FunctionCallbackInfo& args) { const int argc = args.Length(); CHECK_GE(argc, 3); - CHECK(args[0]->IsInt32()); - const int fd = args[0].As()->Value(); + int fd; + if (!GetValidatedFd(env, args[0]).To(&fd)) { + return; + } CHECK(args[1]->IsNumber()); const double atime = args[1].As()->Value(); diff --git a/src/util.cc b/src/util.cc index 38b1f9be72747e..22b17c6f5f6a08 100644 --- a/src/util.cc +++ b/src/util.cc @@ -20,6 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. #include "util.h" // NOLINT(build/include_inline) +#include #include "util-inl.h" #include "debug_utils-inl.h" @@ -31,6 +32,7 @@ #include "node_v8_platform-inl.h" #include "string_bytes.h" #include "uv.h" +#include "v8-value.h" #ifdef _WIN32 #include // _S_IREAD _S_IWRITE @@ -702,4 +704,87 @@ RAIIIsolate::RAIIIsolate(const SnapshotData* data) RAIIIsolate::~RAIIIsolate() {} +// Returns a string representation of the input value, including type. +// JavaScript implementation is available in lib/internal/errors.js +std::string DetermineSpecificErrorType(Environment* env, + v8::Local input) { + if (input->IsFunction()) { + return "function"; + } else if (input->IsString()) { + auto value = Utf8Value(env->isolate(), input).ToString(); + if (value.size() > 28) { + value = value.substr(0, 25) + "..."; + } + if (value.find('\'') == std::string::npos) { + return SPrintF("type string ('%s')", value); + } + + // Stringify the input value. + Local stringified = + v8::JSON::Stringify(env->context(), input).ToLocalChecked(); + Utf8Value stringified_value(env->isolate(), stringified); + return SPrintF("type string (%s)", stringified_value.out()); + } else if (input->IsObject()) { + v8::Local constructor_name = + input.As()->GetConstructorName(); + Utf8Value name(env->isolate(), constructor_name); + return SPrintF("an instance of %s", name.out()); + } + + Utf8Value utf8_value(env->isolate(), + input->ToString(env->context()).ToLocalChecked()); + + if (input->IsNumber() || input->IsInt32() || input->IsUint32()) { + auto value = input.As()->Value(); + if (std::isnan(value)) { + return "type number (NaN)"; + } else if (std::isinf(value)) { + return "type number (Infinity)"; + } + return SPrintF("type number (%s)", utf8_value.out()); + } else if (input->IsBigInt() || input->IsBoolean() || input->IsSymbol()) { + Utf8Value type(env->isolate(), input->TypeOf(env->isolate())); + return SPrintF("type %s (%s)", type.out(), utf8_value.out()); + } + + // For example: null, undefined + return utf8_value.ToString(); +} + +v8::Maybe GetValidatedFd(Environment* env, + v8::Local input) { + if (!input->IsInt32() && !input->IsNumber()) { + std::string error_type = node::DetermineSpecificErrorType(env, input); + THROW_ERR_INVALID_ARG_TYPE(env, + "The \"fd\" argument must be of type " + "number. Received %s", + error_type.c_str()); + return v8::Nothing(); + } + + const double fd = input.As()->Value(); + const bool is_out_of_range = fd < 0 || fd > INT32_MAX; + + if (is_out_of_range || !IsSafeJsInt(input)) { + Utf8Value utf8_value( + env->isolate(), input->ToDetailString(env->context()).ToLocalChecked()); + if (is_out_of_range && !std::isinf(fd)) { + THROW_ERR_OUT_OF_RANGE(env, + "The value of \"fd\" is out of range. " + "It must be >= 0 && <= %s. Received %s", + std::to_string(INT32_MAX), + utf8_value.out()); + } else { + THROW_ERR_OUT_OF_RANGE( + env, + "The value of \"fd\" is out of range. It must be an integer. " + "Received %s", + utf8_value.out()); + } + return v8::Nothing(); + } + + return v8::Just(static_cast(fd)); +} + } // namespace node diff --git a/src/util.h b/src/util.h index 08b9151f1cfabf..83e5d226701b9b 100644 --- a/src/util.h +++ b/src/util.h @@ -1013,6 +1013,11 @@ class RAIIIsolate { v8::Isolate::Scope isolate_scope_; }; +std::string DetermineSpecificErrorType(Environment* env, + v8::Local input); + +v8::Maybe GetValidatedFd(Environment* env, v8::Local input); + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/test/cctest/test_util.cc b/test/cctest/test_util.cc index c48803cff0337e..79113965f50e36 100644 --- a/test/cctest/test_util.cc +++ b/test/cctest/test_util.cc @@ -1,8 +1,13 @@ #include "debug_utils-inl.h" #include "env-inl.h" #include "gtest/gtest.h" +#include "node_options-inl.h" +#include "node_test_fixture.h" #include "simdutf.h" #include "util-inl.h" +#include "v8-function-callback.h" +#include "v8-primitive.h" +#include "v8.h" using node::Calloc; using node::Malloc; @@ -14,7 +19,9 @@ using node::ToLower; using node::UncheckedCalloc; using node::UncheckedMalloc; -TEST(UtilTest, ListHead) { +class UtilTest : public EnvironmentTestFixture {}; + +TEST_F(UtilTest, ListHead) { struct Item { node::ListNode node_; }; typedef node::ListHead List; @@ -68,7 +75,7 @@ TEST(UtilTest, ListHead) { EXPECT_FALSE(list.begin() != list.end()); } -TEST(UtilTest, StringEqualNoCase) { +TEST_F(UtilTest, StringEqualNoCase) { EXPECT_FALSE(StringEqualNoCase("a", "b")); EXPECT_TRUE(StringEqualNoCase("", "")); EXPECT_TRUE(StringEqualNoCase("equal", "equal")); @@ -78,7 +85,7 @@ TEST(UtilTest, StringEqualNoCase) { EXPECT_FALSE(StringEqualNoCase("equals", "equal")); } -TEST(UtilTest, StringEqualNoCaseN) { +TEST_F(UtilTest, StringEqualNoCaseN) { EXPECT_FALSE(StringEqualNoCaseN("a", "b", strlen("a"))); EXPECT_TRUE(StringEqualNoCaseN("", "", strlen(""))); EXPECT_TRUE(StringEqualNoCaseN("equal", "equal", strlen("equal"))); @@ -92,7 +99,7 @@ TEST(UtilTest, StringEqualNoCaseN) { EXPECT_FALSE(StringEqualNoCaseN("abc\0abc", "abcd\0efg", strlen("abcdefgh"))); } -TEST(UtilTest, ToLower) { +TEST_F(UtilTest, ToLower) { EXPECT_EQ('0', ToLower('0')); EXPECT_EQ('a', ToLower('a')); EXPECT_EQ('a', ToLower('A')); @@ -105,28 +112,28 @@ TEST(UtilTest, ToLower) { free(pointer); \ } while (0) -TEST(UtilTest, Malloc) { +TEST_F(UtilTest, Malloc) { TEST_AND_FREE(Malloc, 0); TEST_AND_FREE(Malloc, 1); TEST_AND_FREE(Malloc, 0); TEST_AND_FREE(Malloc, 1); } -TEST(UtilTest, Calloc) { +TEST_F(UtilTest, Calloc) { TEST_AND_FREE(Calloc, 0); TEST_AND_FREE(Calloc, 1); TEST_AND_FREE(Calloc, 0); TEST_AND_FREE(Calloc, 1); } -TEST(UtilTest, UncheckedMalloc) { +TEST_F(UtilTest, UncheckedMalloc) { TEST_AND_FREE(UncheckedMalloc, 0); TEST_AND_FREE(UncheckedMalloc, 1); TEST_AND_FREE(UncheckedMalloc, 0); TEST_AND_FREE(UncheckedMalloc, 1); } -TEST(UtilTest, UncheckedCalloc) { +TEST_F(UtilTest, UncheckedCalloc) { TEST_AND_FREE(UncheckedCalloc, 0); TEST_AND_FREE(UncheckedCalloc, 1); TEST_AND_FREE(UncheckedCalloc, 0); @@ -212,7 +219,7 @@ static void MaybeStackBufferBasic() { free(rawbuf); } -TEST(UtilTest, MaybeStackBuffer) { +TEST_F(UtilTest, MaybeStackBuffer) { MaybeStackBufferBasic(); MaybeStackBufferBasic(); @@ -253,7 +260,7 @@ TEST(UtilTest, MaybeStackBuffer) { } } -TEST(UtilTest, SPrintF) { +TEST_F(UtilTest, SPrintF) { // %d, %u and %s all do the same thing. The actual C++ type is used to infer // the right representation. EXPECT_EQ(SPrintF("%s", false), "false"); @@ -300,6 +307,50 @@ TEST(UtilTest, SPrintF) { EXPECT_EQ(SPrintF("%s", with_zero), with_zero); } -TEST(UtilTest, DumpJavaScriptStackWithNoIsolate) { +TEST_F(UtilTest, DumpJavaScriptStackWithNoIsolate) { node::DumpJavaScriptBacktrace(stderr); } + +TEST_F(UtilTest, DetermineSpecificErrorType) { + const v8::HandleScope handle_scope(isolate_); + Argv argv; + Env env{handle_scope, argv, node::EnvironmentFlags::kNoBrowserGlobals}; + + // Boolean + EXPECT_EQ( + node::DetermineSpecificErrorType(*env, v8::Boolean::New(isolate_, true)), + "type boolean (true)"); + + // BigInt + EXPECT_EQ( + node::DetermineSpecificErrorType(*env, v8::BigInt::New(isolate_, 255)), + "type bigint (255)"); + + // String + EXPECT_EQ( + node::DetermineSpecificErrorType( + *env, v8::String::NewFromUtf8(isolate_, "input").ToLocalChecked()), + "type string ('input')"); + // String that calls JSONStringify + EXPECT_EQ( + node::DetermineSpecificErrorType( + *env, v8::String::NewFromUtf8(isolate_, "'input'").ToLocalChecked()), + "type string (\"'input'\")"); + EXPECT_EQ(node::DetermineSpecificErrorType( + *env, + v8::String::NewFromUtf8(isolate_, + "string with more than 26 characters") + .ToLocalChecked()), + "type string ('string with more than 26 ...')"); + + // Number, Int32, Uint32 + EXPECT_EQ( + node::DetermineSpecificErrorType(*env, v8::Number::New(isolate_, 10)), + "type number (10)"); + EXPECT_EQ( + node::DetermineSpecificErrorType(*env, v8::Int32::New(isolate_, -255)), + "type number (-255)"); + EXPECT_EQ( + node::DetermineSpecificErrorType(*env, v8::Uint32::New(isolate_, 255)), + "type number (255)"); +} diff --git a/test/parallel/test-fs-fchmod.js b/test/parallel/test-fs-fchmod.js index da37080ddeedd8..16425e7d1fe818 100644 --- a/test/parallel/test-fs-fchmod.js +++ b/test/parallel/test-fs-fchmod.js @@ -14,8 +14,8 @@ const fs = require('fs'); message: 'The "fd" argument must be of type number.' + common.invalidArgTypeHelper(input) }; - assert.throws(() => fs.fchmod(input), errObj); - assert.throws(() => fs.fchmodSync(input), errObj); + assert.throws(() => fs.fchmod(input, 0o666, () => {}), errObj); + assert.throws(() => fs.fchmodSync(input, 0o666), errObj); }); @@ -38,8 +38,8 @@ assert.throws(() => fs.fchmod(1, '123x'), { message: 'The value of "fd" is out of range. It must be >= 0 && <= ' + `2147483647. Received ${input}` }; - assert.throws(() => fs.fchmod(input), errObj); - assert.throws(() => fs.fchmodSync(input), errObj); + assert.throws(() => fs.fchmod(input, 0o666, () => {}), errObj); + assert.throws(() => fs.fchmodSync(input, 0o666), errObj); }); [-1, 2 ** 32].forEach((input) => { @@ -50,7 +50,7 @@ assert.throws(() => fs.fchmod(1, '123x'), { `4294967295. Received ${input}` }; - assert.throws(() => fs.fchmod(1, input), errObj); + assert.throws(() => fs.fchmod(1, input, () => {}), errObj); assert.throws(() => fs.fchmodSync(1, input), errObj); }); @@ -61,10 +61,10 @@ assert.throws(() => fs.fchmod(1, '123x'), { message: 'The value of "fd" is out of range. It must be an integer. ' + `Received ${input}` }; - assert.throws(() => fs.fchmod(input), errObj); - assert.throws(() => fs.fchmodSync(input), errObj); + assert.throws(() => fs.fchmod(input, 0o666, () => {}), errObj); + assert.throws(() => fs.fchmodSync(input, 0o666), errObj); errObj.message = errObj.message.replace('fd', 'mode'); - assert.throws(() => fs.fchmod(1, input), errObj); + assert.throws(() => fs.fchmod(1, input, () => {}), errObj); assert.throws(() => fs.fchmodSync(1, input), errObj); }); @@ -75,9 +75,9 @@ assert.throws(() => fs.fchmod(1, '123x'), { message: 'The value of "fd" is out of range. It must be an integer. ' + `Received ${input}` }; - assert.throws(() => fs.fchmod(input), errObj); - assert.throws(() => fs.fchmodSync(input), errObj); + assert.throws(() => fs.fchmod(input, 0o666, () => {}), errObj); + assert.throws(() => fs.fchmodSync(input, 0o666), errObj); errObj.message = errObj.message.replace('fd', 'mode'); - assert.throws(() => fs.fchmod(1, input), errObj); + assert.throws(() => fs.fchmod(1, input, () => {}), errObj); assert.throws(() => fs.fchmodSync(1, input), errObj); }); diff --git a/test/parallel/test-fs-fchown.js b/test/parallel/test-fs-fchown.js index 03a9ef3780ae6b..758bdde2575d9c 100644 --- a/test/parallel/test-fs-fchown.js +++ b/test/parallel/test-fs-fchown.js @@ -5,8 +5,8 @@ const assert = require('assert'); const fs = require('fs'); function testFd(input, errObj) { - assert.throws(() => fs.fchown(input), errObj); - assert.throws(() => fs.fchownSync(input), errObj); + assert.throws(() => fs.fchown(input, 0, 0, () => {}), errObj); + assert.throws(() => fs.fchownSync(input, 0, 0), errObj); } function testUid(input, errObj) { diff --git a/test/parallel/test-fs-truncate.js b/test/parallel/test-fs-truncate.js index de9687ebb27f0c..efaeeca7173c12 100644 --- a/test/parallel/test-fs-truncate.js +++ b/test/parallel/test-fs-truncate.js @@ -270,7 +270,7 @@ function testFtruncate(cb) { ['', false, null, undefined, {}, []].forEach((input) => { ['ftruncate', 'ftruncateSync'].forEach((fnName) => { assert.throws( - () => fs[fnName](input), + () => fs[fnName](input, 1, () => {}), { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError',