From 86629746696b21ce73bf389d45740dc1c0191e88 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 5 Jul 2016 13:24:10 -0600 Subject: [PATCH 1/3] uv: expose uv_strerror() to JS API Retrieving the error message along with the error name is useful. So exposing to the JS API. Extend the existing error messages to include more information about the invalid error code. Also add additional UV_ERRNO_MAX range check to ErrName() to help prevent the following comment from libuv documentation on both uv_strerror() and uv_err_name(): Leaks a few bytes of memory when you call it with an unknown error code. --- src/uv.cc | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/uv.cc b/src/uv.cc index 21520f5cb8783c..088791d73c4815 100644 --- a/src/uv.cc +++ b/src/uv.cc @@ -3,9 +3,18 @@ #include "env.h" #include "env-inl.h" +#include // snprintf + namespace node { namespace uv { +#define RETURN_RANGE_ERROR(...) \ + do { \ + char buf[100]; \ + snprintf(buf, sizeof(buf), __VA_ARGS__); \ + return env->ThrowRangeError(buf); \ + } while (0) + using v8::Context; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -19,19 +28,31 @@ using v8::Value; void ErrName(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); int err = args[0]->Int32Value(); - if (err >= 0) - return env->ThrowError("err >= 0"); + if (err >= 0 || err < UV_ERRNO_MAX) + RETURN_RANGE_ERROR("err is an invalid error code: %i", err); const char* name = uv_err_name(err); args.GetReturnValue().Set(OneByteString(env->isolate(), name)); } +void StrError(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + int err = args[0]->Int32Value(); + if (err >= 0 || err < UV_ERRNO_MAX) + RETURN_RANGE_ERROR("err is an invalid error code: %i", err); + const char* name = uv_strerror(err); + args.GetReturnValue().Set(OneByteString(env->isolate(), name)); +} + + void Initialize(Local target, Local unused, Local context) { Environment* env = Environment::GetCurrent(context); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "errname"), env->NewFunctionTemplate(ErrName)->GetFunction()); + target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "strerror"), + env->NewFunctionTemplate(StrError)->GetFunction()); #define V(name, _) \ target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "UV_" # name), \ Integer::New(env->isolate(), UV_ ## name)); From bf8827b5051812ee0fef983fe5ee37b4d226277e Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 14 Jul 2016 11:57:08 -0600 Subject: [PATCH 2/3] fs: allow realpath to resolve deep symlinks realpath(3) would fail if the symbolic link depth was too deep. If ELOOP is encountered then resolve the path in parts until the entire thing is resolved. This excludes if the number of symbolic links is too deep, or if they are recursive. Fixes: https://github.com/nodejs/node/issues/7175 --- lib/fs.js | 58 +++++++++++++++++++++++++++- src/node_file.cc | 98 +++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 145 insertions(+), 11 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 39bb3777bf9035..86bc59362ffd8e 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1590,12 +1590,68 @@ fs.realpath = function realpath(path, options, callback) { if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); - req.oncomplete = callback; + req.oncomplete = function oncomplete(err, resolvedPath) { + interceptELOOP(err, resolvedPath, path, options, callback); + }; binding.realpath(pathModule._makeLong(path), options.encoding, req); return; }; +function interceptELOOP(err, resolvedPath, path, options, callback) { + if (resolvedPath) + return callback(err, resolvedPath); + if (err && err.code !== 'ELOOP') + return callback(err); + var retPath = ''; + // Start at -1 since first operation is to + 1. + var offset = -1; + var current = 0; + var slashCount = 0; + var callDepth = 0; + + // Can assume '/' because uv_fs_realpath() cannot return UV_ELOOP on win. + (function pathResolver(err2, resolvedPath2) { + // No need to handle an error that was returned by a recursive call. + if (err2) return callback(err2); + // callDepth is too much. Return original error. + if (++callDepth > 100) return callback(err); + // Done iterating over the path. + if (offset === path.length) return callback(null, resolvedPath2); + + retPath = resolvedPath2; + slashCount = countSlashes(retPath); + offset = get32SlashOffset(path, offset + 1, slashCount); + + var tmpPath = retPath + path.slice(current, offset); + current = offset; + var req = new FSReqWrap(); + req.oncomplete = pathResolver; + binding.realpath(pathModule._makeLong(tmpPath), options.encoding, req); + }(null, '')); +} + + +function get32SlashOffset(path, offset, slashCounter) { + // OSX libc bails with ELOOP when encountering more than MAXSYMLINKS, which + // is hard coded to in the kernel header to 32. + while ((offset = path.indexOf('/', offset + 1)) !== -1 && + ++slashCounter < 32); + if (offset === -1) + offset = path.length; + return offset; +} + + +function countSlashes(path) { + var offset = -1; + var counter = 0; + while ((offset = path.indexOf('/', offset + 1)) !== -1) + counter++; + return counter; +} + + fs.mkdtemp = function(prefix, options, callback) { if (!prefix || typeof prefix !== 'string') throw new TypeError('filename prefix is required'); diff --git a/src/node_file.cc b/src/node_file.cc index 968284788a4b72..83cfafda960287 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -361,16 +361,19 @@ class fs_req_wrap { #define ASYNC_CALL(func, req, encoding, ...) \ ASYNC_DEST_CALL(func, req, nullptr, encoding, __VA_ARGS__) \ -#define SYNC_DEST_CALL(func, path, dest, ...) \ - fs_req_wrap req_wrap; \ +#define SYNC_CALL_NO_THROW(req_wrap, func, dest, ...) \ env->PrintSyncTrace(); \ int err = uv_fs_ ## func(env->event_loop(), \ - &req_wrap.req, \ + &(req_wrap).req, \ __VA_ARGS__, \ - nullptr); \ - if (err < 0) { \ - return env->ThrowUVException(err, #func, nullptr, path, dest); \ - } \ + nullptr); + +#define SYNC_DEST_CALL(func, path, dest, ...) \ + fs_req_wrap req_wrap; \ + SYNC_CALL_NO_THROW(req_wrap, func, dest, __VA_ARGS__) \ + if (SYNC_RESULT < 0) { \ + return env->ThrowUVException(SYNC_RESULT, #func, nullptr, path, dest); \ + } #define SYNC_CALL(func, path, ...) \ SYNC_DEST_CALL(func, path, nullptr, __VA_ARGS__) \ @@ -882,6 +885,77 @@ static void MKDir(const FunctionCallbackInfo& args) { } } + +static size_t CountSlashes(const char* str, size_t len) { + size_t cntr = 0; + for (size_t i = 0; i < len; i++) { + if (str[i] == '/') cntr++; + } + return cntr; +} + + +static int ResolveRealPathSync(Environment* env, + std::string* ret_str, + const char* path, + size_t call_depth) { + fs_req_wrap req_wrap; + SYNC_CALL_NO_THROW(req_wrap, realpath, path, path); + + call_depth++; + if (SYNC_RESULT != UV_ELOOP) { + if (SYNC_RESULT) return SYNC_RESULT; + *ret_str = std::string(static_cast(SYNC_REQ.ptr)); + return SYNC_RESULT; + // TODO(trevnorris): Instead of simply not allowing too many recursive + // calls, would it instead be a viable solution to attempt detection of + // recursive symlinks? Thus preventing false negatives. + } else if (SYNC_RESULT == UV_ELOOP && call_depth > 100) { + return UV_ELOOP; + } + +#ifdef _WIN32 + const char separator = '\\'; +#else + const char separator = '/'; +#endif + std::string str_path(path); + size_t offset = 0; + size_t current = 0; + size_t slash_count = 0; + + // Can assume '/' because uv_fs_realpath() cannot return UV_ELOOP on win. + while ((offset = str_path.find(separator, offset + 1)) != std::string::npos) { + // OSX libc bails with ELOOP when encountering more than MAXSYMLINKS, + // which is hard coded to in the kernel header to 32. + if (++slash_count < 32) { + continue; + } + + std::string partial = *ret_str + str_path.substr(current, offset - current); + int err2 = ResolveRealPathSync(env, ret_str, partial.c_str(), call_depth); + // No need to handle an error that was returned by a recursive call. + if (err2) { + *ret_str = std::string(); + return err2; + } + + current = offset; + slash_count = CountSlashes(ret_str->c_str(), ret_str->length()); + } + + if (offset == std::string::npos) { + offset = str_path.length(); + } + if (current >= offset) { + return 0; + } + + std::string pass(*ret_str + str_path.substr(current, offset - current)); + return ResolveRealPathSync(env, ret_str, pass.c_str(), call_depth); +} + + static void RealPath(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -902,10 +976,14 @@ static void RealPath(const FunctionCallbackInfo& args) { if (callback->IsObject()) { ASYNC_CALL(realpath, callback, encoding, *path); } else { - SYNC_CALL(realpath, *path, *path); - const char* link_path = static_cast(SYNC_REQ.ptr); + std::string rc_string; + // Resolve the symlink attempting simple amount of deep path resolution. + int err = ResolveRealPathSync(env, &rc_string, *path, 0); + if (err) { + return env->ThrowUVException(err, "realpath", nullptr, *path, *path); + } Local rc = StringBytes::Encode(env->isolate(), - link_path, + rc_string.c_str(), encoding); if (rc.IsEmpty()) { return env->ThrowUVException(UV_EINVAL, From e7a38ab3e1819adfc02687ab5e18c79ad544db77 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 14 Jul 2016 11:57:26 -0600 Subject: [PATCH 3/3] test,bench: add tests/bench for fs.realpath() fix The test/benchmarks included also work for the previous JS implementation of fs.realpath(). In case the new implementation of realpath() needs to be reverted, we want these changes to stick around. --- benchmark/fs/bench-realpath.js | 46 ++++++++++++++++++ benchmark/fs/bench-realpathSync.js | 39 ++++++++++++++++ test/parallel/test-fs-realpath.js | 75 +++++++++++++++++++++++++++++- 3 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 benchmark/fs/bench-realpath.js create mode 100644 benchmark/fs/bench-realpathSync.js diff --git a/benchmark/fs/bench-realpath.js b/benchmark/fs/bench-realpath.js new file mode 100644 index 00000000000000..1a181935f14ec2 --- /dev/null +++ b/benchmark/fs/bench-realpath.js @@ -0,0 +1,46 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const path = require('path'); +const resolved_path = path.resolve(__dirname, '../../lib/'); +const relative_path = path.relative(__dirname, '../../lib/'); + +const bench = common.createBenchmark(main, { + n: [1e4], + type: ['relative', 'resolved'], +}); + + +function main(conf) { + const n = conf.n >>> 0; + const type = conf.type; + + bench.start(); + if (type === 'relative') + relativePath(n); + else if (type === 'resolved') + resolvedPath(n); + else + throw new Error('unknown "type": ' + type); +} + +function relativePath(n) { + (function r(cntr) { + if (--cntr <= 0) + return bench.end(n); + fs.realpath(relative_path, function() { + r(cntr); + }); + }(n)); +} + +function resolvedPath(n) { + (function r(cntr) { + if (--cntr <= 0) + return bench.end(n); + fs.realpath(resolved_path, function() { + r(cntr); + }); + }(n)); +} diff --git a/benchmark/fs/bench-realpathSync.js b/benchmark/fs/bench-realpathSync.js new file mode 100644 index 00000000000000..ae1c78d30d1b35 --- /dev/null +++ b/benchmark/fs/bench-realpathSync.js @@ -0,0 +1,39 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const path = require('path'); +const resolved_path = path.resolve(__dirname, '../../lib/'); +const relative_path = path.relative(__dirname, '../../lib/'); + +const bench = common.createBenchmark(main, { + n: [1e4], + type: ['relative', 'resolved'], +}); + + +function main(conf) { + const n = conf.n >>> 0; + const type = conf.type; + + bench.start(); + if (type === 'relative') + relativePath(n); + else if (type === 'resolved') + resolvedPath(n); + else + throw new Error('unknown "type": ' + type); + bench.end(n); +} + +function relativePath(n) { + for (var i = 0; i < n; i++) { + fs.realpathSync(relative_path); + } +} + +function resolvedPath(n) { + for (var i = 0; i < n; i++) { + fs.realpathSync(resolved_path); + } +} diff --git a/test/parallel/test-fs-realpath.js b/test/parallel/test-fs-realpath.js index 29be3127aae7a6..9d20f169ae543d 100644 --- a/test/parallel/test-fs-realpath.js +++ b/test/parallel/test-fs-realpath.js @@ -204,7 +204,7 @@ function test_cyclic_link_protection(callback) { fs.symlinkSync(t[1], t[0], 'dir'); unlink.push(t[0]); }); - assert.throws(function() { fs.realpathSync(entry); }); + assert.throws(function() { fs.realpathSync(entry); }, /ELOOP/); asynctest(fs.realpath, [entry], callback, function(err, result) { assert.ok(err && true); return true; @@ -461,6 +461,76 @@ function test_abs_with_kids(cb) { }); } +function test_deep_symlink_eloop(callback) { + console.log('test_deep_symlink_eloop'); + if (skipSymlinks) { + common.skip('symlink test (no privs)'); + return runNextTest(); + } + + const deepsymPath = path.join(targetsAbsDir, 'deep-symlink'); + const aPath = path.join(deepsymPath, 'a'); + const bSympath = path.join(aPath, 'b'); + const cleanupPaths = [bSympath]; + const pRepeat = 33; + + function cleanup() { + while (cleanupPaths.length > 0) { + try {fs.unlinkSync(cleanupPaths.pop());} catch (e) {} + } + } + + fs.mkdirSync(deepsymPath); + fs.mkdirSync(aPath); + fs.mkdirSync(path.join(targetsAbsDir, 'deep-symlink', 'c')); + try {fs.unlinkSync(bSympath);} catch (e) {} + fs.symlinkSync(deepsymPath, bSympath); + + // First test sync calls. + + const testPath = aPath + '/b/a'.repeat(pRepeat) + '/b/c'; + const resolvedPath = fs.realpathSync(testPath); + assert.equal(path.relative(deepsymPath, resolvedPath), 'c'); + + var reallyBigSymPath = deepsymPath; + var prev = null; + + // Make massively deep set of symlinks + for (var i = 97; i < 105; i++) { + for (var j = 97; j < 101; j++) { + const link = String.fromCharCode(i) + String.fromCharCode(j); + const link_path = path.join(deepsymPath, link); + cleanupPaths.push(link_path); + try {fs.unlinkSync(link_path);} catch (e) {} + if (prev) + fs.symlinkSync(link_path, prev); + reallyBigSymPath += '/' + link; + prev = link_path; + } + } + fs.symlinkSync(deepsymPath, prev); + reallyBigSymPath += '/' + path.basename(prev); + + assert.throws(() => fs.realpathSync(reallyBigSymPath), /ELOOP/); + + // Now test async calls. + + fs.realpath(testPath, (err, resolvedPath) => { + if (err) throw err; + assert.equal(path.relative(deepsymPath, resolvedPath), 'c'); + checkAsyncReallyBigSymPath(); + }); + + function checkAsyncReallyBigSymPath() { + fs.realpath(reallyBigSymPath, (err, path) => { + assert.ok(err); + assert.ok(/ELOOP/.test(err.message)); + cleanup(); + runNextTest(); + }); + } +} + // ---------------------------------------------------------------------------- var tests = [ @@ -476,7 +546,8 @@ var tests = [ test_non_symlinks, test_escape_cwd, test_abs_with_kids, - test_up_multiple + test_up_multiple, + test_deep_symlink_eloop, ]; var numtests = tests.length; var testsRun = 0;