streamich / memfs

JavaScript file system utilities
http://streamich.github.io/memfs/
Apache License 2.0
1.76k stars 129 forks source link

Side effect when mocking `process.getuid()` under Windows #809

Closed aleen42 closed 2 years ago

aleen42 commented 2 years ago

Recently I have found that when I imported memfs/lib/process.js and used gulp to copy a directory with a subdirectory, I got an error Error: EPERM: operation not permitted, futime.

dest
test
│   index.js
└───sub
    │   index.js

Here is the snippet to show how to reproduce the problem:

// test/index.js
const path = require('path');
const gulp = require('gulp');

// require('memfs/lib/process.js');
process.getuid = process.getuid || (() => 0);
gulp.src(['**/*']).pipe(gulp.dest(path.resolve(__dirname, '../dest')))

It seems that it is a mocked uid resulting in side effects when trying to futime.

The temporary workaround is to revert it within the current process:

require('memfs');
process.platform === 'win32' && delete process.getuid;
G-Rath commented 2 years ago

Right, this looks to be because getuid isn't actually present - it doesn't look like we're using it ourselves though.

It's debatable where the bug actually lies: us shiming getuid by itself is not an issue, but gulp must be seeing that and assumes it's on Linux

https://gulpjs.com/docs/en/api/dest/#metadata-updates:

Whenever the dest() stream creates a file, the Vinyl object's mode, mtime, and atime are compared to the created file. If they differ, the created file will be updated to reflect the Vinyl object's metadata. If those properties are the same, or gulp doesn't have permissions to make changes, the attempt is skipped silently.

This functionality is disabled on Windows or other operating systems that don't support Node's process.getuid() or process.geteuid() methods. This is due to Windows having unexpected results through usage of fs.fchmod() and fs.futimes().

It looks like we're using that method here: https://github.com/streamich/memfs/blob/8523ceca916918d0cfe324766ce9c1a07f0a650e/src/node.ts#L20

It could be argued that removing our polyfill is a breaking change, but it's not something we advertise as being a feature (rather it's an internal fallback for us) so we could probably replace it with process.getuid?.() ?? 0 🤔

aleen42 commented 2 years ago

Yes, do not modify the method directly is always a better practice. What memfs needs is to just avoid the NPE exception when accessing process.getuid() in Windows.

G-Rath commented 2 years ago

PRs are always welcome :)

streamich commented 2 years ago

:tada: This issue has been resolved in version 3.4.7 :tada:

The release is available on:

Your semantic-release bot :package::rocket: