From e0f3e78fb36fab28f36bd9c6b026607bf24aef2c Mon Sep 17 00:00:00 2001 From: Plecra Date: Sun, 4 Jul 2021 19:51:04 +0100 Subject: [PATCH 1/6] fix dereferencing of unaligned FILE_NAME_INFO --- src/lib.rs | 43 +++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 501cad6..4a2270a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -115,32 +115,51 @@ unsafe fn console_on_any(fds: &[DWORD]) -> bool { /// Returns true if there is an MSYS tty on the given handle. #[cfg(windows)] -unsafe fn msys_tty_on(fd: DWORD) -> bool { +fn msys_tty_on(fd: DWORD) -> bool { use std::{mem, slice}; use winapi::{ ctypes::c_void, shared::minwindef::MAX_PATH, um::{ - fileapi::FILE_NAME_INFO, minwinbase::FileNameInfo, processenv::GetStdHandle, + minwinbase::FileNameInfo, processenv::GetStdHandle, winbase::GetFileInformationByHandleEx, }, }; - let size = mem::size_of::(); - let mut name_info_bytes = vec![0u8; size + MAX_PATH * mem::size_of::()]; - let res = GetFileInformationByHandleEx( - GetStdHandle(fd), - FileNameInfo, - &mut *name_info_bytes as *mut _ as *mut c_void, - name_info_bytes.len() as u32, - ); + /// Mirrors winapi::um::fileapi::FILE_NAME_INFO, giving it a fixed length that + /// we can stack allocate + #[repr(C)] + struct FILE_NAME_INFO { + FileNameLength: DWORD, + FileName: [mem::MaybeUninit; MAX_PATH] + } + let mut name_info = mem::MaybeUninit::::uninit(); + let handle = unsafe { + // Safety: function has no invariants. an invalid handle id will cause + // GetFileInformationByHandleEx to return an error + GetStdHandle(fd) + }; + let res = unsafe { + // Safety: handle is valid, buffer is large enough for expected data + // and we have fully allocated it. + GetFileInformationByHandleEx( + handle, + FileNameInfo, + name_info_bytes.as_mut_ptr() as *mut c_void, + mem::size_of_val(&name_info_bytes) as u32, + ) + }; if res == 0 { return false; } - let name_info: &FILE_NAME_INFO = &*(name_info_bytes.as_ptr() as *const FILE_NAME_INFO); + let name_info = unsafe { + // Safety: The API call succeeded, so name_info has been initialized + // Note name_info.FileName is only partially initialized + &*name_info_bytes.as_ptr() + }; let s = slice::from_raw_parts( - name_info.FileName.as_ptr(), + name_info.FileName.as_ptr() as *const WCHAR, name_info.FileNameLength as usize / 2, ); let name = String::from_utf16_lossy(s); From ba7ef898c62a5925d4e19b00648c0517ecdd4494 Mon Sep 17 00:00:00 2001 From: Plecra Date: Sun, 4 Jul 2021 19:55:24 +0100 Subject: [PATCH 2/6] update variable names --- src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 4a2270a..3828812 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -146,8 +146,8 @@ fn msys_tty_on(fd: DWORD) -> bool { GetFileInformationByHandleEx( handle, FileNameInfo, - name_info_bytes.as_mut_ptr() as *mut c_void, - mem::size_of_val(&name_info_bytes) as u32, + name_info.as_mut_ptr() as *mut c_void, + mem::size_of_val(&name_info) as u32, ) }; if res == 0 { @@ -156,7 +156,7 @@ fn msys_tty_on(fd: DWORD) -> bool { let name_info = unsafe { // Safety: The API call succeeded, so name_info has been initialized // Note name_info.FileName is only partially initialized - &*name_info_bytes.as_ptr() + &*name_info.as_ptr() }; let s = slice::from_raw_parts( name_info.FileName.as_ptr() as *const WCHAR, From 844cc337d53a70779f2e1558396d5e6f507338ad Mon Sep 17 00:00:00 2001 From: Plecra Date: Sun, 4 Jul 2021 19:59:42 +0100 Subject: [PATCH 3/6] update unsafe blocks --- src/lib.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 3828812..3f76bb5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -90,7 +90,7 @@ pub fn is(stream: Stream) -> bool { // Otherwise, we fall back to a very strange msys hack to see if we can // sneakily detect the presence of a tty. - unsafe { msys_tty_on(fd) } + msys_tty_on(fd) } /// returns true if this is _not_ a tty @@ -158,10 +158,13 @@ fn msys_tty_on(fd: DWORD) -> bool { // Note name_info.FileName is only partially initialized &*name_info.as_ptr() }; - let s = slice::from_raw_parts( - name_info.FileName.as_ptr() as *const WCHAR, - name_info.FileNameLength as usize / 2, - ); + let s = unsafe { + // Safety: the buffer is guaranteed to have FileNameLength initialized bytes + slice::from_raw_parts( + name_info.FileName.as_ptr() as *const WCHAR, + name_info.FileNameLength as usize / 2, + ) + }; let name = String::from_utf16_lossy(s); // This checks whether 'pty' exists in the file name, which indicates that // a pseudo-terminal is attached. To mitigate against false positives From 73762b13d5451d04bbc90770d716e996aa014371 Mon Sep 17 00:00:00 2001 From: Plecra Date: Tue, 13 Jul 2021 09:21:55 +0100 Subject: [PATCH 4/6] Minimize unsafety in msys_tty_on --- src/lib.rs | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 3f76bb5..ddc2be6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -132,39 +132,30 @@ fn msys_tty_on(fd: DWORD) -> bool { #[repr(C)] struct FILE_NAME_INFO { FileNameLength: DWORD, - FileName: [mem::MaybeUninit; MAX_PATH] + FileName: [WCHAR; MAX_PATH] } - let mut name_info = mem::MaybeUninit::::uninit(); + let mut name_info = FILE_NAME_INFO { + FileNameLength: 0, + FileName: [0; MAX_PATH] + }; let handle = unsafe { // Safety: function has no invariants. an invalid handle id will cause // GetFileInformationByHandleEx to return an error GetStdHandle(fd) }; let res = unsafe { - // Safety: handle is valid, buffer is large enough for expected data - // and we have fully allocated it. + // Safety: handle is valid, and buffer length is fixed GetFileInformationByHandleEx( handle, FileNameInfo, - name_info.as_mut_ptr() as *mut c_void, - mem::size_of_val(&name_info) as u32, + &mut name_info, + mem::size_of::() as u32, ) }; if res == 0 { return false; } - let name_info = unsafe { - // Safety: The API call succeeded, so name_info has been initialized - // Note name_info.FileName is only partially initialized - &*name_info.as_ptr() - }; - let s = unsafe { - // Safety: the buffer is guaranteed to have FileNameLength initialized bytes - slice::from_raw_parts( - name_info.FileName.as_ptr() as *const WCHAR, - name_info.FileNameLength as usize / 2, - ) - }; + let s = &name_info.FileName[..name_info.FileNameLength]; let name = String::from_utf16_lossy(s); // This checks whether 'pty' exists in the file name, which indicates that // a pseudo-terminal is attached. To mitigate against false positives From 132797297dde571b248eb3451a6d6b521ab61953 Mon Sep 17 00:00:00 2001 From: Plecra Date: Tue, 13 Jul 2021 09:26:48 +0100 Subject: [PATCH 5/6] erase pointer type for winapi call --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index ddc2be6..44e5df2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -148,7 +148,7 @@ fn msys_tty_on(fd: DWORD) -> bool { GetFileInformationByHandleEx( handle, FileNameInfo, - &mut name_info, + &mut name_info as *mut _ as *mut c_void, mem::size_of::() as u32, ) }; From b6322e5f1a953eea752db1c6646280730fc3e6ab Mon Sep 17 00:00:00 2001 From: Plecra Date: Tue, 13 Jul 2021 09:28:48 +0100 Subject: [PATCH 6/6] cast length to usize --- src/lib.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 44e5df2..4895768 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -116,8 +116,6 @@ unsafe fn console_on_any(fds: &[DWORD]) -> bool { /// Returns true if there is an MSYS tty on the given handle. #[cfg(windows)] fn msys_tty_on(fd: DWORD) -> bool { - use std::{mem, slice}; - use winapi::{ ctypes::c_void, shared::minwindef::MAX_PATH, @@ -149,13 +147,13 @@ fn msys_tty_on(fd: DWORD) -> bool { handle, FileNameInfo, &mut name_info as *mut _ as *mut c_void, - mem::size_of::() as u32, + std::mem::size_of::() as u32, ) }; if res == 0 { return false; } - let s = &name_info.FileName[..name_info.FileNameLength]; + let s = &name_info.FileName[..name_info.FileNameLength as usize]; let name = String::from_utf16_lossy(s); // This checks whether 'pty' exists in the file name, which indicates that // a pseudo-terminal is attached. To mitigate against false positives