Using 'unlink' is not safe when running as 'root' as on Solaris 10

[itay]

So, it turns out that this comment:

// Two possible strategies.
// 1. Assume it's a file.  unlink it, then do the dir stuff on EPERM or EISDIR
// 2. Assume it's a directory.  readdir, then do the file stuff on ENOTDIR
//
// Both result in an extra syscall when you guess wrong.  However, there
// are likely far more normal files in the world than directories.  This
// is based on the assumption that a the average number of files per
// directory is >= 1.
//
// If anyone ever complains about this, then I guess the strategy could
// be made configurable somehow.  But until then, YAGNI.

That I actually do need it.

I honestly can't believe I hit this, but I did. Basically, the issue is this:

On Solaris 10 (at least), if you run as root, it will let you call unlink on a directory and not return an error. This in turn makes any directory containing this undeletable, because the link count is now wrong.

The solution here is simple: either always use rmdir-readdir-unlink, or have an option to enable it in select cases.

The specific place I hit it was when using node-gyp and calling rebuild on a package, which did a clean, which in turn called rimraf.

I still can't believe this happened.

[isaacs]

Wow, that's pretty wild! Filesystems are jerks.

Patch welcome to pass some kind of "stat first" flag. Maybe pass {stat:true} in the options object.

Note that the "readdir, do file stuff on ENOTDIR" strategy is wildly unsafe, since it'll mean that symlinked directories lose their contents, rather than just removing the link.

[isaacs]

Oh! Another option you could do today is to pass in a custom unlink method that stats first.

rimraf(someDir, {
  unlink: function (path, cb) {
    fs.lstat(path, function (er, st) {
      if (er) return cb(er);
      if (st.isDirectory()) {
        var er = new Error('A directory');
        er.path = path;
        er.code = 'EISDIR';
        return cb(er);
      }
      return fs.unlink(path, cb);
    })
  }
}, cb)

(EDIT: updated to do lstat, because symlinks, and also correct error code)

[mitchblank]

FWIW, Solaris is actually doing the "classic" UNIX behavior for unlink(), it's just that the classic behavior is a bit insane. The relevant text from the unlink(2) man page:

If the path argument is a directory and the filesystem  sup-
ports  unlink() and unlinkat() on directories, the directory
is unlinked from its parent with no cleanup being performed.
In  UFS,  the  disconnected directory will be found the next
time the filesystem is checked with fsck(1M).  The  unlink()
and  unlinkat()  functions  will  not  fail simply because a
directory is not empty. The user with appropriate privileges
can orphan a non-empty directory without generating an error
message.

If the path argument is a directory and the filesystem  does
not  support unlink() and unlink() on directories (for exam-
ple, ZFS), the call will fail with errno set to EPERM.

It makes it very hard to safely implement rm -rf as root since even if you lstat or rmdir first you still have a TOCTOU bug. Still, it's better than nothing.

Here's what a Solaris 10 system does for rm -rf a where a is just an empty directory:

lstat64("a", 0x08047B00)                        = 0
open64(".", O_RDONLY)                           = 3
stat64(".", 0x08047A60)                         = 0
chdir("..")                                     = 0
lstat64(".", 0x08047A60)                        = 0
chdir("..")                                     = 0
lstat64(".", 0x08047A60)                        = 0
fchdir(3)                                       = 0
stat64(".", 0x08047A20)                         = 0
openat(-3041965, "a", O_RDONLY|O_NDELAY|O_LARGEFILE) = 4
fcntl(4, F_SETFD, 0x00000001)                   = 0
fstat64(4, 0x08047A00)                          = 0
chdir("a")                                      = 0
getdents64(4, 0xFEF94000, 8192)                 = 48
sysconfig(_CONFIG_PAGESIZE)                     = 4096
getdents64(4, 0xFEF94000, 8192)                 = 0
close(4)                                        = 0
fchdir(3)                                       = 0
rmdir("a")                                      = 0

That "-3041965" is the magic AT_FDCWD constant which makes the *at() calls operate on the current directory. If a is a plain file it looks like:

lstat64("a", 0x08047B00)                        = 0
unlink("a")                                     = 0

So basically solaris itself uses the lstat-first strategy.

[psimondk]

Makes you wonder what will happen if you where to execute the following as root ;-)

unlink / ; sync ; halt