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

fs: fix long symlinks in realpath #7548

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions benchmark/fs/bench-realpath.js
Original file line number Diff line number Diff line change
@@ -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));
}
39 changes: 39 additions & 0 deletions benchmark/fs/bench-realpathSync.js
Original file line number Diff line number Diff line change
@@ -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);
}
}
58 changes: 57 additions & 1 deletion lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, ''));
}
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to do a single async call to ResolveRealPath instead of mirroring the C++ code in JS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Reason I did it this way was because handling/passing the async resources was being a thorn at the time. I'll see about moving this into C++.



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');
Expand Down
98 changes: 88 additions & 10 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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__) \
Expand Down Expand Up @@ -882,6 +885,77 @@ static void MKDir(const FunctionCallbackInfo<Value>& args) {
}
}


static size_t CountSlashes(const char* str, size_t len) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply pass the std::string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I'm using a simple for(), c_str() would still need to be called. So figured I'd pull it out before.

size_t cntr = 0;
for (size_t i = 0; i < len; i++) {
if (str[i] == '/') cntr++;
Copy link
Member

Choose a reason for hiding this comment

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

won't this be a problem on Windows? (I this code ever gets run there, that is)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't see in any of the win calls of uv_fs_realpath() that could return ERROR_CANT_RESOLVE_FILENAME (mapped to UV_ELOOP). Specifically those are CreateFileW() and pGetFinalPathNameByHandleW(). I just added a couple tests that will push up in a moment and run CI to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saghul So, seems I can get ELOOP from windows. Working on that.

}
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<const char*>(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<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand All @@ -902,10 +976,14 @@ static void RealPath(const FunctionCallbackInfo<Value>& args) {
if (callback->IsObject()) {
ASYNC_CALL(realpath, callback, encoding, *path);
} else {
SYNC_CALL(realpath, *path, *path);
const char* link_path = static_cast<const char*>(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<Value> rc = StringBytes::Encode(env->isolate(),
link_path,
rc_string.c_str(),
encoding);
if (rc.IsEmpty()) {
return env->ThrowUVException(UV_EINVAL,
Expand Down
25 changes: 23 additions & 2 deletions src/uv.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,18 @@
#include "env.h"
#include "env-inl.h"

#include <stdio.h> // 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;
Expand All @@ -19,19 +28,31 @@ using v8::Value;
void ErrName(const FunctionCallbackInfo<Value>& 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<Value>& 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<Object> target,
Local<Value> unused,
Local<Context> 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));
Expand Down
Loading