install: Ensure walking the tree doesn't result in infinite loops

Fixes: #9223
PR-URL: npm/npm#9261

lib/dedupe.js

@@ -112,17 +112,29 @@ function moveRemainingChildren (node, diff) {
 }
 
 function remove (child, diff, done) {
+  remove_(child, diff, {}, done)
+}
+
+function remove_ (child, diff, seen, done) {
+  if (seen[child.path]) return done()
+  seen[child.path] = true
   diff.push(['remove', child])
   child.parent.children = without(child.parent.children, child)
   asyncMap(child.children, function (child, next) {
-    remove(child, diff, next)
+    remove_(child, diff, seen, next)
   }, done)
 }
 
 function hoistChildren (tree, diff, next) {
-  validate('OAF', arguments)
+  hoistChildren_(tree, diff, {}, next)
+}
+
+function hoistChildren_ (tree, diff, seen, next) {
+  validate('OAOF', arguments)
+  if (seen[tree.path]) return next()
+  seen[tree.path] = true
   asyncMap(tree.children, function (child, done) {
-    if (!tree.parent) return hoistChildren(child, diff, done)
+    if (!tree.parent) return hoistChildren_(child, diff, seen, done)
     var better = findRequirement(tree.parent, child.package.name, child.package._requested || npa(child.package.name + '@' + child.package.version))
     if (better) {
       return chain([
@@ -135,7 +147,7 @@ function hoistChildren (tree, diff, next) {
       move(child, hoistTo, diff)
       chain([
         [recalculateMetadata, hoistTo, log],
-        [hoistChildren, child, diff],
+        [hoistChildren_, child, diff, seen],
         [function (next) {
           moveRemainingChildren(child, diff)
           next()

lib/install.js

@@ -698,13 +698,15 @@ Installer.prototype.debugTree = function (name, treeName, cb) {
 
 Installer.prototype.prettify = function (tree) {
   validate('O', arguments)
+  var seen = {}
   function byName (aa, bb) {
     return aa.package.name.localeCompare(bb.package.name)
   }
   function expandTree (tree) {
+    seen[tree.path] = true
     return {
       label: getPackageId(tree),
-      nodes: tree.children.sort(byName).map(expandTree)
+      nodes: tree.children.filter(function (tree) { return !seen[tree.path] }).sort(byName).map(expandTree)
     }
   }
   return archy(expandTree(tree), '', { unicode: npm.config.get('unicode') })

lib/install/deps.js

@@ -74,8 +74,14 @@ function doesChildVersionMatch (child, requested) {
   return semver.satisfies(child.package.version, requested.spec)
 }
 
-var recalculateMetadata = exports.recalculateMetadata = function (tree, log, next) {
-  validate('OOF', arguments)
+exports.recalculateMetadata = function (tree, log, next) {
+  recalculateMetadata(tree, log, {}, next)
+}
+
+function recalculateMetadata (tree, log, seen, next) {
+  validate('OOOF', arguments)
+  if (seen[tree.path]) return next()
+  seen[tree.path] = true
   if (tree.parent == null) resetMetadata(tree)
   function markDeps (spec, done) {
     validate('SF', arguments)
@@ -105,7 +111,7 @@ var recalculateMetadata = exports.recalculateMetadata = function (tree, log, nex
   tree.children = tree.children.filter(function (child) { return !child.failed })
   chain([
     [asyncMap, tomark, markDeps],
-    [asyncMap, tree.children, function (child, done) { recalculateMetadata(child, log, done) }]
+    [asyncMap, tree.children, function (child, done) { recalculateMetadata(child, log, seen, done) }]
   ], function () {
     tree.userRequired = tree.package._requiredBy.some(function (req) { req === '#USER' })
     tree.existing = tree.package._requiredBy.some(function (req) { req === '#EXISTING' })
@@ -323,10 +329,16 @@ exports.loadDevDeps = function (tree, log, next) {
 }
 
 exports.loadExtraneous = function loadExtraneous (tree, log, next) {
-  validate('OOF', arguments)
-  asyncMap(tree.children.filter(function (child) { return !child.loaded }), function (child, done) {
-    resolveWithExistingModule(child, tree, log, done)
-  }, andForEachChild(loadExtraneous, andFinishTracker(log, next)))
+  var seen = {}
+  function loadExtraneous (tree, log, next) {
+    validate('OOF', arguments)
+    if (seen[tree.path]) return next()
+    seen[tree.path] = true
+    asyncMap(tree.children.filter(function (child) { return !child.loaded }), function (child, done) {
+      resolveWithExistingModule(child, tree, log, done)
+    }, andForEachChild(loadExtraneous, andFinishTracker(log, next)))
+  }
+  loadExtraneous(tree, log, next)
 }
 
 exports.loadExtraneous.andResolveDeps = function (tree, log, next) {
@@ -445,10 +457,16 @@ var validatePeerDeps = exports.validatePeerDeps = function (tree, onInvalid) {
   })
 }
 
-var validateAllPeerDeps = exports.validateAllPeerDeps = function (tree, onInvalid) {
-  validate('OF', arguments)
+exports.validateAllPeerDeps = function (tree, onInvalid) {
+  validateAllPeerDeps(tree, onInvalid, {})
+}
+
+function validateAllPeerDeps (tree, onInvalid, seen) {
+  validate('OFO', arguments)
+  if (seen[tree.path]) return
+  seen[tree.path] = true
   validatePeerDeps(tree, onInvalid)
-  tree.children.forEach(function (child) { validateAllPeerDeps(child, onInvalid) })
+  tree.children.forEach(function (child) { validateAllPeerDeps(child, onInvalid, seen) })
 }
 
 // Determine if a module requirement is already met by the tree at or above

lib/install/flatten-tree.js

@@ -3,17 +3,21 @@ var validate = require('aproba')
 
 module.exports = function (tree) {
   validate('O', arguments)
+  var seen = {}
   var flat = {}
   var todo = [[tree, '/']]
   while (todo.length) {
     var next = todo.shift()
     var pkg = next[0]
+    seen[pkg.path] = true
     var path = next[1]
     flat[path] = pkg
     if (path !== '/') path += '/'
     for (var ii = 0; ii < pkg.children.length; ++ii) {
       var child = pkg.children[ii]
-      todo.push([child, flatName(path, child)])
+      if (!seen[child.path]) {
+        todo.push([child, flatName(path, child)])
+      }
     }
   }
   return flat

lib/install/logical-tree.js

@@ -55,12 +55,18 @@ module.exports.asReadInstalled = function (tree) {
 }
 
 function translateTree (tree) {
+  return translateTree_(tree, {})
+}
+
+function translateTree_ (tree, seen) {
   var pkg = tree.package
+  if (seen[tree.path]) return pkg
+  seen[tree.path] = pkg
   if (pkg._dependencies) return pkg
   pkg._dependencies = pkg.dependencies
   pkg.dependencies = {}
   tree.children.forEach(function (child) {
-    pkg.dependencies[child.package.name] = translateTree(child)
+    pkg.dependencies[child.package.name] = translateTree_(child, seen)
   })
   Object.keys(tree.missingDeps).forEach(function (name) {
     if (pkg.dependencies[name]) {

lib/install/node.js

@@ -39,7 +39,14 @@ var create = exports.create = function (node, template) {
   }
   return node
 }
-var reset = exports.reset = function (node) {
+
+exports.reset = function (node) {
+  reset(node, {})
+}
+
+function reset (node, seen) {
+  if (seen[node.path]) return
+  seen[node.path] = true
   var child = create(node)
   child.package._requiredBy = child.package._requiredBy.filter(function (req) {
     return req[0] === '#'
@@ -49,6 +56,6 @@ var reset = exports.reset = function (node) {
   // FIXME: cleaning up after read-package-json's mess =(
   if (child.package._id === '@') delete child.package._id
   child.missingDeps = {}
-  child.children.forEach(reset)
+  child.children.forEach(function (child) { reset(child, seen) })
   if (!child.package.version) child.package.version = ''
 }

lib/shrinkwrap.js

@@ -58,8 +58,11 @@ function treeToShrinkwrap (tree, dev) {
   return pkginfo
 }
 
-function shrinkwrapDeps (dev, problems, deps, tree) {
-  validate('BAOO', arguments)
+function shrinkwrapDeps (dev, problems, deps, tree, seen) {
+  validate('BAOO', [dev, problems, deps, tree])
+  if (!seen) seen = {}
+  if (seen[tree.path]) return
+  seen[tree.path] = true
   Object.keys(tree.missingDeps).forEach(function (name) {
     var invalid = tree.children.filter(function (dep) { return dep.package.name === name })[0]
     if (invalid) {
@@ -87,7 +90,7 @@ function shrinkwrapDeps (dev, problems, deps, tree) {
         ', required by ' + child.package._id)
     })
     if (child.children.length) {
-      shrinkwrapDeps(dev, problems, pkginfo.dependencies = {}, child)
+      shrinkwrapDeps(dev, problems, pkginfo.dependencies = {}, child, seen)
     }
   })
 }

test/tap/symlink-cycle.js

@@ -0,0 +1,61 @@
+'use strict'
+var fs = require('fs')
+var path = require('path')
+var test = require('tap').test
+var mkdirp = require('mkdirp')
+var osenv = require('osenv')
+var rimraf = require('rimraf')
+var writeFileSync = require('fs').writeFileSync
+var common = require('../common-tap.js')
+
+var base = path.join(__dirname, path.basename(__filename, '.js'))
+var cycle = path.join(base, 'cycle')
+
+var cycleJSON = {
+  name: 'cycle',
+  version: '1.0.0',
+  description: '',
+  main: 'index.js',
+  scripts: {
+    test: 'echo \"Error: no test specified\" && exit 1'
+  },
+  dependencies: {
+    'cycle': '*'
+  },
+  author: '',
+  license: 'ISC'
+}
+
+test('setup', function (t) {
+  setup()
+  t.end()
+})
+
+test('ls', function (t) {
+  process.chdir(cycle)
+  common.npm(['ls'], {}, function (err, code, stdout, stderr) {
+    t.ifError(err, 'installed w/o error')
+    t.is(stderr, '', 'no warnings printed to stderr')
+    t.end()
+  })
+})
+
+test('cleanup', function (t) {
+  process.chdir(osenv.tmpdir())
+  cleanup()
+  t.end()
+})
+
+function cleanup () {
+  rimraf.sync(base)
+}
+
+function setup () {
+  cleanup()
+  mkdirp.sync(path.join(cycle, 'node_modules'))
+  writeFileSync(
+    path.join(cycle, 'package.json'),
+    JSON.stringify(cycleJSON, null, 2)
+  )
+  fs.symlinkSync(cycle, path.join(cycle, 'node_modules', 'cycle'))
+}