src,module: add --preserve-symlinks command line flag

jasnell · evanlucas · commit 95b7560d8e5b · 2016-05-17T08:52:27.000-05:00
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: #6537 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -41,7 +41,10 @@ void InitConfig(Local<Object> 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
@@ -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
@@ -0,0 +1,13 @@
+#include <node.h>
+#include <v8.h>
+
+void Method(const v8::FunctionCallbackInfo<v8::Value>& args) {
+  v8::Isolate* isolate = args.GetIsolate();
+  args.GetReturnValue().Set(v8::String::NewFromUtf8(isolate, "world"));
+}
+
+void init(v8::Local<v8::Object> 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
