`remove_dir_all` fails removing directores (with subdirectories) on network drives (SFTP) under windows about rust HOT 28 CLOSED

It was indeed confirmed, that the error was in the Network Drive SFTP driver.

Details can be found winfsp/winfsp#561

from rust.

If you end up creating an issue at https://github.com/winfsp/sshfs-win/, please link to it. :)

from rust.

BTW: mimicing the remove_dir_all with this function works:

use std::{fs, io, path::Path};

fn main() {
    // fs::remove_dir_all("Z:\\latest\\.xxx").unwrap();
    remove_dir_all_alternative(Path::new("Z:\\latest\\.xxx")).unwrap();
}

fn remove_dir_all_alternative(path: &Path) -> io::Result<()> {
    for entry in fs::read_dir(path)? {
        let path = entry?.path();

        if path.is_dir() {
            remove_dir_all_alternative(&path)?;
        } else {
            fs::remove_file(path)?;
        }
    }
    fs::remove_dir(path)?;

    Ok(())
}

from rust.

cc @ChrisDenton

from rust.

Does it work if you call remove_dir_all repeatedly? Maybe with a sleep between calls.

from rust.

When running this

use std::{fs, thread};

fn main() {
    let path = "Z:\\latest\\.xxx";
    // let path = "C:\\temp\\.xxx";

    for i in 1..10 {
        match fs::remove_dir_all(path) {
            Ok(_) => {
                println!("Success!");
                break;
            }
            Err(_) => {
                println!("Failed! Retrying... ({}/10)", i);
                thread::sleep(std::time::Duration::from_millis(50));
            }
        }
    }
}

I get:

     Running `target\debug\remover.exe`
Failed! Retrying... (1/10)
Failed! Retrying... (2/10)
Failed! Retrying... (3/10)
Failed! Retrying... (4/10)
Failed! Retrying... (5/10)
Failed! Retrying... (6/10)
Failed! Retrying... (7/10)
Failed! Retrying... (8/10)
Failed! Retrying... (9/10)

when I run this with the C:\temp\.xxx path, it works on first try.

Also playing with the sleep didn't help.

from rust.

Does anything get deleted or is it all still there?

from rust.

No, actually nothing gets deleted.

from rust.

BTW: I'm using x86_64-pc-windows-gnu as target

from rust.

Ok, thank you. Rust's implementation of remove_dir_all is rather gnarly to defend against CVE-2022-21658. My best guess is the filesystem is either returning ERROR_DIR_NOT_EMPTY when deleting a file in the way we do or else it's returning success but not deleting the file.

Which sftp client are you using btw?

from rust.

I'm using this driver: https://github.com/winfsp/sshfs-win

from rust.

Hm, a cygwin port of a unix tool? That may make error codes misleading because it's translating them to/from POSIX error codes.

I've extracted the part that I think is failing into a test program you could try. It should either fail when calling the custom delete function or with the assert! at the end.

# Cargo.toml
[dependencies]
windows-sys = { version = "0.52.0", features = ["Win32_Foundation", "Win32_Storage_FileSystem"] }

// src/main.rs
use std::fs::File;
use std::os::windows::fs::OpenOptionsExt;
use std::os::windows::io::AsRawHandle;
use windows_sys::Win32::Storage::FileSystem::{
    FileDispositionInfoEx, SetFileInformationByHandle, DELETE, FILE_DISPOSITION_FLAG_DELETE,
    FILE_DISPOSITION_FLAG_IGNORE_READONLY_ATTRIBUTE, FILE_DISPOSITION_FLAG_POSIX_SEMANTICS,
    FILE_DISPOSITION_INFO_EX,
};

fn main() {
    let path = r"Z:\latest\.xxx\some-file.txt";
    let file = File::options().access_mode(DELETE).open(path).unwrap();
    delete(&file).unwrap();
    drop(file);
    assert!(!std::path::Path::new(path).exists());
}

fn delete(file: &File) -> std::io::Result<()> {
    unsafe {
        let handle = file.as_raw_handle() as _;
        let info = FILE_DISPOSITION_INFO_EX {
            Flags: FILE_DISPOSITION_FLAG_DELETE
                | FILE_DISPOSITION_FLAG_POSIX_SEMANTICS
                | FILE_DISPOSITION_FLAG_IGNORE_READONLY_ATTRIBUTE,
        };
        let result = SetFileInformationByHandle(
            handle,
            FileDispositionInfoEx,
            std::ptr::from_ref(&info).cast(),
            std::mem::size_of::<FILE_DISPOSITION_INFO_EX>() as u32,
        );
        if result == 0 {
            Err(std::io::Error::last_os_error())
        } else {
            Ok(())
        }
    }
}

from rust.

:) This works perfectly fine. The file r"Z:\latest\.xxx\some-file.txt" is gone and the program exited with success.

from rust.

Huh interesting, that unfortunately means the failure is somewhere in the more complicated bit of code.

from rust.

I tried a different SFTP integartion: https://www.nsoftware.com/sftpdrive

When deleting it from network drives with that integration, the remove_dir_all works like a charm.

from rust.

When I have some free time I'll extract the full remove_dir_all code into a standalone program so the exact cause of the error can be determined. It's likely either the special way we open files or else directory iteration.

In the meantime you could also try the remove_dir_all crate. This uses a similar method to std but it's just different enough that it may work. It might also have better error reporting.

from rust.

Is there something like strace for Windows? It would really help here.

from rust.

Thx for all this effort.

I already tried to extract the remove_dir_all_iterative into my program, but it has too many internal dependencies for my current Rust-skills :)

I'll try to find out if the used SFTP integration has some sort of log. If I find something, I'll add it here.

from rust.

Is there something like strace for Windows? It would really help here.

I'd usually use Process Monitor for this.

from rust.

@guenhter Could you try running the original code, but attached to Process Monitor, and report what call fails and with which error?

from rust.

This tool is awesome:

here a screenshot and the zipped log of the process-monitor.

Logfile.zip

from rust.

And this happens, If I delete a directory with only a file inside and no more subdirs (successful operation).

from rust.

In the first screenshot we see the source of the problem:

CreateFile is called with the path .xxx\y\some.txt (with DesiredAccess: Delete, Synchronize, Disposition: Open, Options: OPen Reparse Point, Attribtes: n/a, ShareMode: Read, Write, Delete, AllocationSize: n/a).

The file itself seems to exist. But the call fails with BAD NETWORK PATH.

The next step is to figure out whether this is a problem with the Rust code or with https://github.com/winfsp/sshfs-win.

from rust.

When looking at the code, the only thing what is different of the root directory and the subdirectories is the way the file-handles to the directories are opened:

// File handle to root dir
let file = open_link(path, c::DELETE | c::FILE_LIST_DIRECTORY)?;

// File handle to sub directories
let child_dir = open_link_no_reparse(
    &dir,
    &name,
    c::SYNCHRONIZE | c::DELETE | c::FILE_LIST_DIRECTORY,
);

The rest is (file deletion handling, ...) seems idential. If we could extract the imporant parts of open_link_no_reparse, I guess it would be easy for me to test if this subtle different is the issue...

from rust.

Hmm, that is the code for opening subdirectories based on a parent directory handle, which is the core of the CVE mitigation. It could be there's some combination of flags it doesn't like. If it doesn't support this at all then that's more of a problem and will be difficult to reconcile.

Here's a very cut down program that basically copy/pastes the open calls but doesn't do directory iteration:

[dependencies]
windows-sys = { version = "0.52.0", features = [
    "Wdk_Foundation",
    "Wdk_Storage_FileSystem",
    "Win32_Foundation",
    "Win32_Storage_FileSystem",
    "Win32_System_IO",
    "Win32_System_Kernel",
] }

use std::fs::File;
use std::io;
use std::os::windows::fs::OpenOptionsExt;
use std::os::windows::io::AsRawHandle;
use std::os::windows::io::FromRawHandle;
use std::path::Path;
use std::ptr;
use windows_sys::Wdk::Foundation::OBJECT_ATTRIBUTES;
use windows_sys::Wdk::Storage::FileSystem::{NtCreateFile, FILE_OPEN, FILE_OPEN_REPARSE_POINT};
use windows_sys::Win32::Foundation::{
    RtlNtStatusToDosError, ERROR_DELETE_PENDING, STATUS_DELETE_PENDING, STATUS_PENDING,
    UNICODE_STRING,
};
use windows_sys::Win32::Storage::FileSystem::{
    FileDispositionInfoEx, SetFileInformationByHandle, DELETE, FILE_DISPOSITION_FLAG_DELETE,
    FILE_DISPOSITION_FLAG_IGNORE_READONLY_ATTRIBUTE, FILE_DISPOSITION_FLAG_POSIX_SEMANTICS,
    FILE_DISPOSITION_INFO_EX, FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT,
    FILE_LIST_DIRECTORY, FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, SYNCHRONIZE,
};
use windows_sys::Win32::System::Kernel::OBJ_DONT_REPARSE;
use windows_sys::Win32::System::IO::{IO_STATUS_BLOCK, IO_STATUS_BLOCK_0};

