scan_and_remove_entry_recursively hangs if directory contains a named pipe (FIFO)

[angxiang]

Hello,

we are using your crate to perform directory cleanup on a Linux machine. When using the remove_dir_all / remove_dir_contents call, we have noticed that if the directory or subdirectories contain a named pipe, the function will hang and the cleanup will not finish. The issue comes from using open_dir_at in scan_and_remove_entry_recursively to determine whether you are currently handling a directory or not. Since open_dir_at will always pass the O_RDONLY flag, when this is called on a named pipe, the call blocks (until another process opens the pipe for writing, which does not happen).

[ijackson]

Steps

fn main() -> Result<(), std::io::Error> {
    remove_dir_all::remove_dir_all("tmp")
}

rm -rf tmp; mkdir -p tmp/a; touch tmp/b; mkfifo -m600 tmp/p; mkdir tmp/z
strace .../target/debug/rm-tmp

Expected results

It deletes the pipe without opening it

Actual results

openat(4, "p", O_RDONLY|O_NOCTTY|O_NOFOLLOW|O_CLOEXEC [hangs]

Discussion

Nothing in this crate should open anything without O_DIRECTORY.

Opening files is not side-effect free on Unix. For pipes it can hang. Or it can disrupt other processes which are waiting on the pipe. For a tape device, it could rewind the tape!

I don't know how we are implementing open_dir_at but perhaps it will be sufficient to pass O_DIRECTORY.

[ijackson]

Also it can fail unnecessarily: I encountered this bug because I was calling remove_dir_contents on a directory containing an AF_UNIX socket object. Trying to open those gets you ENXIO.

[ijackson]

Also, I don't think it is sound to try to open the file until you know it is a directory. You should use unlinkat first, like std::fs::remove_dir_all does.

Otherwise you might get EACCES when trying to delete an object with no read access.

[rbtcollins]

This is what I see in the stdlib:

            let result: io::Result<()> = try {
                match is_dir(&child) {
                    Some(true) => {
                        remove_dir_all_recursive(Some(fd), child_name)?;
                    }
                    Some(false) => {
                        cvt(unsafe { unlinkat(fd, child_name.as_ptr(), 0) })?;
                    }
                    None => {
                        // POSIX specifies that calling unlink()/unlinkat(..., 0) on a directory can succeed
                        // if the process has the appropriate privileges. This however can causing orphaned
                        // directories requiring an fsck e.g. on Solaris and Illumos. So we try recursing
                        // into it first instead of trying to unlink() it.
                        remove_dir_all_recursive(Some(fd), child_name)?;
                    }
                }
            };

• it does not unlinkat until after stat(), because Solaris/Illumos do nasty things. And that was a factor in the implementation we have here.

I don't have a dev setup handy, but I'll try to come back to this soon. Thank you for the SSCCE.

[rbtcollins]

I've put up a PR to address the ENXIO issue. If someone has time to review, great, otherwise I'll just merge it once I get to the other bug.