vfs: fix readlink, realpath and mkdtemp encoding

On a mounted VFS, `readlink()`, `realpath()` and `mkdtemp()` only
returned a Buffer for `{ encoding: 'buffer' }`. The string form
`'buffer'`, any other encoding, and a Buffer `mkdtemp()` prefix were
ignored. The `VirtualFileSystem` `readlink` and `realpath` methods
ignored the encoding with `MemoryProvider` and passed it on to
`RealFSProvider`, which threw `ERR_INVALID_ARG_TYPE` for `'buffer'`,
while for other encodings `realpath()` failed and `readlink()`
returned the host path of absolute targets.

Get these paths from the provider as strings and apply the encoding
to the result, normalizing the options with `getOptions()` and
converting with `encodeRealpathResult()` as `fs` does. The latter
moves to `internal/fs/utils` so the VFS can use it.

Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/66352
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

lib/fs.js

@@ -102,6 +102,7 @@ const {
   collectRecursiveReaddirResult,
   copyObject,
   Dirent,
+  encodeRealpathResult,
   getDirents,
   getRecursiveDirents,
   getOptions,
@@ -3319,16 +3320,6 @@ if (isWindows) {
 
 }
 
-function encodeRealpathResult(result, options) {
-  if (!options || !options.encoding || options.encoding === 'utf8')
-    return result;
-  const asBuffer = Buffer.from(result);
-  if (options.encoding === 'buffer') {
-    return asBuffer;
-  }
-  return asBuffer.toString(options.encoding);
-}
-
 // Finds the next portion of a (partial) path, up to the next path delimiter
 let nextPart;
 if (isWindows) {

lib/internal/fs/utils.js

@@ -427,6 +427,16 @@ function getOptions(options, defaultOptions = kEmptyObject) {
   return options;
 }
 
+function encodeRealpathResult(result, options) {
+  if (!options || !options.encoding || options.encoding === 'utf8')
+    return result;
+  const asBuffer = Buffer.from(result);
+  if (options.encoding === 'buffer') {
+    return asBuffer;
+  }
+  return asBuffer.toString(options.encoding);
+}
+
 /**
  * @param {InternalFSBinding.FSSyncContext} ctx
  */
@@ -1209,6 +1219,7 @@ module.exports = {
   copyObject,
   Dirent,
   DirentFromStats,
+  encodeRealpathResult,
   getDirent,
   getDirents,
   getOptions,

lib/internal/vfs/file_system.js

@@ -35,6 +35,7 @@ const {
   createEBUSY,
   createEISDIR,
 } = require('internal/vfs/errors');
+const { encodeRealpathResult, getOptions } = require('internal/fs/utils');
 const { VirtualReadStream, VirtualWriteStream } = require('internal/vfs/streams');
 const { VirtualDir } = require('internal/vfs/dir');
 const { emitExperimentalWarning, kEmptyObject } = require('internal/util');
@@ -457,24 +458,26 @@ class VirtualFileSystem {
   /**
    * Gets the real path by resolving all symlinks.
    * @param {string} filePath The path
-   * @param {object} [options] Options
-   * @returns {string}
+   * @param {object|string} [options] Options or encoding
+   * @returns {string|Buffer}
    */
   realpathSync(filePath, options) {
+    options = getOptions(options);
     const providerPath = this.#toProviderPath(filePath);
-    const realProviderPath = this[kProvider].realpathSync(providerPath, options);
-    return this.#toMountedPath(realProviderPath);
+    const realProviderPath = this[kProvider].realpathSync(providerPath);
+    return encodeRealpathResult(this.#toMountedPath(realProviderPath), options);
   }
 
   /**
    * Reads the target of a symbolic link.
    * @param {string} linkPath The symlink path
-   * @param {object} [options] Options
-   * @returns {string}
+   * @param {object|string} [options] Options or encoding
+   * @returns {string|Buffer}
    */
   readlinkSync(linkPath, options) {
+    options = getOptions(options);
     const providerPath = this.#toProviderPath(linkPath);
-    return this[kProvider].readlinkSync(providerPath, options);
+    return encodeRealpathResult(this[kProvider].readlinkSync(providerPath), options);
   }
 
   /**
@@ -825,7 +828,7 @@ class VirtualFileSystem {
   /**
    * Gets the real path asynchronously.
    * @param {string} filePath The path
-   * @param {object|Function} [options] Options or callback
+   * @param {object|string|Function} [options] Options, encoding, or callback
    * @param {Function} [callback] Callback (err, resolvedPath)
    */
   realpath(filePath, options, callback) {
@@ -833,16 +836,17 @@ class VirtualFileSystem {
       callback = options;
       options = undefined;
     }
+    options = getOptions(options);
 
-    this[kProvider].realpath(this.#toProviderPath(filePath), options)
-      .then((realPath) => callback(null, this.#toMountedPath(realPath)),
+    this[kProvider].realpath(this.#toProviderPath(filePath))
+      .then((realPath) => callback(null, encodeRealpathResult(this.#toMountedPath(realPath), options)),
             (err) => callback(err));
   }
 
   /**
    * Reads symlink target asynchronously.
    * @param {string} linkPath The symlink path
-   * @param {object|Function} [options] Options or callback
+   * @param {object|string|Function} [options] Options, encoding, or callback
    * @param {Function} [callback] Callback (err, target)
    */
   readlink(linkPath, options, callback) {
@@ -850,9 +854,10 @@ class VirtualFileSystem {
       callback = options;
       options = undefined;
     }
+    options = getOptions(options);
 
-    this[kProvider].readlink(this.#toProviderPath(linkPath), options)
-      .then((target) => callback(null, target), (err) => callback(err));
+    this[kProvider].readlink(this.#toProviderPath(linkPath))
+      .then((target) => callback(null, encodeRealpathResult(target, options)), (err) => callback(err));
   }
 
   /**
@@ -1265,13 +1270,15 @@ class VirtualFileSystem {
       },
 
       async realpath(filePath, options) {
+        options = getOptions(options);
         const providerPath = toProviderPath(filePath);
-        return toMountedPath(await provider.realpath(providerPath, options));
+        return encodeRealpathResult(toMountedPath(await provider.realpath(providerPath)), options);
       },
 
       async readlink(linkPath, options) {
+        options = getOptions(options);
         const providerPath = toProviderPath(linkPath);
-        return provider.readlink(providerPath, options);
+        return encodeRealpathResult(await provider.readlink(providerPath), options);
       },
 
       async symlink(target, path, type) {

lib/internal/vfs/setup.js

@@ -37,7 +37,12 @@ const {
 } = require('internal/vfs/router');
 const { ReservedRootProvider } = require('internal/vfs/root');
 const { getVirtualFd, closeVirtualFd, createVfsFileHandle } = require('internal/vfs/fd');
-const { assertEncoding, setVfsHandlers } = require('internal/fs/utils');
+const {
+  assertEncoding,
+  encodeRealpathResult,
+  getOptions,
+  setVfsHandlers,
+} = require('internal/fs/utils');
 const permission = require('internal/process/permission');
 const { getOptionValue } = require('internal/options');
 const nativeModulesBinding = internalBinding('modules');
@@ -73,6 +78,14 @@ function toWriteBuffer(data, options) {
   return Buffer.from(data, encoding || 'utf8');
 }
 
+function getMkdtempOptions(prefix, options) {
+  options = getOptions(options);
+  if (Buffer.isBuffer(prefix)) {
+    options = { __proto__: null, ...options, encoding: 'buffer' };
+  }
+  return options;
+}
+
 function writeFileSyncFd(fd, data, options) {
   const vfd = getVirtualFd(fd);
   if (vfd === undefined) return undefined;
@@ -433,11 +446,7 @@ function createVfsHandlers() {
       return vfsRead(path, 'stat', (vfs, n) => vfs.statSync(n, options));
     },
     realpathSync(path, options) {
-      const result = vfsRead(path, 'realpath', (vfs, n) => vfs.realpathSync(n));
-      if (result !== undefined && options?.encoding === 'buffer') {
-        return Buffer.from(result);
-      }
-      return result;
+      return vfsRead(path, 'realpath', (vfs, n) => vfs.realpathSync(n, options));
     },
     accessSync(path, mode) {
       const pathStr = toPathStr(path);
@@ -458,9 +467,7 @@ function createVfsHandlers() {
       if (pathStr === null) return undefined;
       const r = findVFS(pathStr);
       if (r === null) return undefined;
-      const result = r.vfs.readlinkSync(pathStr, options);
-      if (options?.encoding === 'buffer') return Buffer.from(result);
-      return result;
+      return r.vfs.readlinkSync(pathStr, options);
     },
     statfsSync(path, options) {
       const pathStr = toPathStr(path);
@@ -526,11 +533,10 @@ function createVfsHandlers() {
       });
     },
     mkdtempSync(prefix, options) {
-      const result = vfsOp(prefix, (vfs, n) => vfs.mkdtempSync(n));
-      if (result !== undefined && options?.encoding === 'buffer') {
-        return Buffer.from(result);
-      }
-      return result;
+      return vfsOp(prefix, (vfs, n) => {
+        options = getMkdtempOptions(prefix, options);
+        return encodeRealpathResult(vfs.mkdtempSync(n), options);
+      });
     },
     mkstempSync(prefix, options) {
       const result = vfsOp(prefix, (vfs, n) => vfs.mkstempSync(n));
@@ -768,11 +774,7 @@ function createVfsHandlers() {
       return vfsOp(path, (vfs, n) => vfs.promises.readFile(n, options));
     },
     realpath(path, options) {
-      const promise = vfsOp(path, (vfs, n) => vfs.promises.realpath(n, options));
-      if (promise !== undefined && options?.encoding === 'buffer') {
-        return promise.then((result) => Buffer.from(result));
-      }
-      return promise;
+      return vfsOp(path, (vfs, n) => vfs.promises.realpath(n, options));
     },
     access(path, mode) {
       return vfsOp(path, (vfs, n) => {
@@ -787,11 +789,7 @@ function createVfsHandlers() {
       if (pathStr === null) return undefined;
       const r = findVFS(pathStr);
       if (r === null) return undefined;
-      const promise = r.vfs.promises.readlink(pathStr, options);
-      if (options?.encoding === 'buffer') {
-        return promise.then((result) => Buffer.from(result));
-      }
-      return promise;
+      return r.vfs.promises.readlink(pathStr, options);
     },
     chown: (path, uid, gid) =>
       vfsOp(path, (vfs, n) => vfs.promises.chown(n, uid, gid).then(() => true)),
@@ -863,11 +861,10 @@ function createVfsHandlers() {
       });
     },
     mkdtemp(prefix, options) {
-      const promise = vfsOp(prefix, (vfs, n) => vfs.promises.mkdtemp(n));
-      if (promise !== undefined && options?.encoding === 'buffer') {
-        return promise.then((result) => Buffer.from(result));
-      }
-      return promise;
+      return vfsOp(prefix, (vfs, n) => {
+        options = getMkdtempOptions(prefix, options);
+        return vfs.promises.mkdtemp(n).then((result) => encodeRealpathResult(result, options));
+      });
     },
     // Wrapped in async functions so provider throws become rejections.
     mkstemp(prefix, options) {

test/parallel/test-vfs-path-encoding.js

@@ -0,0 +1,104 @@
+// Flags: --disable-warning=ExperimentalWarning
+'use strict';
+
+// readlink(), realpath() and mkdtemp() on a mounted VFS apply the encoding
+// option the way fs does: given as a string or in an object, 'buffer' or any
+// other encoding, and a Buffer mkdtemp() prefix always gives a Buffer. The
+// VirtualFileSystem readlink and realpath methods do the same.
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const { promisify } = require('util');
+const vfs = require('node:vfs');
+const tmpdir = require('../common/tmpdir');
+
+function decode(value, encoding) {
+  if (encoding === 'buffer') {
+    assert.ok(Buffer.isBuffer(value));
+    return value.toString();
+  }
+  assert.strictEqual(typeof value, 'string');
+  return Buffer.from(value, encoding).toString();
+}
+
+const memoryVfs = vfs.create();
+memoryVfs.writeFileSync('/file.txt', 'x');
+memoryVfs.symlinkSync('file.txt', '/link');
+memoryVfs.symlinkSync('/file.txt', '/abs-link');
+const vfsInstances = [memoryVfs];
+
+if (common.canCreateSymLink()) {
+  tmpdir.refresh();
+  fs.writeFileSync(tmpdir.resolve('file.txt'), 'x');
+  fs.symlinkSync('file.txt', tmpdir.resolve('link'));
+  // RealFSProvider returns an absolute target inside its root as a VFS path.
+  fs.symlinkSync(tmpdir.resolve('file.txt'), tmpdir.resolve('abs-link'));
+  vfsInstances.push(vfs.create(new vfs.RealFSProvider(tmpdir.path)));
+}
+
+const readlinks = [
+  fs.readlinkSync,
+  promisify(fs.readlink),
+  fs.promises.readlink,
+];
+const realpaths = [
+  fs.realpathSync,
+  fs.realpathSync.native,
+  promisify(fs.realpath),
+  promisify(fs.realpath.native),
+  fs.promises.realpath,
+];
+const mkdtemps = [
+  fs.mkdtempSync,
+  promisify(fs.mkdtemp),
+  fs.promises.mkdtemp,
+];
+
+(async () => {
+  for (const myVfs of vfsInstances) {
+    const mountPoint = myVfs.mount();
+    const prefix = path.join(mountPoint, 'tmp-');
+    const vfsReadlinks = [
+      myVfs.readlinkSync.bind(myVfs),
+      promisify(myVfs.readlink.bind(myVfs)),
+      myVfs.promises.readlink,
+    ];
+    const vfsRealpaths = [
+      myVfs.realpathSync.bind(myVfs),
+      promisify(myVfs.realpath.bind(myVfs)),
+      myVfs.promises.realpath,
+    ];
+
+    for (const options of ['buffer', { encoding: 'buffer' }, 'hex', { encoding: 'hex' }]) {
+      const encoding = options.encoding ?? options;
+
+      for (const name of ['link', 'abs-link']) {
+        const link = path.join(mountPoint, name);
+        for (const readlink of [...readlinks, ...vfsReadlinks]) {
+          assert.strictEqual(decode(await readlink(link, options), encoding),
+                             fs.readlinkSync(link));
+        }
+        for (const realpath of [...realpaths, ...vfsRealpaths]) {
+          assert.strictEqual(decode(await realpath(link, options), encoding),
+                             fs.realpathSync(link));
+        }
+      }
+
+      for (const mkdtemp of mkdtemps) {
+        const dir = decode(await mkdtemp(prefix, options), encoding);
+        assert.ok(dir.startsWith(prefix));
+        assert.ok(fs.statSync(dir).isDirectory());
+      }
+    }
+
+    for (const mkdtemp of mkdtemps) {
+      const dir = await mkdtemp(Buffer.from(prefix), 'hex');
+      assert.ok(Buffer.isBuffer(dir));
+      assert.ok(fs.statSync(dir).isDirectory());
+    }
+
+    myVfs.unmount();
+  }
+})().then(common.mustCall());