-
Notifications
You must be signed in to change notification settings - Fork 137
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
Replace which with a manual implementation using SearchPathW #96
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,6 +191,53 @@ pub fn run_target( | |
do_request(req, copy_env, manually_requested_dir) | ||
} | ||
|
||
/// Returns all the extensions from PATHEXT that are used for "executables" | ||
fn get_path_extensions() -> Vec<String> { | ||
env::var("PATHEXT") | ||
.unwrap_or_else(|_| { | ||
// If PATHEXT isn't set, use the default | ||
".COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC".to_string() | ||
}) | ||
.split(';') | ||
.map(|ext| ext.to_string()) | ||
.collect() | ||
} | ||
|
||
/// Searches the PATH for the given filename, with the extensions from PATHEXT. | ||
/// Returns the full path to the file if it's found, or an error if it's not. | ||
fn search_path_with_extensions(filename: &str) -> Result<String> { | ||
let filename = HSTRING::from(filename); | ||
let mut buffer = vec![0; 260]; | ||
let extensions = get_path_extensions(); | ||
for extension in extensions { | ||
let extension_hstring = HSTRING::from(extension); | ||
// SearchPathW will ignore the extension if the filename already has one. | ||
let len: u32 = | ||
unsafe { SearchPathW(None, &filename, &extension_hstring, Some(&mut buffer), None) }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am fairly certain that SearchPathW doesn't take an Is this a weird There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unlike There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I am happy to accept “there are magic conversions in the rust Windows crate,” but that is exactly what I mean when I ask if it is a weirdness. I also want somebody to say it on-record.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume by In which case, |
||
|
||
// If the function fails, the return value is zero. | ||
if len == 0 { | ||
continue; | ||
} | ||
|
||
// If the function succeeds, the value returned is the | ||
// length of the string that is copied to the buffer, | ||
// not including the terminating null character. | ||
if len < buffer.len() as u32 { | ||
buffer.truncate(len as usize); | ||
return Ok(String::from_utf16(&buffer)?); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW you could also consider returning an |
||
} | ||
|
||
// If the return value is greater than nBufferLength, the value | ||
// returned is the size of the buffer that is required to hold | ||
// the path, including the terminating null character. | ||
// | ||
// Includes some padding just in case and because it's cheap. | ||
buffer.resize((len + 64) as usize, 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, doesn't this mean that if it fails to fit in the buffer we will move on to the next extension after growing the buffer? That doesn't seem correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hah. I didn't even catch this! My initial suggestion had a loop, so this looked quite similar. 😅 But it needs a loop in a loop. |
||
} | ||
Err(Error::from_win32()) | ||
} | ||
|
||
/// Constructs an ElevateRequest from the given arguments. We'll package up | ||
/// handles, we'll separate out the application and args, and we'll do some | ||
/// other work to make sure the request is ready to go. | ||
|
@@ -262,7 +309,7 @@ fn prepare_request( | |
event_log_request(true, &req); | ||
|
||
// Does the application exist somewhere on the path? | ||
let where_result = which::which(&req.application); | ||
let where_result = search_path_with_extensions(&req.application); | ||
|
||
if let Ok(path) = where_result { | ||
// It's a real file that exists on the PATH. | ||
|
@@ -271,7 +318,9 @@ fn prepare_request( | |
// ensures that the elevated sudo will execute the same thing that was | ||
// found here in the unelevated context. | ||
|
||
req.application = absolute_path(&path)?.to_string_lossy().to_string(); | ||
req.application = absolute_path(Path::new(&path))? | ||
.to_string_lossy() | ||
.to_string(); | ||
adjust_args_for_gui_exes(&mut req); | ||
} else { | ||
tracing::trace_command_not_found(&req.application); | ||
|
@@ -521,6 +570,20 @@ fn runas_admin_impl(exe: &OsStr, args: &OsStr, show: SHOW_WINDOW_CMD) -> Result< | |
mod tests { | ||
use super::*; | ||
|
||
#[test] | ||
fn test_search_path_cmd() { | ||
let path = search_path_with_extensions("cmd").unwrap(); | ||
assert!(path.eq_ignore_ascii_case("C:\\Windows\\System32\\cmd.exe")); | ||
let path = search_path_with_extensions("cmd.exe").unwrap(); | ||
assert!(path.eq_ignore_ascii_case("C:\\Windows\\System32\\cmd.exe")); | ||
} | ||
#[test] | ||
fn test_search_path_bad() { | ||
// There's no way this file exists | ||
let path = search_path_with_extensions("acaeb0cf7e91430eb8958a64bee752a7").unwrap_err(); | ||
assert_eq!(path.code().0, E_FILENOTFOUND.0); | ||
} | ||
|
||
#[test] | ||
fn test_cmd_is_cui() { | ||
let app_name = "cmd".to_string(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this was inside
search_path_with_extensions
it wouldn't need to collect into a vector first, nor allocate each string chunk.Like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it some more, I think it'd be preferable if we still did this, but it's not something I would block over.