EINVAL should not be a fatal error when chown-ing

[isaacs]

EINVAL should not be a fatal error when chown-ing

[alanshields]

I think when it comes down to it the important thing is to ignore chown failures. It's simply not a crucial operation. The only situation where we'd care would be if the suid bit was set on the file - Ruby handles this case by dropping the suid bit if chown fails.

Thanks for your time on this!

[isaacs]

Thanks for the writeup. I think the right thing would be to follow cp on this.

The chown_failure_ok function looks interesting. It seems like it still fails if running as root, though?

[alanshields]

As root, yes. I looked up the behavior in install as well and it's the same. install.c had the handy explanatory text:

 We don't normally ignore errors from chown because the idea of
 the install command is that the file is supposed to end up with
 precisely the attributes that the user specified (or defaulted).
 If the file doesn't end up with the group they asked for, they'll
 want to know.

and the code:

  if (! (owner_id == (uid_t) -1 && group_id == (gid_t) -1)
      && lchown (name, owner_id, group_id) != 0)
    error (0, errno, _("cannot change ownership of %s"), quote (name));
  else if (chmod (name, mode) != 0)
    error (0, errno, _("cannot change permissions of %s"), quote (name));
  else
    ok = true;

owner_id is set to -1 if files should be owned by the current user - in other words it skips the chown.

Hmm.

Okay, to break this down into two separate cases:

1. If we are installing a file as root to be owned by another user, we should error out if that fails because it's a violation of the user's stated desire.
2. If we are installing a file as a normal user, we should skip the chown (install-style) or at least ignore failure (cp-style).

For NPM's purposes, this would be divided into (1) "all-user global installs" - where it would be a failure to not have the file owned by root or whomever is designated, and (2) "node_modules installs" - where the current user creates files owned by the current user.

Am I misunderstanding the cases?

[isaacs]

Let's stick to what graceful-fs should do here. It sounds like the algorithm you're proposing is:

if (er is EINVAL or EPERM) {
  if (user is root) {
    raise exception
  } else {
    ignore exception
  }
} else if (some other error) raise exception

Here's how npm decides whether or not to chown:

if local: uid=(owner of nearest ancestor of file, or SUDO_UID)
else: uid=("user" config val, default="nobody")

if (not root, or "unsafe-perm" config flag is set) doChown = false
else doChown = true

if (doChown) chown(file, uid)

So, really, if we just swallowed any EINVAL or EPERM for non-root uses of chown, it should satisfy you, I'd think. It shouldn't even try to do chown if you're not root (or that's a bug), but it might be trying to chown on file systems that don't support ownership, and then incorrectly failing, but this change would fix that.

Am I understanding that correctly?

[alanshields]

I think we're exactly on the same page. I do wonder how chown is being called in that case, but if we eat the error on non-root we'll be fine either way.

As my npm-fu is not that strong, if you'll let me know how to test any change you make without you doing a full release, I'll hop on it.

Thanks!