From 95b7560d8e5be44ffef8190a556400a7aff5d2cd Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 2 May 2016 16:31:20 -0700 Subject: [PATCH] src,module: add --preserve-symlinks command line flag Add the `--preserve-symlinks` flag. This makes the changes added in #5950 conditional. By default the old behavior is used. With the flag set, symlinks are preserved, switching to the new behavior. This should be considered to be a temporary solution until we figure out how to solve the symlinked peer dependency problem in a more general way that does not break everything else. Additional test cases are included. PR-URL: https://github.com/nodejs/node/pull/6537 Reviewed-By: Ben Noordhuis --- doc/api/cli.md | 38 ++++++++++- doc/node.1 | 5 ++ lib/module.js | 14 ++-- src/node.cc | 9 +++ src/node_config.cc | 5 +- src/node_internals.h | 5 ++ test/addons/symlinked-module/binding.cc | 13 ++++ test/addons/symlinked-module/binding.gyp | 8 +++ test/addons/symlinked-module/submodule.js | 13 ++++ test/addons/symlinked-module/test.js | 34 ++++++++++ .../parallel/test-module-circular-symlinks.js | 68 +++++++++++++++++++ .../test-module-symlinked-peer-modules.js | 65 ++++++++++++++++++ test/parallel/test-require-symlink.js | 3 +- 13 files changed, 271 insertions(+), 9 deletions(-) create mode 100644 test/addons/symlinked-module/binding.cc create mode 100644 test/addons/symlinked-module/binding.gyp create mode 100644 test/addons/symlinked-module/submodule.js create mode 100644 test/addons/symlinked-module/test.js create mode 100644 test/parallel/test-module-circular-symlinks.js create mode 100644 test/parallel/test-module-symlinked-peer-modules.js diff --git a/doc/api/cli.md b/doc/api/cli.md index ac0233b9d7e2c4..edb375b5258861 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -97,6 +97,43 @@ Automatically zero-fills all newly allocated [Buffer][] and [SlowBuffer][] instances. +### `--preserve-symlinks` + +Instructs the module loader to preserve symbolic links when resolving and +caching modules. + +By default, when Node.js loads a module from a path that is symbolically linked +to a different on-disk location, Node.js will dereference the link and use the +actual on-disk "real path" of the module as both an identifier and as a root +path to locate other dependency modules. In most cases, this default behavior +is acceptable. However, when using symbolically linked peer dependencies, as +illustrated in the example below, the default behavior causes an exception to +be thrown if `moduleA` attempts to require `moduleB` as a peer dependency: + +```text +{appDir} + ├── app + │ ├── index.js + │ └── node_modules + │ ├── moduleA -> {appDir}/moduleA + │ └── moduleB + │ ├── index.js + │ └── package.json + └── moduleA + ├── index.js + └── package.json +``` + +The `--preserve-symlinks` command line flag instructs Node.js to use the +symlink path for modules as opposed to the real path, allowing symbolically +linked peer dependencies to be found. + +Note, however, that using `--preserve-symlinks` can have other side effects. +Specifically, symbolically linked *native* modules can fail to load if those +are linked from more than one location in the dependency tree (Node.js would +see those as two separate modules and would attempt to load the module multiple +times, causing an exception to be thrown). + ### `--track-heap-objects` Track heap object allocations for heap snapshots. @@ -138,7 +175,6 @@ Force FIPS-compliant crypto on startup. (Cannot be disabled from script code.) Specify ICU data load path. (overrides `NODE_ICU_DATA`) - ## Environment Variables ### `NODE_DEBUG=module[,…]` diff --git a/doc/node.1 b/doc/node.1 index cd3ffb17a038dd..cec87222b54d11 100644 --- a/doc/node.1 +++ b/doc/node.1 @@ -95,6 +95,11 @@ of the event loop. .BR \-\-zero\-fill\-buffers Automatically zero-fills all newly allocated Buffer and SlowBuffer instances. +.TP +.BR \-\-preserve\-symlinks +Instructs the module loader to preserve symbolic links when resolving and +caching modules. + .TP .BR \-\-track\-heap-objects Track heap object allocations for heap snapshots. diff --git a/lib/module.js b/lib/module.js index 344ea97af91953..ccbb5ba0e0a74d 100644 --- a/lib/module.js +++ b/lib/module.js @@ -10,6 +10,7 @@ const fs = require('fs'); const path = require('path'); const internalModuleReadFile = process.binding('fs').internalModuleReadFile; const internalModuleStat = process.binding('fs').internalModuleStat; +const preserveSymlinks = !!process.binding('config').preserveSymlinks; // If obj.hasOwnProperty has been overridden, then calling // obj.hasOwnProperty(prop) will break. @@ -109,14 +110,15 @@ function tryPackage(requestPath, exts, isMain) { } // check if the file exists and is not a directory -// resolve to the absolute realpath if running main module, -// otherwise resolve to absolute while keeping symlinks intact. +// if using --preserve-symlinks and isMain is false, +// keep symlinks intact, otherwise resolve to the +// absolute realpath. function tryFile(requestPath, isMain) { const rc = stat(requestPath); - if (isMain) { - return rc === 0 && fs.realpathSync(requestPath); + if (preserveSymlinks && !isMain) { + return rc === 0 && path.resolve(requestPath); } - return rc === 0 && path.resolve(requestPath); + return rc === 0 && fs.realpathSync(requestPath); } // given a path check a the file exists with any of the set extensions @@ -159,7 +161,7 @@ Module._findPath = function(request, paths, isMain) { if (!trailingSlash) { const rc = stat(basePath); if (rc === 0) { // File. - if (!isMain) { + if (preserveSymlinks && !isMain) { filename = path.resolve(basePath); } else { filename = fs.realpathSync(basePath); diff --git a/src/node.cc b/src/node.cc index b7b53d895b49ba..6d2aad7264548c 100644 --- a/src/node.cc +++ b/src/node.cc @@ -168,6 +168,11 @@ bool force_fips_crypto = false; bool no_process_warnings = false; bool trace_warnings = false; +// Set in node.cc by ParseArgs when --preserve-symlinks is used. +// Used in node_config.cc to set a constant on process.binding('config') +// that is used by lib/module.js +bool config_preserve_symlinks = false; + // process-relative uptime base, initialized at start-up static double prog_start_time; static bool debugger_running; @@ -3460,6 +3465,8 @@ static void PrintHelp() { " note: linked-in ICU data is\n" " present.\n" #endif + " --preserve-symlinks preserve symbolic links when resolving\n" + " and caching modules.\n" #endif "\n" "Environment variables:\n" @@ -3589,6 +3596,8 @@ static void ParseArgs(int* argc, } else if (strncmp(arg, "--security-revert=", 18) == 0) { const char* cve = arg + 18; Revert(cve); + } else if (strcmp(arg, "--preserve-symlinks") == 0) { + config_preserve_symlinks = true; } else if (strcmp(arg, "--prof-process") == 0) { prof_process = true; short_circuit = true; diff --git a/src/node_config.cc b/src/node_config.cc index 84917397744a4e..414d2e858d96a1 100644 --- a/src/node_config.cc +++ b/src/node_config.cc @@ -41,7 +41,10 @@ void InitConfig(Local target, if (flag_icu_data_dir) READONLY_BOOLEAN_PROPERTY("usingICUDataDir"); #endif // NODE_HAVE_I18N_SUPPORT -} + + if (config_preserve_symlinks) + READONLY_BOOLEAN_PROPERTY("preserveSymlinks"); +} // InitConfig } // namespace node diff --git a/src/node_internals.h b/src/node_internals.h index 485ba18b3c742e..0d660705c8efb4 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -30,6 +30,11 @@ struct sockaddr; namespace node { +// Set in node.cc by ParseArgs when --preserve-symlinks is used. +// Used in node_config.cc to set a constant on process.binding('config') +// that is used by lib/module.js +extern bool config_preserve_symlinks; + // Forward declaration class Environment; diff --git a/test/addons/symlinked-module/binding.cc b/test/addons/symlinked-module/binding.cc new file mode 100644 index 00000000000000..cdf9904e3f8d47 --- /dev/null +++ b/test/addons/symlinked-module/binding.cc @@ -0,0 +1,13 @@ +#include +#include + +void Method(const v8::FunctionCallbackInfo& args) { + v8::Isolate* isolate = args.GetIsolate(); + args.GetReturnValue().Set(v8::String::NewFromUtf8(isolate, "world")); +} + +void init(v8::Local target) { + NODE_SET_METHOD(target, "hello", Method); +} + +NODE_MODULE(binding, init); diff --git a/test/addons/symlinked-module/binding.gyp b/test/addons/symlinked-module/binding.gyp new file mode 100644 index 00000000000000..3bfb84493f3e87 --- /dev/null +++ b/test/addons/symlinked-module/binding.gyp @@ -0,0 +1,8 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'binding.cc' ] + } + ] +} diff --git a/test/addons/symlinked-module/submodule.js b/test/addons/symlinked-module/submodule.js new file mode 100644 index 00000000000000..d4b59e9efa4f5e --- /dev/null +++ b/test/addons/symlinked-module/submodule.js @@ -0,0 +1,13 @@ +'use strict'; +require('../../common'); +const path = require('path'); +const assert = require('assert'); + +// This is a subtest of symlinked-module/test.js. This is not +// intended to be run directly. + +module.exports.test = function test(bindingDir) { + const mod = require(path.join(bindingDir, 'binding.node')); + assert.notStrictEqual(mod, null); + assert.strictEqual(mod.hello(), 'world'); +}; diff --git a/test/addons/symlinked-module/test.js b/test/addons/symlinked-module/test.js new file mode 100644 index 00000000000000..8d3ced56e133db --- /dev/null +++ b/test/addons/symlinked-module/test.js @@ -0,0 +1,34 @@ +'use strict'; +const common = require('../../common'); +const fs = require('fs'); +const path = require('path'); +const assert = require('assert'); + +// This test verifies that symlinked native modules can be required multiple +// times without error. The symlinked module and the non-symlinked module +// should be the same instance. This expectation was not previously being +// tested and ended up being broken by https://github.com/nodejs/node/pull/5950. + +// This test should pass in Node.js v4 and v5. This test will pass in Node.js +// with https://github.com/nodejs/node/pull/5950 reverted. + +common.refreshTmpDir(); + +const addonPath = path.join(__dirname, 'build', 'Release'); +const addonLink = path.join(common.tmpDir, 'addon'); + +try { + fs.symlinkSync(addonPath, addonLink); +} catch (err) { + if (err.code !== 'EPERM') throw err; + common.skip('module identity test (no privs for symlinks)'); + return; +} + +const sub = require('./submodule'); +[addonPath, addonLink].forEach((i) => { + const mod = require(path.join(i, 'binding.node')); + assert.notStrictEqual(mod, null); + assert.strictEqual(mod.hello(), 'world'); + assert.doesNotThrow(() => sub.test(i)); +}); diff --git a/test/parallel/test-module-circular-symlinks.js b/test/parallel/test-module-circular-symlinks.js new file mode 100644 index 00000000000000..a04022c6564bbd --- /dev/null +++ b/test/parallel/test-module-circular-symlinks.js @@ -0,0 +1,68 @@ +'use strict'; + +// This tests to make sure that modules with symlinked circular dependencies +// do not blow out the module cache and recurse forever. See issue +// https://github.com/nodejs/node/pull/5950 for context. PR #5950 attempted +// to solve a problem with symlinked peer dependencies by caching using the +// symlink path. Unfortunately, that breaks the case tested in this module +// because each symlinked module, despite pointing to the same code on disk, +// is loaded and cached as a separate module instance, which blows up the +// cache and leads to a recursion bug. + +// This test should pass in Node.js v4 and v5. It should pass in Node.js v6 +// after https://github.com/nodejs/node/pull/5950 has been reverted. + +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const fs = require('fs'); + +// {tmpDir} +// ├── index.js +// └── node_modules +// ├── moduleA +// │ ├── index.js +// │ └── node_modules +// │ └── moduleB -> {tmpDir}/node_modules/moduleB +// └── moduleB +// ├── index.js +// └── node_modules +// └── moduleA -> {tmpDir}/node_modules/moduleA + +common.refreshTmpDir(); +const tmpDir = common.tmpDir; + +const node_modules = path.join(tmpDir, 'node_modules'); +const moduleA = path.join(node_modules, 'moduleA'); +const moduleB = path.join(node_modules, 'moduleB'); +const moduleA_link = path.join(moduleB, 'node_modules', 'moduleA'); +const moduleB_link = path.join(moduleA, 'node_modules', 'moduleB'); + +fs.mkdirSync(node_modules); +fs.mkdirSync(moduleA); +fs.mkdirSync(moduleB); +fs.mkdirSync(path.join(moduleA, 'node_modules')); +fs.mkdirSync(path.join(moduleB, 'node_modules')); + +try { + fs.symlinkSync(moduleA, moduleA_link); + fs.symlinkSync(moduleB, moduleB_link); +} catch (err) { + if (err.code !== 'EPERM') throw err; + common.skip('insufficient privileges for symlinks'); + return; +} + +fs.writeFileSync(path.join(tmpDir, 'index.js'), + 'module.exports = require(\'moduleA\');', 'utf8'); +fs.writeFileSync(path.join(moduleA, 'index.js'), + 'module.exports = {b: require(\'moduleB\')};', 'utf8'); +fs.writeFileSync(path.join(moduleB, 'index.js'), + 'module.exports = {a: require(\'moduleA\')};', 'utf8'); + +// Ensure that the symlinks are not followed forever... +const obj = require(path.join(tmpDir, 'index')); +assert.ok(obj); +assert.ok(obj.b); +assert.ok(obj.b.a); +assert.ok(!obj.b.a.b); diff --git a/test/parallel/test-module-symlinked-peer-modules.js b/test/parallel/test-module-symlinked-peer-modules.js new file mode 100644 index 00000000000000..83aca75ed197a8 --- /dev/null +++ b/test/parallel/test-module-symlinked-peer-modules.js @@ -0,0 +1,65 @@ +// Flags: --preserve-symlinks +'use strict'; +// Refs: https://github.com/nodejs/node/pull/5950 + +// This test verifies that symlinked modules are able to find their peer +// dependencies when using the --preserve-symlinks command line flag. + +// This test passes in v6.2+ with --preserve-symlinks on and in v6.0/v6.1. +// This test will fail in Node.js v4 and v5 and should not be backported. + +const common = require('../common'); +const fs = require('fs'); +const path = require('path'); +const assert = require('assert'); + +common.refreshTmpDir(); + +const tmpDir = common.tmpDir; + +// Creates the following structure +// {tmpDir} +// ├── app +// │ ├── index.js +// │ └── node_modules +// │ ├── moduleA -> {tmpDir}/moduleA +// │ └── moduleB +// │ ├── index.js +// │ └── package.json +// └── moduleA +// ├── index.js +// └── package.json + +const moduleA = path.join(tmpDir, 'moduleA'); +const app = path.join(tmpDir, 'app'); +const moduleB = path.join(app, 'node_modules', 'moduleB'); +const moduleA_link = path.join(app, 'node_modules', 'moduleA'); +fs.mkdirSync(moduleA); +fs.mkdirSync(app); +fs.mkdirSync(path.join(app, 'node_modules')); +fs.mkdirSync(moduleB); + +// Attempt to make the symlink. If this fails due to lack of sufficient +// permissions, the test will bail out and be skipped. +try { + fs.symlinkSync(moduleA, moduleA_link); +} catch (err) { + if (err.code !== 'EPERM') throw err; + common.skip('insufficient privileges for symlinks'); + return; +} + +fs.writeFileSync(path.join(moduleA, 'package.json'), + JSON.stringify({name: 'moduleA', main: 'index.js'}), 'utf8'); +fs.writeFileSync(path.join(moduleA, 'index.js'), + 'module.exports = require(\'moduleB\');', 'utf8'); +fs.writeFileSync(path.join(app, 'index.js'), + '\'use strict\'; require(\'moduleA\');', 'utf8'); +fs.writeFileSync(path.join(moduleB, 'package.json'), + JSON.stringify({name: 'moduleB', main: 'index.js'}), 'utf8'); +fs.writeFileSync(path.join(moduleB, 'index.js'), + 'module.exports = 1;', 'utf8'); + +assert.doesNotThrow(() => { + require(path.join(app, 'index')); +}); diff --git a/test/parallel/test-require-symlink.js b/test/parallel/test-require-symlink.js index 6b716ffa1381ac..d72805436a3f10 100644 --- a/test/parallel/test-require-symlink.js +++ b/test/parallel/test-require-symlink.js @@ -1,3 +1,4 @@ +// Flags: --preserve-symlinks 'use strict'; const common = require('../common'); const assert = require('assert'); @@ -49,7 +50,7 @@ function test() { // load symlinked-script as main var node = process.execPath; - var child = spawn(node, [linkScript]); + var child = spawn(node, ['--preserve-symlinks', linkScript]); child.on('close', function(code, signal) { assert(!code); assert(!signal);