Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop using process.umask() #252

Closed
isaacs opened this issue Apr 3, 2020 · 5 comments
Closed

Stop using process.umask() #252

isaacs opened this issue Apr 3, 2020 · 5 comments

Comments

@isaacs
Copy link
Owner

isaacs commented Apr 3, 2020

Refs: nodejs/node#32321

Summary: process.umask() (no args) will be deprecated and removed.

I couldn't quite divine what lib/config/defaults.js uses process.umask() for but in most cases you don't need to deal with the umask directly - the operating system will apply it automatically.

Example:

const mode = 0o777 & ~process.umask();
fs.mkdirSync(dir, mode);

Computing the file mode that way is superfluous, it can be replaced with just this:

fs.mkdirSync(dir, 0o777);

nodejs/node#32321 (comment)

cc @bnoordhuis

@isaacs
Copy link
Owner Author

isaacs commented Apr 3, 2020

  • Add a chmod: false option to unpack. In this mode, just create the file with the entry mode, and let it ride with whatever the OS gives us.
  • If chmod:false is not set, then create the file with the entry.mode, stat the file, and chmod if different. This will be slower than the current approach, unfortunately.

@isaacs
Copy link
Owner Author

isaacs commented Apr 3, 2020

Note: even though it's potentially slower, creating the file and then checking the mode is a more technically correct approach, as it is resilient against process.umask() changing during the unpack, or returning an incorrect value.

@bnoordhuis
Copy link

FWIW, I did some quick, completely unscientific benchmarks on Linux and changing create-close to create-fchmod-close slows it down by just under 15%:

// As root, run `echo 3 > /proc/sys/vm/drop_caches` first
'use strict';
const { O_CREAT, O_WRONLY } = require('fs').constants;
const { openSync, closeSync, unlinkSync } = require('fs');
let { fchmodSync } = require('fs');
if (process.argv[2]) fchmodSync = () => {};

const N = 1e4;
const files = [];
for (let i = 0; i < N; i++) files.push('/tmp/xyzzy' + i);

const start = Date.now();
for (const file of files) {
  const fd = openSync(file, O_CREAT|O_WRONLY);
  fchmodSync(fd, 0o666);
  closeSync(fd);
}
const end = Date.now();

console.log(start, end, end - start);
for (const file of files) unlinkSync(file);

15% sounds like a lot, however:

  1. The benchmark doesn't write any data. Writing 4x4k blocks shrinks the relative gap to <5%.

  2. There is only a statistically significant difference when the cache is already warmed up.

I assume cold caches are the common case for node-tar because you don't normally unpack the same tarball to the same location 10 times in a row. (Well, I don't, anyway.)

@ql-nano
Copy link

ql-nano commented Aug 2, 2020

If you really need to use

process.umask()

(without args), then you may use get-umask module. It suppresses those annoying warnings. And yeah, it's written by me.

@isaacs
Copy link
Owner Author

isaacs commented Aug 3, 2021

Suppress warnings by using { noChmod: true }, so that it won't have to calculate anything. Otherwise, there's no way around having to get the process umask, or else it's a big security problem.

@isaacs isaacs closed this as completed Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants