Windows: handle illegal filenames a little better (#7999)

This PR is an incremental improvement to `ls` when it encounters
'illegal' file paths on Windows. Related:
https://github.com/nushell/nushell/issues/7869

## Context

We have trouble with filenames that Windows doesn't like, for example
[files with a `.` at the end of their
name](https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions).
To make a long story short, the Rust stdlib and several Win32 APIs will
choke if asked to do something with an illegal filepath. This is a
problem because files with illegal names can be created via other means
(like `touch foo.` in MINGW bash).

Previously `ls` would fail completely in a directory with a bad file,
which isn't great. After this PR, bad files get included in `ls` results
but without any metadata columns. This is not quite where we want to be
— eventually we want to be able to display file metadata for _all_ files
(even naughty ones) — but it's an improvement on the status quo.

### Before


![image](https://user-images.githubusercontent.com/26268125/217340906-26afd6d3-0ec3-454f-bed4-2bfcc9cf3a2f.png)

### After


![image](https://user-images.githubusercontent.com/26268125/217344373-6b81cc39-50b8-4390-8061-3e570502a784.png)

## Future work

Try the workarounds @ChrisDenton suggested:
https://github.com/nushell/nushell/issues/7869#issuecomment-1405977221

Some info on verbatim paths:
https://users.rust-lang.org/t/understanding-windows-paths/58583

## Testing

I tried to write a test for this, but it looks like our testing sandbox
can't create files with illegal filenames.😔 Here's the code in case it
proves useful someday:

```rust
/// Windows doesn't like certain file names, like file names ending with a period:
/// https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions
/// However, those files can still be created with tools like MINGW bash.
/// We may not be able to get full metadata for those files, but we should test that we can at least include them in ls results
#[test]
#[cfg(windows)]
fn can_list_illegal_files() {
    Playground::setup("ls_test_all_columns", |dirs, sandbox| {
        sandbox.with_files(vec![
            EmptyFile("foo"),
            EmptyFile("bar."),
            EmptyFile("baz"),
        ]);

        let actual = nu!(
            cwd: dirs.test(),
            "ls | length"
        );
        assert_eq!(actual.out, "3");

        let actual = nu!(
            cwd: dirs.test(),
            "ls"
        );
        assert_eq!(actual.out, "1");

        let actual = nu!(
            cwd: dirs.test(),
            "ls | where {|f| $f.name | str ends-with 'bar.'} | length"
        );
        assert_eq!(actual.out, "1");
    })
}
```
This commit is contained in:
Reilly Wood 2023-02-07 20:30:37 +00:00 committed by GitHub
parent 58f0d0b945
commit a562f492e3
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23

View file

@ -419,7 +419,12 @@ pub(crate) fn dir_entry_dict(
) -> Result<Value, ShellError> {
#[cfg(windows)]
if metadata.is_none() {
return windows_helper::dir_entry_dict_windows_fallback(filename, display_name, span, long);
return Ok(windows_helper::dir_entry_dict_windows_fallback(
filename,
display_name,
span,
long,
));
}
let mut cols = vec![];
@ -673,7 +678,6 @@ fn unix_time_to_local_date_time(secs: i64) -> Option<DateTime<Local>> {
mod windows_helper {
use super::*;
use std::mem::MaybeUninit;
use std::os::windows::prelude::OsStrExt;
use windows::Win32::Foundation::FILETIME;
use windows::Win32::Storage::FileSystem::{
@ -692,7 +696,7 @@ mod windows_helper {
display_name: &str,
span: Span,
long: bool,
) -> Result<Value, ShellError> {
) -> Value {
let mut cols = vec![];
let mut vals = vec![];
@ -702,7 +706,20 @@ mod windows_helper {
span,
});
let find_data = find_first_file(filename, span)?;
let find_data = match find_first_file(filename, span) {
Ok(fd) => fd,
Err(e) => {
// Sometimes this happens when the file name is not allowed on Windows (ex: ends with a '.')
// For now, we just log it and give up on returning metadata columns
// TODO: find another way to get this data (like cmd.exe, pwsh, and MINGW bash can)
eprintln!(
"Failed to read metadata for '{}'. It may have an illegal filename",
filename.to_string_lossy()
);
log::error!("{e}");
return Value::Record { cols, vals, span };
}
};
cols.push("type".into());
vals.push(Value::String {
@ -783,7 +800,7 @@ mod windows_helper {
vals.push(val);
}
Ok(Value::Record { cols, vals, span })
Value::Record { cols, vals, span }
}
fn unix_time_from_filetime(ft: &FILETIME) -> i64 {
@ -801,7 +818,7 @@ mod windows_helper {
// wrapper around the FindFirstFileW Win32 API
fn find_first_file(filename: &Path, span: Span) -> Result<WIN32_FIND_DATAW, ShellError> {
unsafe {
let mut find_data = MaybeUninit::<WIN32_FIND_DATAW>::uninit();
let mut find_data = WIN32_FIND_DATAW::default();
// The windows crate really needs a nicer way to do string conversions
let filename_wide: Vec<u16> = filename
.as_os_str()
@ -809,23 +826,22 @@ mod windows_helper {
.chain(std::iter::once(0))
.collect();
if FindFirstFileW(
match FindFirstFileW(
windows::core::PCWSTR(filename_wide.as_ptr()),
find_data.as_mut_ptr(),
)
.is_err()
{
return Err(ShellError::ReadingFile(
format!(
"Could not read metadata for '{}'. It may have an illegal filename.",
filename.to_string_lossy()
),
span,
));
&mut find_data,
) {
Ok(_) => Ok(find_data),
Err(e) => {
return Err(ShellError::ReadingFile(
format!(
"Could not read metadata for '{}':\n '{}'",
filename.to_string_lossy(),
e
),
span,
));
}
}
let find_data = find_data.assume_init();
Ok(find_data)
}
}