From a562f492e385fa2007cec0064f8ac3d5550372ce Mon Sep 17 00:00:00 2001 From: Reilly Wood <26268125+rgwood@users.noreply.github.com> Date: Tue, 7 Feb 2023 20:30:37 +0000 Subject: [PATCH] Windows: handle illegal filenames a little better (#7999) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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"); }) } ``` --- crates/nu-command/src/filesystem/ls.rs | 58 ++++++++++++++++---------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/crates/nu-command/src/filesystem/ls.rs b/crates/nu-command/src/filesystem/ls.rs index a94ef6c615..97a0799945 100644 --- a/crates/nu-command/src/filesystem/ls.rs +++ b/crates/nu-command/src/filesystem/ls.rs @@ -419,7 +419,12 @@ pub(crate) fn dir_entry_dict( ) -> Result { #[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> { 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 { 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 { unsafe { - let mut find_data = MaybeUninit::::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 = 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) } }