fn utf16(s: &str) -> Vec<u16> {
    s.encode_utf16().collect()
}

fn main() {
    let path = r"Z:\latest\.xxx";
    let subpath = "y";
    let filename = "some.txt";

    let dir = open_link(path.as_ref(), DELETE | FILE_LIST_DIRECTORY).unwrap();
    let subdir = open_link_no_reparse(
        &dir,
        &utf16(subpath),
        SYNCHRONIZE | DELETE | FILE_LIST_DIRECTORY,
    )
    .unwrap();
    let f = open_link_no_reparse(&subdir, &utf16(filename), SYNCHRONIZE | DELETE).unwrap();
    f.posix_delete().unwrap();
}

trait FileExt {
    fn posix_delete(&self) -> io::Result<()>;
}
impl FileExt for File {
    fn posix_delete(&self) -> io::Result<()> {
        unsafe {
            let handle = self.as_raw_handle() as _;
            let info = FILE_DISPOSITION_INFO_EX {
                Flags: FILE_DISPOSITION_FLAG_DELETE
                    | FILE_DISPOSITION_FLAG_POSIX_SEMANTICS
                    | FILE_DISPOSITION_FLAG_IGNORE_READONLY_ATTRIBUTE,
            };
            let result = SetFileInformationByHandle(
                handle,
                FileDispositionInfoEx,
                std::ptr::from_ref(&info).cast(),
                std::mem::size_of::<FILE_DISPOSITION_INFO_EX>() as u32,
            );
            if result == 0 {
                Err(std::io::Error::last_os_error())
            } else {
                Ok(())
            }
        }
    }
}

fn open_link(path: &Path, access_mode: u32) -> io::Result<File> {
    File::options()
        .access_mode(access_mode)
        .custom_flags(FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT)
        .open(path)
}

fn open_link_no_reparse(parent: &File, name: &[u16], access: u32) -> io::Result<File> {
    unsafe {
        let mut handle = 0;
        let mut io_status = IO_STATUS_BLOCK {
            Anonymous: IO_STATUS_BLOCK_0 {
                Status: STATUS_PENDING,
            },
            Information: 0,
        };
        let mut name_str = UNICODE_STRING {
            Length: (name.len() * 2) as u16,
            MaximumLength: (name.len() * 2) as u16,
            Buffer: name.as_ptr().cast_mut(),
        };
        let object = OBJECT_ATTRIBUTES {
            Length: std::mem::size_of::<OBJECT_ATTRIBUTES>() as u32,
            ObjectName: &mut name_str,
            RootDirectory: parent.as_raw_handle() as _,
            Attributes: OBJ_DONT_REPARSE as _,
            SecurityDescriptor: ptr::null(),
            SecurityQualityOfService: ptr::null(),
        };
        let status = NtCreateFile(
            &mut handle,
            access,
            &object,
            &mut io_status,
            ptr::null_mut(),
            0,
            FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE,
            FILE_OPEN,
            FILE_OPEN_REPARSE_POINT,
            ptr::null_mut(),
            0,
        );
        eprintln!("NtCreateFile: {status:#X}");
        if status >= 0 {
            Ok(File::from_raw_handle(handle as _))
        } else if status == STATUS_DELETE_PENDING {
            Err(io::Error::from_raw_os_error(ERROR_DELETE_PENDING as i32))
        } else {
            Err(io::Error::from_raw_os_error(
                RtlNtStatusToDosError(status) as _
            ))
        }
    }
}

from rust.

Very nice. This reproduces the issue.

So the combination of

let subdir = open_link_no_reparse(...);
let f = open_link_no_reparse(&subdir, ...);

always fails,

and the combination of

let dir = open_link(...).unwrap();
let f = open_link_no_reparse(&dir, ...);

works.

from rust.

That sounds like an issue with the file system driver to me.

from rust.

Yeah, everything points in that direction.

Digging deeper into the two open_link... functions will reveal, that it boils down to those two functions:

CreateFileW(...)   // for open_link

NtCreateFile(...)   // for open_link_no_reparse

I'll isolate those calls even further and then open an issue on the drivers side. I doubt that it has to do anything with the paramters of NtCreateFile becuase it works for the local filesystem, samba, and even for SFTP with another driver.

Thank you very much guys for the incredible support!

from rust.