worker: file creation races with process.umask()

[bnoordhuis]

This code is unsafe when worker threads are active:

node/src/node_process_methods.cc

Lines 248 to 249 in 40b559a

	old = umask(0);
	umask(static_cast<mode_t>(old));

The umask(0) call temporarily changes the process-wide umask and races with fs operations from other threads.

Test case:

'use strict';
const { Worker, isMainThread } = require('worker_threads');
const { statSync, writeFileSync, unlinkSync } = require('fs');

function pummel() {
  for (let i = 0; i < 1e4; i++) process.umask();
  setImmediate(pummel);
}

if (isMainThread) {
  process.umask(0o22);
  new Worker(__filename);
  pummel();
} else {
  const file = 'x.txt';
  for (;;) {
    writeFileSync(file, 'ok', { mode: 0o666 });
    const s = statSync(file);
    s.mode &= 0o777;
    if (0o644 !== s.mode) throw 'unexpected mode: ' + s.mode.toString(8);
    unlinkSync(file);
  }
}

Fails within a few iterations with unexpected mode: 666

process.umask() (no arg) is allowed in workers so this test case works both ways.

This bug is potentially a security issue.

[addaleax]

Fwiw, this seems problematic even without worker threads, because it also affects fs operations running on the libuv thread pool?

[bnoordhuis]

Yes, that's right. I could swear there was a warning about that in the documentation at one point (at least about changing the umask) but not anymore.

IMO, process.umask() (no arg) is fundamentally broken and insecure without any way of making it safe. I think it's best to deprecate and remove it posthaste.

process.umask(mode) is less irredeemably broken but still hard to use securely the moment more than one thread is running (which is basically always.)

[bnoordhuis]

I'm reopening this. It's deprecated now but the goal is to remove it entirely.