msys check fails with long paths

[nikhilm]

https://github.com/softprops/atty/blob/c92ca03bd7a99c2fe8da387b32d653b81f13a667/src/lib.rs#L118

Windows 10 supports longer paths than MAXPATH. When stdout is attached to such a file, the above code panics because it tries to index beyond MAXPATH + 8.

I think this code should be modified to allocate up to 32,767 plus some buffer (see the Note at https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file).

[BurntSushi]

Can you show a backtrace? I can't see where the panic originates.

[nikhilm]

My apologies. I'm not sure any more if this will apply to the atty crate. I'm using the deprecated isatty crate instead (code is part of a larger code base, so I can't change this) and I didn't realize that is a truly different crate that is not deprecated (and I assume this is the replacement). The code is similar, but not exactly the same, as this crate does not access the slice, but does raw pointer access. That said, it seems the from_raw_parts may UB due to reading past the end of the MAX_PATH allocated name_info_bytes buffer if the FileNameLength is longer?

Here is the backtrace from the isatty impl.

thread 'conversion::tests::derive' panicked at 'index 272 out of range for slice of length 268', src\libcore\slice\mod.rs:2569:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys_common::backtrace::print
             at /rustc/50a0defd5a93523067ef239936cc2e0755220904\/src\libstd\sys_common\backtrace.rs:59
   1: std::panicking::default_hook::{{closure}}
             at /rustc/50a0defd5a93523067ef239936cc2e0755220904\/src\libstd\panicking.rs:197
   2: std::panicking::default_hook
             at /rustc/50a0defd5a93523067ef239936cc2e0755220904\/src\libstd\panicking.rs:208
   3: std::panicking::rust_panic_with_hook
             at /rustc/50a0defd5a93523067ef239936cc2e0755220904\/src\libstd\panicking.rs:474
   4: std::panicking::continue_panic_fmt
             at /rustc/50a0defd5a93523067ef239936cc2e0755220904\/src\libstd\panicking.rs:381
   5: std::panicking::rust_begin_panic
             at /rustc/50a0defd5a93523067ef239936cc2e0755220904\/src\libstd\panicking.rs:308
   6: core::panicking::panic_fmt
             at /rustc/50a0defd5a93523067ef239936cc2e0755220904\/src\libcore\panicking.rs:85
   7: core::slice::slice_index_len_fail
             at /rustc/50a0defd5a93523067ef239936cc2e0755220904\/src\libcore\slice\mod.rs:2569
   8: core::slice::{{impl}}::index
             at /rustc/50a0defd5a93523067ef239936cc2e0755220904\src\libcore\slice\mod.rs:2746
   9: core::slice::{{impl}}::index
             at /rustc/50a0defd5a93523067ef239936cc2e0755220904\src\libcore\slice\mod.rs:2552
  10: alloc::vec::{{impl}}::index
             at /rustc/50a0defd5a93523067ef239936cc2e0755220904\src\liballoc\vec.rs:1687
  11: isatty::is_cygwin_pty
             at /rust_vendor/isatty-0.1.9/src/lib.rs:138
  12: isatty::isatty
             at /rust_vendor/isatty-0.1.9/src/lib.rs:111
  13: slog_term::AnyTerminal::should_use_color
             at /rust_vendor/slog-term-2.4.0/src/lib.rs:1176
  14: slog_term::TermDecoratorBuilder::build
             at /rust_vendor/slog-term-2.4.0/src/lib.rs:1259

The slog-term bug is https://github.com/slog-rs/slog/issues/214 268 is 8 + MAXPATH.

Feel free to close if you don't consider this applicable.

[BurntSushi]

That said, it seems the from_raw_parts may UB due to reading past the end of the MAX_PATH allocated name_info_bytes buffer if the FileNameLength is longer?

Yeah, that's exactly what I was thinking, in that it would be UB and not a panic, which is worse. I'd consider this a bug in this crate, definitely.

[nikhilm]

I think atty is safe because it handles wchars properly, whereas isatty assumes chars.

When atty hits the windows call, it has allocated a struct of size 8 + 2*260 = 528 bytes, whereas isatty has allocated 268.

When GetFileInformationByHandleEx encounters a path > what would fit in the buffer, it returns error 234 (more data available) and we hit the early return.

when the path is exactly 262 wchars (524 bytes): atty: correctly reads from byte 4 (FileName offset) to FileNameLength / 2 (*2 since from_raw_parts operates on T, not bytes) -> till 528, which is fine.

for isatty, the same early return happens at 133 wchars and onwards when the path is 131 wchars (262 bytes): isatty attempts to index the slice from (incorrect) index 8 to index (262 + 8) which panics. It similarly fails for 132.

Ref: https://github.com/dtolnay/isatty/blob/1994ab57dabc3ee943e4a87df252bf02075d1bdc/src/lib.rs#L138

I'm going to consider this closed, as slog-term needs to use this crate.

[nikhilm]

if you want to play with this - https://gist.github.com/nikhilm/34d156f58ea353ed3275b434dcaf5808 I did have to comment out all the pre-msys checks in atty to reliably get that code path to execute

[BurntSushi]

Ah okay, great, thanks for the analysis!