Stop using process.umask()

[coreyfarrell]

Ref nodejs/node#32321

---

This can result in a behavior change in an edge case:

• We attempt to create ./dir without providing customMode.
• ./dir already exists but has a difference mode than the current process.umask().

I think the current release would reset the existing directory to match the current umask where the new code will leave it alone.

[coreyfarrell]

Sorry for pushing with broken tests, I realize this cannot be merged as is but hopefully showing the test errors can help us find a way to deal with this.

[phated]

I don't really understand umask and pulled it from some other code. Nor do I really understand what is going on in the linked issues.

I assume this would be a breaking change so we probably couldn't upstream into gulp 4. Is there an interim solution?

[coreyfarrell]

I don't know an interim solution. The issue is that libc does not provide a function to "query umask". The libc function is:

// Set the umask and return the old value
mode_t umask(mode_t mask);

So process.umask() is doing the equivilent of:

const old = process.umask(0);
process.umask(old);

So there is race condition where other threads would create files using an invalid umask of 0 (all new files would have 0o000 permission).

In theory a half-broken solution would be to create our own replacement umask function:

let oldMask = 0;
function getUmask() {
  let newMask = process.umask(oldMask);
  if (newMask !== oldMask) {
    process.umask(newMask);
    oldMask = newMask;
  }

  return newMask
}

Using this getUmask would avoid the deprecation error but would still be susceptible to race conditions during initialization as well as the first time getUmask is run after someone changes umask (example if someone uses process.umask(0o600)).

[phated]

@coreyfarrell if we make this a breaking change, what do you think we should do? I just checked my default install of node and it looks like the default umask is 022, should we just use that as the mask?

[coreyfarrell]

@phated it looks like I was incorrect in my previous assessment. This is not a breaking change, the existing code does only performs chmod on existing paths if a mode is explicitly set. The tests were failing because my first commit had a bug where it was passing the wrong mode value to recursive calls. Sorry for the noise.

About the default umask, node.js does not set this. It is actually set per user profile. For example on my system the umask for my normal user is 002 and the umask for root is 022.

[coreyfarrell]

Additional note the CI failure at npm install is not caused by this PR. I tried nvm i 5 && npm i locally and got the same failure against both master and my own branch.

[phated]

@coreyfarrell I still think we are talking past each other. The point of the masking in this was to replicate the behavior of the mkdir command on linux, which sets the mode on a directory if it exists but the mode is different. Nodejs doesn't do that for mkdir on MacOS, so we have to force it. I'm pretty sure these commands mask it.

[coreyfarrell]

We have two paths for what to do when a directory already exists. When mode is provided the directory is chmod'ed (see https://github.com/gulpjs/fs-mkdirp-stream/blob/master/test/mkdirp.js#L201). When mode is not provided the existing mode is left alone (see https://github.com/gulpjs/fs-mkdirp-stream/blob/master/test/mkdirp.js#L146).

I'm going to be out for a while but when I get back I can work on enabling CI for MacOS?

[phated]

I have a new scaffold to apply here that will add github actions with MacOS support. Should I do that and then we can continue work on this?

[coreyfarrell]

I think that would be a good idea.

[phated]

@coreyfarrell all set! Please let me know how else I can help here.

I'm wondering if we need some more tests to demonstrate how node-core's behavior is different between Linux and MacOS to highlight why the mask mode is needed.

[coreyfarrell]

@phated I'm unsure why the CI is failing, it is giving an error I've never seen on the Checkout step. I tried refreshing the master branch of my fork to make sure it had the required github actions files then did a force-push of my own branch but this did not resolve the problem.

[phated]

@coreyfarrell I'm sorry you were the first to run into this. It turns out that my usage of the prettier action was extremely flawed. I've adjusted the workflow to only format code that is pushed to master, so this should work for you now.

[phated]

According to isaacs/node-mkdirp#22, the umask is applied at the operating system level. As with most things deep inside node, I don't trust it and would like to test that assumption.

[phated]

I disabled the coveralls warnings now.

[phated]

Just pushed a change that updates the applyUmask test utility function that I believe helps test my other comment.

[phated]

For completeness' sake, I just tried fs.mkdir(..., { mode: mode, recursive: true }) and it fails our test suite on MacOS. Of course, this was expected because libuv only defers to the underlying operating system and MacOS mkdir behaves differently than the one on linux. I might create a branch with those failures.

[phated]

Alright, I just figured out the test case that I was concerned about, and I also think I figured out the way to solve it. I will get that shipped over sometime this weekend.

Sorry for the spam.

[phated]

@coreyfarrell this went a bit off the rails, but I think I feel confident I understand how everything is working now.

Could you take a look at the changeset and the test runs with verbose logging enabled to see if you agree with everything that I did?

[coreyfarrell]

@phated I'm not on a computer now but I'll be able to review at some point this weekend. Thanks for helping with the CI issues.

[coreyfarrell]

The options object form of fs.mkdir requires node.js v10.12.0. If you want to be compatible with node.js 10.0.0 then this needs to switch back to fs.mkdir(dirname, mode, onMkdir). If not I'd lean towards saying that package.json#engines.node should specify >=10.12.0.

Other than this I think LGTM!

[phated]

Good catch! I'll put that back.