From 3f1b1c4d5bcba59123edcdc5514ff987657dfe73 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 28 Dec 2017 04:01:12 +0800 Subject: [PATCH 1/6] fs: return errno and take fs_req_wrap in SyncCall --- src/node_file.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index a7581099f6b0e6..4566f08c1a6454 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -375,11 +375,10 @@ inline FSReqWrap* AsyncCall(Environment* env, // the error number and the syscall in the context instead of // creating an error in the C++ land. template -inline void SyncCall(Environment* env, Local ctx, +inline int SyncCall(Environment* env, Local ctx, fs_req_wrap* req_wrap, const char* syscall, Func fn, Args... args) { - fs_req_wrap req_wrap; env->PrintSyncTrace(); - int err = fn(env->event_loop(), &req_wrap.req, args..., nullptr); + int err = fn(env->event_loop(), &(req_wrap->req), args..., nullptr); if (err < 0) { Local context = env->context(); Local ctx_obj = ctx->ToObject(context).ToLocalChecked(); @@ -391,6 +390,7 @@ inline void SyncCall(Environment* env, Local ctx, env->syscall_string(), OneByteString(isolate, syscall)).FromJust(); } + return err; } #define SYNC_DEST_CALL(func, path, dest, ...) \ @@ -426,7 +426,8 @@ void Access(const FunctionCallbackInfo& args) { AsyncCall(env, args, "access", UTF8, AfterNoArgs, uv_fs_access, *path, mode); } else { // access(path, mode, undefined, ctx) - SyncCall(env, args[3], "access", uv_fs_access, *path, mode); + fs_req_wrap req_wrap; + SyncCall(env, args[3], &req_wrap, "access", uv_fs_access, *path, mode); } } @@ -446,7 +447,8 @@ void Close(const FunctionCallbackInfo& args) { AsyncCall(env, args, "close", UTF8, AfterNoArgs, uv_fs_close, fd); } else { // close(fd, undefined, ctx) - SyncCall(env, args[2], "close", uv_fs_close, fd); + fs_req_wrap req_wrap; + SyncCall(env, args[2], &req_wrap, "close", uv_fs_close, fd); } } From ac03d5d11c5a37c74b399be988a0e718a063ba21 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 28 Dec 2017 04:01:45 +0800 Subject: [PATCH 2/6] fs: throw fs.stat{Sync} errors in JS --- lib/fs.js | 18 +++++++++++++++--- src/node_file.cc | 13 +++++++++---- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 77c6998e531e1c..4cf7d51f09573b 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -441,7 +441,11 @@ fs.existsSync = function(path) { return false; } nullCheck(path); - binding.stat(pathModule.toNamespacedPath(path)); + const ctx = { path }; + binding.stat(pathModule.toNamespacedPath(path), undefined, ctx); + if (ctx.errno !== undefined) { + return false; + } return true; } catch (e) { return false; @@ -1127,7 +1131,11 @@ fs.statSync = function(path) { handleError((path = getPathFromURL(path))); nullCheck(path); validatePath(path); - binding.stat(pathModule.toNamespacedPath(path)); + const ctx = { path }; + binding.stat(pathModule.toNamespacedPath(path), undefined, ctx); + if (ctx.errno !== undefined) { + throw new errors.uvException(ctx); + } return statsFromValues(); }; @@ -1927,7 +1935,11 @@ fs.realpathSync = function realpathSync(p, options) { } } if (linkTarget === null) { - binding.stat(baseLong); + const ctx = { path: base }; + binding.stat(baseLong, undefined, ctx); + if (ctx.errno !== undefined) { + throw new errors.uvException(ctx); + } linkTarget = binding.readlink(baseLong); } resolvedLink = pathModule.resolve(previous, linkTarget); diff --git a/src/node_file.cc b/src/node_file.cc index 4566f08c1a6454..7054338c07f82d 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -539,6 +539,7 @@ static void InternalModuleStat(const FunctionCallbackInfo& args) { static void Stat(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); + Local context = env->context(); CHECK_GE(args.Length(), 1); @@ -549,10 +550,14 @@ static void Stat(const FunctionCallbackInfo& args) { CHECK_EQ(args.Length(), 2); AsyncCall(env, args, "stat", UTF8, AfterStat, uv_fs_stat, *path); - } else { // stat(path) - SYNC_CALL(stat, *path, *path) - FillStatsArray(env->fs_stats_field_array(), - static_cast(SYNC_REQ.ptr)); + } else { // stat(path, undefined, ctx) + CHECK_EQ(args.Length(), 3); + fs_req_wrap req_wrap; + int err = SyncCall(env, args[2], &req_wrap, "stat", uv_fs_stat, *path); + if (err == 0) { + FillStatsArray(env->fs_stats_field_array(), + static_cast(req_wrap.req.ptr)); + } } } From 5567331cb4e57bb41d1663d95788ba21c6b9203e Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 28 Dec 2017 14:51:05 +0800 Subject: [PATCH 3/6] fs: throw fs.lstat{Sync} errors in JS --- lib/fs.js | 24 ++++++++++++++++++++---- src/node_file.cc | 13 +++++++++---- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 4cf7d51f09573b..6d389c8c53bb8c 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1123,7 +1123,11 @@ fs.lstatSync = function(path) { handleError((path = getPathFromURL(path))); nullCheck(path); validatePath(path); - binding.lstat(pathModule.toNamespacedPath(path)); + const ctx = { path }; + binding.lstat(pathModule.toNamespacedPath(path), undefined, ctx); + if (ctx.errno !== undefined) { + throw new errors.uvException(ctx); + } return statsFromValues(); }; @@ -1874,7 +1878,11 @@ fs.realpathSync = function realpathSync(p, options) { // On windows, check that the root exists. On unix there is no need. if (isWindows && !knownHard[base]) { - binding.lstat(pathModule.toNamespacedPath(base)); + const ctx = { path: base }; + binding.lstat(pathModule.toNamespacedPath(base), undefined, ctx); + if (ctx.errno !== undefined) { + throw new errors.uvException(ctx); + } knownHard[base] = true; } @@ -1914,7 +1922,11 @@ fs.realpathSync = function realpathSync(p, options) { // for our internal use. var baseLong = pathModule.toNamespacedPath(base); - binding.lstat(baseLong); + const ctx = { path: base }; + binding.lstat(baseLong, undefined, ctx); + if (ctx.errno !== undefined) { + throw new errors.uvException(ctx); + } if ((statValues[1/*mode*/] & S_IFMT) !== S_IFLNK) { knownHard[base] = true; @@ -1957,7 +1969,11 @@ fs.realpathSync = function realpathSync(p, options) { // On windows, check that the root exists. On unix there is no need. if (isWindows && !knownHard[base]) { - binding.lstat(pathModule.toNamespacedPath(base)); + const ctx = { path: base }; + binding.lstat(pathModule.toNamespacedPath(base), undefined, ctx); + if (ctx.errno !== undefined) { + throw new errors.uvException(ctx); + } knownHard[base] = true; } } diff --git a/src/node_file.cc b/src/node_file.cc index 7054338c07f82d..9ca94550d2cf15 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -563,6 +563,7 @@ static void Stat(const FunctionCallbackInfo& args) { static void LStat(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); + Local context = env->context(); CHECK_GE(args.Length(), 1); @@ -573,10 +574,14 @@ static void LStat(const FunctionCallbackInfo& args) { CHECK_EQ(args.Length(), 2); AsyncCall(env, args, "lstat", UTF8, AfterStat, uv_fs_lstat, *path); - } else { // lstat(path) - SYNC_CALL(lstat, *path, *path) - FillStatsArray(env->fs_stats_field_array(), - static_cast(SYNC_REQ.ptr)); + } else { // lstat(path, undefined, ctx) + CHECK_EQ(args.Length(), 3); + fs_req_wrap req_wrap; + int err = SyncCall(env, args[2], &req_wrap, "lstat", uv_fs_lstat, *path); + if (err == 0) { + FillStatsArray(env->fs_stats_field_array(), + static_cast(req_wrap.req.ptr)); + } } } From df89a060b27046f8ee2572dea5261808a8ffbab7 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 28 Dec 2017 23:16:33 +0800 Subject: [PATCH 4/6] fs: throw fs.fstat{Sync} errors in JS --- lib/fs.js | 17 ++++++++++------- src/node_file.cc | 15 ++++++++++----- test/parallel/test-fs-sync-fd-leak.js | 16 +++++++++------- 3 files changed, 29 insertions(+), 19 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 6d389c8c53bb8c..39919306de1b1b 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -633,12 +633,11 @@ function readFileAfterClose(err) { } function tryStatSync(fd, isUserFd) { - var threw = true; - try { - binding.fstat(fd); - threw = false; - } finally { - if (threw && !isUserFd) fs.closeSync(fd); + const ctx = {}; + binding.fstat(fd, undefined, ctx); + if (ctx.errno !== undefined && !isUserFd) { + fs.closeSync(fd); + throw new errors.uvException(ctx); } } @@ -1115,7 +1114,11 @@ fs.stat = function(path, callback) { fs.fstatSync = function(fd) { validateUint32(fd, 'fd'); - binding.fstat(fd); + const ctx = { fd }; + binding.fstat(fd, undefined, ctx); + if (ctx.errno !== undefined) { + throw new errors.uvException(ctx); + } return statsFromValues(); }; diff --git a/src/node_file.cc b/src/node_file.cc index 9ca94550d2cf15..c9bbea1ee5f621 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -587,19 +587,24 @@ static void LStat(const FunctionCallbackInfo& args) { static void FStat(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); + Local context = env->context(); CHECK(args[0]->IsInt32()); - int fd = args[0]->Int32Value(); + int fd = static_cast(args[0]->Int32Value(context).FromJust()); if (args[1]->IsObject()) { // fstat(fd, req) CHECK_EQ(args.Length(), 2); AsyncCall(env, args, "fstat", UTF8, AfterStat, uv_fs_fstat, fd); - } else { // fstat(fd) - SYNC_CALL(fstat, nullptr, fd) - FillStatsArray(env->fs_stats_field_array(), - static_cast(SYNC_REQ.ptr)); + } else { // fstat(fd, undefined, ctx) + CHECK_EQ(args.Length(), 3); + fs_req_wrap req_wrap; + int err = SyncCall(env, args[2], &req_wrap, "fstat", uv_fs_fstat, fd); + if (err == 0) { + FillStatsArray(env->fs_stats_field_array(), + static_cast(req_wrap.req.ptr)); + } } } diff --git a/test/parallel/test-fs-sync-fd-leak.js b/test/parallel/test-fs-sync-fd-leak.js index 7e785ea3a2a82b..e7107546e5b217 100644 --- a/test/parallel/test-fs-sync-fd-leak.js +++ b/test/parallel/test-fs-sync-fd-leak.js @@ -23,6 +23,7 @@ require('../common'); const assert = require('assert'); const fs = require('fs'); +const uv = process.binding('uv'); // ensure that (read|write|append)FileSync() closes the file descriptor fs.openSync = function() { @@ -39,29 +40,30 @@ fs.writeSync = function() { throw new Error('BAM'); }; -process.binding('fs').fstat = function() { - throw new Error('BAM'); +process.binding('fs').fstat = function(fd, _, ctx) { + ctx.errno = uv.UV_EBADF; + ctx.syscall = 'fstat'; }; let close_called = 0; ensureThrows(function() { fs.readFileSync('dummy'); -}); +}, 'EBADF: bad file descriptor, fstat'); ensureThrows(function() { fs.writeFileSync('dummy', 'xxx'); -}); +}, 'BAM'); ensureThrows(function() { fs.appendFileSync('dummy', 'xxx'); -}); +}, 'BAM'); -function ensureThrows(cb) { +function ensureThrows(cb, message) { let got_exception = false; close_called = 0; try { cb(); } catch (e) { - assert.strictEqual(e.message, 'BAM'); + assert.strictEqual(e.message, message); got_exception = true; } From aa1f9055696c752ee28719f40815dc06dc5b7c06 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 28 Dec 2017 23:59:53 +0800 Subject: [PATCH 5/6] test: verify errors thrown from fs stat APIs --- test/parallel/test-fs-error-messages.js | 127 ++++++++++++++++-------- 1 file changed, 85 insertions(+), 42 deletions(-) diff --git a/test/parallel/test-fs-error-messages.js b/test/parallel/test-fs-error-messages.js index f4d203e7d5e79f..39270002b08b5c 100644 --- a/test/parallel/test-fs-error-messages.js +++ b/test/parallel/test-fs-error-messages.js @@ -20,7 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. 'use strict'; -require('../common'); +const common = require('../common'); const fixtures = require('../common/fixtures'); const assert = require('assert'); const fs = require('fs'); @@ -29,72 +29,95 @@ const existingFile = fixtures.path('exit.js'); const existingFile2 = fixtures.path('create-file.js'); const existingDir = fixtures.path('empty'); const existingDir2 = fixtures.path('keys'); +const uv = process.binding('uv'); // ASYNC_CALL -fs.stat(fn, function(err) { +fs.stat(fn, common.mustCall((err) => { assert.strictEqual(fn, err.path); - assert.ok(err.message.includes(fn)); -}); - -fs.lstat(fn, function(err) { - assert.ok(err.message.includes(fn)); -}); + assert.strictEqual( + err.message, + `ENOENT: no such file or directory, stat '${fn}'`); + assert.strictEqual(err.errno, uv.UV_ENOENT); + assert.strictEqual(err.code, 'ENOENT'); + assert.strictEqual(err.syscall, 'stat'); +})); + +fs.lstat(fn, common.mustCall((err) => { + assert.strictEqual(fn, err.path); + assert.strictEqual( + err.message, + `ENOENT: no such file or directory, lstat '${fn}'`); + assert.strictEqual(err.errno, uv.UV_ENOENT); + assert.strictEqual(err.code, 'ENOENT'); + assert.strictEqual(err.syscall, 'lstat'); +})); + +{ + const fd = fs.openSync(existingFile, 'r'); + fs.closeSync(fd); + fs.fstat(fd, common.mustCall((err) => { + assert.strictEqual(err.message, 'EBADF: bad file descriptor, fstat'); + assert.strictEqual(err.errno, uv.UV_EBADF); + assert.strictEqual(err.code, 'EBADF'); + assert.strictEqual(err.syscall, 'fstat'); + })); +} -fs.readlink(fn, function(err) { +fs.readlink(fn, common.mustCall((err) => { assert.ok(err.message.includes(fn)); -}); +})); -fs.link(fn, 'foo', function(err) { +fs.link(fn, 'foo', common.mustCall((err) => { assert.ok(err.message.includes(fn)); -}); +})); -fs.link(existingFile, existingFile2, function(err) { +fs.link(existingFile, existingFile2, common.mustCall((err) => { assert.ok(err.message.includes(existingFile)); assert.ok(err.message.includes(existingFile2)); -}); +})); -fs.symlink(existingFile, existingFile2, function(err) { +fs.symlink(existingFile, existingFile2, common.mustCall((err) => { assert.ok(err.message.includes(existingFile)); assert.ok(err.message.includes(existingFile2)); -}); +})); -fs.unlink(fn, function(err) { +fs.unlink(fn, common.mustCall((err) => { assert.ok(err.message.includes(fn)); -}); +})); -fs.rename(fn, 'foo', function(err) { +fs.rename(fn, 'foo', common.mustCall((err) => { assert.ok(err.message.includes(fn)); -}); +})); -fs.rename(existingDir, existingDir2, function(err) { +fs.rename(existingDir, existingDir2, common.mustCall((err) => { assert.ok(err.message.includes(existingDir)); assert.ok(err.message.includes(existingDir2)); -}); +})); -fs.rmdir(fn, function(err) { +fs.rmdir(fn, common.mustCall((err) => { assert.ok(err.message.includes(fn)); -}); +})); -fs.mkdir(existingFile, 0o666, function(err) { +fs.mkdir(existingFile, 0o666, common.mustCall((err) => { assert.ok(err.message.includes(existingFile)); -}); +})); -fs.rmdir(existingFile, function(err) { +fs.rmdir(existingFile, common.mustCall((err) => { assert.ok(err.message.includes(existingFile)); -}); +})); -fs.chmod(fn, 0o666, function(err) { +fs.chmod(fn, 0o666, common.mustCall((err) => { assert.ok(err.message.includes(fn)); -}); +})); -fs.open(fn, 'r', 0o666, function(err) { +fs.open(fn, 'r', 0o666, common.mustCall((err) => { assert.ok(err.message.includes(fn)); -}); +})); -fs.readFile(fn, function(err) { +fs.readFile(fn, common.mustCall((err) => { assert.ok(err.message.includes(fn)); -}); +})); // Sync @@ -106,7 +129,13 @@ try { fs.statSync(fn); } catch (err) { errors.push('stat'); - assert.ok(err.message.includes(fn)); + assert.strictEqual(fn, err.path); + assert.strictEqual( + err.message, + `ENOENT: no such file or directory, stat '${fn}'`); + assert.strictEqual(err.errno, uv.UV_ENOENT); + assert.strictEqual(err.code, 'ENOENT'); + assert.strictEqual(err.syscall, 'stat'); } try { @@ -130,7 +159,26 @@ try { fs.lstatSync(fn); } catch (err) { errors.push('lstat'); - assert.ok(err.message.includes(fn)); + assert.strictEqual(fn, err.path); + assert.strictEqual( + err.message, + `ENOENT: no such file or directory, lstat '${fn}'`); + assert.strictEqual(err.errno, uv.UV_ENOENT); + assert.strictEqual(err.code, 'ENOENT'); + assert.strictEqual(err.syscall, 'lstat'); +} + +try { + ++expected; + const fd = fs.openSync(existingFile, 'r'); + fs.closeSync(fd); + fs.fstatSync(fd); +} catch (err) { + errors.push('fstat'); + assert.strictEqual(err.message, 'EBADF: bad file descriptor, fstat'); + assert.strictEqual(err.errno, uv.UV_EBADF); + assert.strictEqual(err.code, 'EBADF'); + assert.strictEqual(err.syscall, 'fstat'); } try { @@ -224,9 +272,4 @@ try { assert.ok(err.message.includes(fn)); } -process.on('exit', function() { - assert.strictEqual( - expected, errors.length, - `Test fs sync exceptions raised, got ${errors.length} expected ${expected}` - ); -}); +assert.strictEqual(expected, errors.length); From a262c87bc9ac383a1b7fbdbe1690e427ef203ef2 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 29 Dec 2017 18:14:30 +0800 Subject: [PATCH 6/6] test: test error messages from fs.realpath{Sync} --- test/parallel/test-fs-error-messages.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/parallel/test-fs-error-messages.js b/test/parallel/test-fs-error-messages.js index 39270002b08b5c..625b791b0b1a7a 100644 --- a/test/parallel/test-fs-error-messages.js +++ b/test/parallel/test-fs-error-messages.js @@ -64,6 +64,16 @@ fs.lstat(fn, common.mustCall((err) => { })); } +fs.realpath(fn, common.mustCall((err) => { + assert.strictEqual(fn, err.path); + assert.strictEqual( + err.message, + `ENOENT: no such file or directory, lstat '${fn}'`); + assert.strictEqual(err.errno, uv.UV_ENOENT); + assert.strictEqual(err.code, 'ENOENT'); + assert.strictEqual(err.syscall, 'lstat'); +})); + fs.readlink(fn, common.mustCall((err) => { assert.ok(err.message.includes(fn)); })); @@ -181,6 +191,20 @@ try { assert.strictEqual(err.syscall, 'fstat'); } +try { + ++expected; + fs.realpathSync(fn); +} catch (err) { + errors.push('realpath'); + assert.strictEqual(fn, err.path); + assert.strictEqual( + err.message, + `ENOENT: no such file or directory, lstat '${fn}'`); + assert.strictEqual(err.errno, uv.UV_ENOENT); + assert.strictEqual(err.code, 'ENOENT'); + assert.strictEqual(err.syscall, 'lstat'); +} + try { ++expected; fs.readlinkSync(fn);