fix(plugin-pnpm) Pnpm linker avoids symlink loops on the file system (y…

…arnpkg#4542) * Pnpm linker avoids symlink loops on the file system * symlinkDst -> symlinkDstPath for uniformity with nearby code * Avoids self-require trap * Store paths rather than linkType, and remove now-irrelevant cleanup code * Adds test * Don't create self-references for workspaces, to avoid infinite loops Co-authored-by: Maël Nison <[email protected]>
jakutis · Aug 11, 2022 · 6fbc4a4 · 6fbc4a4
1 parent 7a42886
commit 6fbc4a4
Show file tree

Hide file tree

Showing 4 changed files with 151 additions and 73 deletions.
diff --git a/.yarn/versions/fbc424b9.yml b/.yarn/versions/fbc424b9.yml
@@ -0,0 +1,23 @@
+releases:
+  "@yarnpkg/cli": patch
+  "@yarnpkg/plugin-pnpm": patch
+
+declined:
+  - "@yarnpkg/plugin-compat"
+  - "@yarnpkg/plugin-constraints"
+  - "@yarnpkg/plugin-dlx"
+  - "@yarnpkg/plugin-essentials"
+  - "@yarnpkg/plugin-init"
+  - "@yarnpkg/plugin-interactive-tools"
+  - "@yarnpkg/plugin-nm"
+  - "@yarnpkg/plugin-npm-cli"
+  - "@yarnpkg/plugin-pack"
+  - "@yarnpkg/plugin-patch"
+  - "@yarnpkg/plugin-pnp"
+  - "@yarnpkg/plugin-stage"
+  - "@yarnpkg/plugin-typescript"
+  - "@yarnpkg/plugin-version"
+  - "@yarnpkg/plugin-workspace-tools"
+  - "@yarnpkg/builder"
+  - "@yarnpkg/core"
+  - "@yarnpkg/doctor"
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,10 @@ Yarn now accepts sponsorships! Please give a look at our [OpenCollective](https:
 
 ## 3.2.2
 
+### Installs
+
+- The `pnpm` linker avoids creating symlinks that lead to loops on the file system, by moving them higher up in the directory structure.
+
 ### Compatibility
 
 - The patched filesystem now supports `ftruncate`.

diff --git a/packages/acceptance-tests/pkg-tests-specs/sources/features/pnpm.test.ts b/packages/acceptance-tests/pkg-tests-specs/sources/features/pnpm.test.ts
@@ -0,0 +1,36 @@
+import {PortablePath, ppath, xfs} from '@yarnpkg/fslib';
+
+describe(`Features`, () => {
+  describe(`Pnpm Mode `, () => {
+    test(
+      `it shouldn't crash if we recursively traverse a node_modules`,
+      makeTemporaryEnv({
+        dependencies: {
+          [`no-deps`]: `1.0.0`,
+        },
+      }, {
+        nodeLinker: `pnpm`,
+      }, async ({path, run, source}) => {
+        await run(`install`);
+
+        let iterationCount = 0;
+
+        const getRecursiveDirectoryListing = async (p: PortablePath) => {
+          if (iterationCount++ > 500)
+            throw new Error(`Possible infinite recursion detected`);
+
+          for (const entry of await xfs.readdirPromise(p)) {
+            const entryPath = ppath.join(p, entry);
+            const stat = await xfs.statPromise(entryPath);
+
+            if (stat.isDirectory()) {
+              await getRecursiveDirectoryListing(entryPath);
+            }
+          }
+        };
+
+        await getRecursiveDirectoryListing(path);
+      }),
+    );
+  });
+});
diff --git a/packages/plugin-pnpm/sources/PnpmLinker.ts b/packages/plugin-pnpm/sources/PnpmLinker.ts
@@ -4,11 +4,21 @@ import {jsInstallUtils}
 import {UsageError}                                                                                                                                                                                                        from 'clipanion';
 
 export type PnpmCustomData = {
-  pathByLocator: Map<LocatorHash, PortablePath>;
   locatorByPath: Map<PortablePath, string>;
+  pathsByLocator: Map<LocatorHash, {
+    packageLocation: PortablePath;
+    dependenciesLocation: PortablePath | null;
+  }>;
 };
 
 export class PnpmLinker implements Linker {
+  getCustomDataKey() {
+    return JSON.stringify({
+      name: `PnpmLinker`,
+      version: 3,
+    });
+  }
+
   supportsPackage(pkg: Package, opts: MinimalLinkOptions) {
     return this.isEnabled(opts);
   }
@@ -22,11 +32,11 @@ export class PnpmLinker implements Linker {
     if (!customData)
       throw new UsageError(`The project in ${formatUtils.pretty(opts.project.configuration, `${opts.project.cwd}/package.json`, formatUtils.Type.PATH)} doesn't seem to have been installed - running an install there might help`);
 
-    const packageLocation = customData.pathByLocator.get(locator.locatorHash);
-    if (typeof packageLocation === `undefined`)
+    const packagePaths = customData.pathsByLocator.get(locator.locatorHash);
+    if (typeof packagePaths === `undefined`)
       throw new UsageError(`Couldn't find ${structUtils.prettyLocator(opts.project.configuration, locator)} in the currently installed pnpm map - running an install might help`);
 
-    return packageLocation;
+    return packagePaths.packageLocation;
   }
 
   async findPackageLocator(location: PortablePath, opts: LinkOptions): Promise<Locator | null> {
@@ -82,7 +92,7 @@ class PnpmInstaller implements Installer {
   }
 
   private customData: PnpmCustomData = {
-    pathByLocator: new Map(),
+    pathsByLocator: new Map(),
     locatorByPath: new Map(),
   };
 
@@ -101,27 +111,36 @@ class PnpmInstaller implements Installer {
   }
 
   async installPackageSoft(pkg: Package, fetchResult: FetchResult, api: InstallPackageExtraApi) {
-    const pkgPath = ppath.resolve(fetchResult.packageFs.getRealPath(), fetchResult.prefixPath);
-    this.customData.pathByLocator.set(pkg.locatorHash, pkgPath);
+    const packageLocation = ppath.resolve(fetchResult.packageFs.getRealPath(), fetchResult.prefixPath);
+
+    const dependenciesLocation = this.opts.project.tryWorkspaceByLocator(pkg)
+      ? ppath.join(packageLocation, Filename.nodeModules)
+      : null;
+
+    this.customData.pathsByLocator.set(pkg.locatorHash, {
+      packageLocation,
+      dependenciesLocation,
+    });
 
     return {
-      packageLocation: pkgPath,
+      packageLocation,
       buildDirective: null,
     };
   }
 
   async installPackageHard(pkg: Package, fetchResult: FetchResult, api: InstallPackageExtraApi) {
-    const pkgPath = getPackageLocation(pkg, {project: this.opts.project});
+    const packagePaths = getPackagePaths(pkg, {project: this.opts.project});
+    const packageLocation = packagePaths.packageLocation;
 
-    this.customData.locatorByPath.set(pkgPath, structUtils.stringifyLocator(pkg));
-    this.customData.pathByLocator.set(pkg.locatorHash, pkgPath);
+    this.customData.locatorByPath.set(packageLocation, structUtils.stringifyLocator(pkg));
+    this.customData.pathsByLocator.set(pkg.locatorHash, packagePaths);
 
     api.holdFetchResult(this.asyncActions.set(pkg.locatorHash, async () => {
-      await xfs.mkdirPromise(pkgPath, {recursive: true});
+      await xfs.mkdirPromise(packageLocation, {recursive: true});
 
       // Copy the package source into the <root>/n_m/.store/<hash> directory, so
       // that we can then create symbolic links to it later.
-      await xfs.copyPromise(pkgPath, fetchResult.prefixPath, {
+      await xfs.copyPromise(packageLocation, fetchResult.prefixPath, {
         baseFs: fetchResult.packageFs,
         overwrite: false,
       });
@@ -141,7 +160,7 @@ class PnpmInstaller implements Installer {
     const buildScripts = jsInstallUtils.extractBuildScripts(pkg, buildConfig, dependencyMeta, {configuration: this.opts.project.configuration, report: this.opts.report});
 
     return {
-      packageLocation: pkgPath,
+      packageLocation,
       buildDirective: buildScripts,
     };
   }
@@ -154,39 +173,44 @@ class PnpmInstaller implements Installer {
     if (!isPnpmVirtualCompatible(locator, {project: this.opts.project}))
       return;
 
-    this.asyncActions.reduce(locator.locatorHash, async action => {
-      // Wait that the package is properly installed before starting to copy things into it
-      await action;
+    const packagePaths = this.customData.pathsByLocator.get(locator.locatorHash);
+    if (typeof packagePaths === `undefined`)
+      throw new Error(`Assertion failed: Expected the package to have been registered (${structUtils.stringifyLocator(locator)})`);
 
-      const pkgPath = this.customData.pathByLocator.get(locator.locatorHash);
-      if (typeof pkgPath === `undefined`)
-        throw new Error(`Assertion failed: Expected the package to have been registered (${structUtils.stringifyLocator(locator)})`);
+    const {
+      dependenciesLocation,
+    } = packagePaths;
 
-      const nmPath = ppath.join(pkgPath, Filename.nodeModules);
+    if (!dependenciesLocation)
+      return;
 
-      const concurrentPromises: Array<Promise<void>> = [];
+    this.asyncActions.reduce(locator.locatorHash, async action => {
+      await xfs.mkdirPromise(dependenciesLocation, {recursive: true});
 
       // Retrieve what's currently inside the package's true nm folder. We
       // will use that to figure out what are the extraneous entries we'll
       // need to remove.
-      const extraneous = await getNodeModulesListing(nmPath);
+      const initialEntries = await getNodeModulesListing(dependenciesLocation);
+      const extraneous = new Map(initialEntries);
 
-      for (const [descriptor, dependency] of dependencies) {
+      const concurrentPromises: Array<Promise<void>> = [action];
+
+      const installDependency = (descriptor: Descriptor, dependency: Locator) => {
         // Downgrade virtual workspaces (cf isPnpmVirtualCompatible's documentation)
         let targetDependency = dependency;
         if (!isPnpmVirtualCompatible(dependency, {project: this.opts.project})) {
           this.opts.report.reportWarning(MessageName.UNNAMED, `The pnpm linker doesn't support providing different versions to workspaces' peer dependencies`);
           targetDependency = structUtils.devirtualizeLocator(dependency);
         }
 
-        const depSrcPath = this.customData.pathByLocator.get(targetDependency.locatorHash);
-        if (typeof depSrcPath === `undefined`)
+        const depSrcPaths = this.customData.pathsByLocator.get(targetDependency.locatorHash);
+        if (typeof depSrcPaths === `undefined`)
           throw new Error(`Assertion failed: Expected the package to have been registered (${structUtils.stringifyLocator(dependency)})`);
 
         const name = structUtils.stringifyIdent(descriptor) as PortablePath;
-        const depDstPath = ppath.join(nmPath, name);
+        const depDstPath = ppath.join(dependenciesLocation, name);
 
-        const depLinkPath = ppath.relative(ppath.dirname(depDstPath), depSrcPath);
+        const depLinkPath = ppath.relative(ppath.dirname(depDstPath), depSrcPaths.packageLocation);
 
         const existing = extraneous.get(name);
         extraneous.delete(name);
@@ -204,14 +228,25 @@ class PnpmInstaller implements Installer {
           await xfs.mkdirpPromise(ppath.dirname(depDstPath));
 
           if (process.platform == `win32`) {
-            await xfs.symlinkPromise(depSrcPath, depDstPath, `junction`);
+            await xfs.symlinkPromise(depSrcPaths.packageLocation, depDstPath, `junction`);
           } else {
             await xfs.symlinkPromise(depLinkPath, depDstPath);
           }
         }));
+      };
+
+      let hasExplicitSelfDependency = false;
+      for (const [descriptor, dependency] of dependencies) {
+        if (descriptor.identHash === locator.identHash)
+          hasExplicitSelfDependency = true;
+
+        installDependency(descriptor, dependency);
       }
 
-      concurrentPromises.push(cleanNodeModules(nmPath, extraneous));
+      if (!hasExplicitSelfDependency && !this.opts.project.tryWorkspaceByLocator(locator))
+        installDependency(structUtils.convertLocatorToDescriptor(locator), locator);
+
+      concurrentPromises.push(cleanNodeModules(dependenciesLocation, extraneous));
 
       await Promise.all(concurrentPromises);
     });
@@ -227,50 +262,28 @@ class PnpmInstaller implements Installer {
     if (this.opts.project.configuration.get(`nodeLinker`) !== `pnpm`) {
       await xfs.removePromise(storeLocation);
     } else {
-      const removals: Array<Promise<void>> = [];
-
-      const expectedEntries = new Set<Filename>();
-      for (const packageLocation of this.customData.pathByLocator.values()) {
-        const subpath = ppath.contains(storeLocation, packageLocation);
-        if (subpath !== null) {
-          const [storeEntry, /* Filename.nodeModules */, ...identComponents] = subpath.split(ppath.sep);
-          expectedEntries.add(storeEntry as Filename);
-
-          const storeEntryPath = ppath.join(storeLocation, storeEntry as Filename);
-
-          removals.push(xfs.readdirPromise(storeEntryPath)
-            .then(entries => {
-              return Promise.all(entries.map(async entry => {
-                const p = ppath.join(storeEntryPath, entry);
-                if (entry === Filename.nodeModules) {
-                  const extraneous = await getNodeModulesListing(p);
-                  extraneous.delete(identComponents.join(ppath.sep) as PortablePath);
-                  return cleanNodeModules(p, extraneous);
-                } else {
-                  return xfs.removePromise(p);
-                }
-              }));
-            })
-            .catch(error => {
-              if (error.code !== `ENOENT`) {
-                throw error;
-              }
-            }) as Promise<void>);
-        }
-      }
-
-      let storeRecords: Array<Filename>;
+      let extraneous: Set<Filename>;
       try {
-        storeRecords = await xfs.readdirPromise(storeLocation);
+        extraneous = new Set(await xfs.readdirPromise(storeLocation));
       } catch {
-        storeRecords = [];
+        extraneous = new Set();
       }
 
-      for (const record of storeRecords)
-        if (!expectedEntries.has(record))
-          removals.push(xfs.removePromise(ppath.join(storeLocation, record)));
+      for (const {dependenciesLocation} of this.customData.pathsByLocator.values()) {
+        if (!dependenciesLocation)
+          continue;
+
+        const subpath = ppath.contains(storeLocation, dependenciesLocation);
+        if (subpath === null)
+          continue;
 
-      await Promise.all(removals);
+        const [storeEntry] = subpath.split(ppath.sep);
+        extraneous.delete(storeEntry as Filename);
+      }
+
+      await Promise.all([...extraneous].map(async extraneousEntry => {
+        await xfs.removePromise(ppath.join(storeLocation, extraneousEntry));
+      }));
     }
 
     // Wait for the package installs to catch up
@@ -301,12 +314,14 @@ function getStoreLocation(project: Project) {
   return ppath.join(getNodeModulesLocation(project), `.store` as Filename);
 }
 
-function getPackageLocation(locator: Locator, {project}: {project: Project}) {
+function getPackagePaths(locator: Locator, {project}: {project: Project}) {
   const pkgKey = structUtils.slugifyLocator(locator);
-  const prefixPath = structUtils.getIdentVendorPath(locator);
-  const pkgPath = ppath.join(getStoreLocation(project), pkgKey, prefixPath);
+  const storeLocation = getStoreLocation(project);
+
+  const packageLocation = ppath.join(storeLocation, pkgKey, `package` as Filename);
+  const dependenciesLocation = ppath.join(storeLocation, pkgKey, Filename.nodeModules);
 
-  return pkgPath;
+  return {packageLocation, dependenciesLocation};
 }
 
 function isPnpmVirtualCompatible(locator: Locator, {project}: {project: Project}) {