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

Handle GetFileInformationByHandleEx returning a long filename. #19

Merged
merged 3 commits into from
Feb 19, 2023

Conversation

sunfishcode
Copy link
Owner

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.

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 Author

I've now also sync'd the crates's code with updates from std, which was also forked from the original atty code, except for one change:

std's version uses raw pointer arithmetic here to form the name slice, so it wouldn't catch this out of bounds error. If there are no problems with the version in this PR, I'll submit a PR to Rust to fix this upstream too.

@LingMan
Copy link

LingMan commented Feb 19, 2023

Wouldn't it be better to allocate a bigger buffer and try again if the FileNameLength field indicates a size bigger than what was passed in is needed?

If the call to GetFileInformationByHandleEx fails because there was not enough buffer space for the full length of the FileName then the FileNameLength field will contain the required length of the FileName in bytes.
https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_name_info

@ChrisDenton
Copy link

ChrisDenton commented Feb 19, 2023

In general. yes. but in this case we're only interested in a very specific pipe name. In fact it would just do an exact string match except that the name includes a random component and the precise format may change between versions.

This code is basically a hack to detect msys2 or cygwin emulated consoles (with other hacks to lower the risk of false positives).

@LingMan
Copy link

LingMan commented Feb 19, 2023

Alright, if the specific pipe name is known to be shorter than MAX_PATH, staying with the simpler solution is probably a good idea.

@sunfishcode sunfishcode merged commit 5b264e5 into main Feb 19, 2023
@sunfishcode sunfishcode deleted the sunfishcode/long-filenames branch February 19, 2023 20:37
sunfishcode added a commit to sunfishcode/rust that referenced this pull request 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.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request 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 pull request 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 pull request 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 pull request 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.
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

Successfully merging this pull request may close these issues.

3 participants