Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crashes on long path lengths on Windows #18

Closed
epage opened this issue Feb 14, 2023 · 4 comments
Closed

Crashes on long path lengths on Windows #18

epage opened this issue Feb 14, 2023 · 4 comments

Comments

@epage
Copy link

epage commented Feb 14, 2023

cargo just got a report of a crash on Windows with long paths (rust-lang/cargo#11710)

Problem

cargo clean, cargo tree, cargo build, etc. generates panic

thread 'main' panicked at 'range end index 270 out of range for slice of length 260', C:\Users\runneradmin\.cargo\registry\src\github.com-1ecc6299db9ec823\is-terminal-0.4.0\src\lib.rs:169:14 stack backtrace: 0: 0x7ff6d75f5db2 - git_midx_writer_dump 1: 0x7ff6d761b89b - git_midx_writer_dump 2: 0x7ff6d75ecb4a - git_midx_writer_dump 3: 0x7ff6d75f5afb - git_midx_writer_dump 4: 0x7ff6d75f8ef9 - git_midx_writer_dump 5: 0x7ff6d75f8b7b - git_midx_writer_dump 6: 0x7ff6d75f9791 - git_midx_writer_dump 7: 0x7ff6d75f951e - git_midx_writer_dump 8: 0x7ff6d75f6aaf - git_midx_writer_dump 9: 0x7ff6d75f91d0 - git_midx_writer_dump 10: 0x7ff6d76ae435 - git_midx_writer_dump 11: 0x7ff6d761e267 - git_midx_writer_dump 12: 0x7ff6d76ae959 - git_midx_writer_dump 13: 0x7ff6d74a4659 - git_midx_writer_dump 14: 0x7ff6d6cc7087 - git_libgit2_prerelease 15: 0x7ff6d6cc15ba - git_libgit2_prerelease 16: 0x7ff6d6cc1499 - git_libgit2_prerelease 17: 0x7ff6d6c3f8c7 - git_libgit2_prerelease 18: 0x7ff6d6cb77be - git_libgit2_prerelease 19: 0x7ff6d6c34e16 - git_libgit2_prerelease 20: 0x7ff6d6c44c2c - git_libgit2_prerelease 21: 0x7ff6d75e4f0e - git_midx_writer_dump 22: 0x7ff6d6cba51c - git_libgit2_prerelease 23: 0x7ff6d7624ee0 - git_midx_writer_dump 24: 0x7ffb5eb184d4 - BaseThreadInitThunk 25: 0x7ffb60ca1791 - RtlUserThreadStart

Steps

1. setup rust build in remote session, Jenkins, for example, using really long file names

2. use next script to be called by CI
rustup default 1.67.0
rustup update 1.67.0
cargo clean
3. panic

Possible Solution(s)

Switch to version 1.66.0

Notes

Normal Windows command line Session works fine.

Problem lies in is_terminal:0.4.0, which failed if real terminal is not available. The problem is related to the badly supported long file names: 260 is a MAX_PATH, and we have really long paths. thread 'main' panicked at 'range end index 270 out of range for slice of length 260' code```

 let mut name_info = FILE_NAME_INFO {
    FileNameLength: 0,
    FileName: [0; MAX_PATH as usize], //// <- ERROR FileNameLength is 260
};
// Safety: function has no invariants. an invalid handle id will cause
//         GetFileInformationByHandleEx to return an error
let handle = GetStdHandle(fd);
// Safety: handle is valid, and buffer length is fixed
let res = GetFileInformationByHandleEx(
    handle,
    FileNameInfo,
    &mut name_info as *mut _ as *mut c_void,
    std::mem::size_of::<FILE_NAME_INFO>() as u32,
);
if res == 0 {
    return false;
}
let s = &name_info.FileName[..name_info.FileNameLength as usize]; // <- FAULT FileNameLength is 270

### Version

```text
cargo 1.67.1 (8ecd4f20a 2023-01-10)
release: 1.67.1
commit-hash: 8ecd4f20a9efb626975ac18a016d480dc7183d9b
commit-date: 2023-01-10
host: x86_64-pc-windows-msvc
libgit2: 1.5.0 (sys:0.16.0 vendored)
libcurl: 7.86.0-DEV (sys:0.4.59+curl-7.86.0 vendored ssl:Schannel)
os: Windows 10.0.14393 (Windows Server 2016 Standard) [64-bit]
@ChrisDenton
Copy link

That seems weird. The code in question checks for failure, and not having enough space in the buffer should be a failure:

is-terminal/src/lib.rs

Lines 158 to 166 in bee73f2

let res = GetFileInformationByHandleEx(
handle,
FileNameInfo,
&mut name_info as *mut _ as *mut c_void,
std::mem::size_of::<FILE_NAME_INFO>() as u32,
);
if res == 0 {
return false;
}

Maybe it's a result of a server that's using customized drivers (or other third party kernel tweaks) or maybe there's a bug in that version of Windows Server. Either way I guess it'll have to verify the FileNameLength too.

sunfishcode added a commit that referenced this issue Feb 14, 2023
In #18 there is a report that under some situations
`GetFileInformationByHandleEx` can produce a file name with a length
that extends beyond the end of the buffer. Check for this case and
return false if it is.

Also, backport the change from std to only search the file name for
"msys-" and "cygwin-", rather than the whole path.
@sunfishcode
Copy link
Owner

I've now prepared #19 with a check that the filename length is in bounds before we use it.

sunfishcode added a commit that referenced this issue Feb 19, 2023
* Handle `GetFileInformationByHandleEx` returning a long filename.

In #18 there is a report that under some situations
`GetFileInformationByHandleEx` can produce a file name with a length
that extends beyond the end of the buffer. Check for this case and
return false if it is.

Also, backport the change from std to only search the file name for
"msys-" and "cygwin-", rather than the whole path.

* Rebase the implementation on std's implementation.
@weihanglo
Copy link

Hi @sunfishcode, is there any plan to have a point release in a few days? We're going to cut a beta backport on Cargo side. No worries if there is not. Thank you for the quick fix!

@sunfishcode
Copy link
Owner

I've now released is-terminal 0.4.4 with this fix.

sunfishcode added a commit to sunfishcode/rust that referenced this issue Feb 23, 2023
As reported in sunfishcode/is-terminal#18, there are situations where
`GetFileInformationByHandleEx` can write a file name length that is
longer than the provided buffer. To avoid deferencing memory past the
end of the buffer, use a bounds-checked function to form a slice to
the buffer and handle the out-of-bounds case.

This ports the fix from sunfishcode/is-terminal#19 to std's `is_terminal`
implementation.
weihanglo added a commit to weihanglo/cargo that referenced this issue Feb 23, 2023
bors added a commit to rust-lang/cargo that referenced this issue Feb 23, 2023
weihanglo added a commit to weihanglo/cargo that referenced this issue Feb 23, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 23, 2023
…l-file-length, r=ChrisDenton

Fix `is_terminal`'s handling of long paths on Windows.

As reported in sunfishcode/is-terminal#18, there are situations where `GetFileInformationByHandleEx` can write a file name length that is longer than the provided buffer. To avoid deferencing memory past the end of the buffer, use a bounds-checked function to form a slice to the buffer and handle the out-of-bounds case.

This ports the fix from sunfishcode/is-terminal#19 to std's `is_terminal` implementation.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 24, 2023
…l-file-length, r=ChrisDenton

Fix `is_terminal`'s handling of long paths on Windows.

As reported in sunfishcode/is-terminal#18, there are situations where `GetFileInformationByHandleEx` can write a file name length that is longer than the provided buffer. To avoid deferencing memory past the end of the buffer, use a bounds-checked function to form a slice to the buffer and handle the out-of-bounds case.

This ports the fix from sunfishcode/is-terminal#19 to std's `is_terminal` implementation.
thomcc pushed a commit to tcdi/postgrestd that referenced this issue May 31, 2023
As reported in sunfishcode/is-terminal#18, there are situations where
`GetFileInformationByHandleEx` can write a file name length that is
longer than the provided buffer. To avoid deferencing memory past the
end of the buffer, use a bounds-checked function to form a slice to
the buffer and handle the out-of-bounds case.

This ports the fix from sunfishcode/is-terminal#19 to std's `is_terminal`
implementation.
thomcc pushed a commit to tcdi/postgrestd that referenced this issue May 31, 2023
…ngth, r=ChrisDenton

Fix `is_terminal`'s handling of long paths on Windows.

As reported in sunfishcode/is-terminal#18, there are situations where `GetFileInformationByHandleEx` can write a file name length that is longer than the provided buffer. To avoid deferencing memory past the end of the buffer, use a bounds-checked function to form a slice to the buffer and handle the out-of-bounds case.

This ports the fix from sunfishcode/is-terminal#19 to std's `is_terminal` implementation.
LingMan added a commit to LingMan/console that referenced this issue Aug 30, 2023
In some situations involving long paths it is possible that `GetFileInformationByHandleEx` produces
a `FILE_NAME_INFO` with `FileNameLength` exceeding the provided buffer [1].
Return `false` from `msys_tty_on` if that happens, instead of reading beyond the end of the
allocation.

[1] sunfishcode/is-terminal#18
LingMan added a commit to LingMan/console that referenced this issue Aug 30, 2023
In some situations involving long paths it is possible that `GetFileInformationByHandleEx` produces
a `FILE_NAME_INFO` with `FileNameLength` exceeding the provided buffer [1].
Return `false` from `msys_tty_on` if that happens, instead of reading beyond the end of the
allocation.

[1] sunfishcode/is-terminal#18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants