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

Replace which with a manual implementation using SearchPathW #96

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .cargo/config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,8 @@ rustflags = ["-Clink-args=/DEFAULTLIB:rpcrt4.lib"]
# -Clink-args=/DYNAMICBASE /CETCOMPAT: Enable "shadow stack" (https://learn.microsoft.com/en-us/cpp/build/reference/cetcompat)
[target.'cfg(all(target_os = "windows", any(target_arch = "x86", target_arch = "x86_64")))']
rustflags = ["-Clink-args=/DYNAMICBASE /CETCOMPAT"]

[registries]
Sudo_PublicPackages = { index = "sparse+https://pkgs.dev.azure.com/shine-oss/sudo/_packaging/Sudo_PublicPackages/Cargo/index/" }
[source.crates-io]
replace-with = "Sudo_PublicPackages"
4 changes: 2 additions & 2 deletions .cargo/ms-toolchain-config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@ rustflags = ["-Clink-args=/DYNAMICBASE /CETCOMPAT"]
# Setup the ADO Artifacts feed as a Registry: you'll need to use your own feed in your project that upstreams from crates.io.
# For more details see https://eng.ms/docs/cloud-ai-platform/devdiv/one-engineering-system-1es/1es-docs/azure-artifacts/cargo
[registries]
sudo_public_cargo = { index = "sparse+https://pkgs.dev.azure.com/microsoft/Dart/_packaging/sudo_public_cargo/Cargo/index/" }
Sudo_PublicPackages = { index = "sparse+https://pkgs.dev.azure.com/shine-oss/sudo/_packaging/Sudo_PublicPackages/Cargo/index/" }
[source.crates-io]
replace-with = "sudo_public_cargo"
replace-with = "Sudo_PublicPackages"
18 changes: 18 additions & 0 deletions Building.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,21 @@ cargo build --config .cargo\ms-toolchain-config.toml
```

Note, if you run that on the public toolchain, you'll most definitely run into ``error: unknown codegen option: `ehcont_guard` `` when building.

### Notes on updating the cargo feed

For internal reasons, we need to maintain a separate Azure Artifacts cargo feed.
Largely we just pull dependencies through from crates.io into that feed. Hence
the config to replace the default cargo feed with our own.

As a maintainer, if you need to update that feed, You should check out the steps
under ["Cargo" on this
page](https://dev.azure.com/shine-oss/sudo/_artifacts/feed/Sudo_PublicPackages).
TL;DR, do the following:

```cmd
az login
az account get-access-token --query "join(' ', ['Bearer', accessToken])" --output tsv | cargo login --registry Sudo_PublicPackages
```

That'll log you in via the Azure CLI and then log you into the cargo feed. I believe that should allow you to pull packages through to the Azure feed, from the public feed. If it doesn't, maintainers should have the necessary permissions to add packages to the feed.
72 changes: 1 addition & 71 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ cc = "1.0"
# See: https://docs.rs/clap/latest/clap/_features/index.html
clap = { version = "4.4.7", default-features = false, features = ["std"] }
embed-manifest = "1.4"
which = "6.0"
win_etw_provider = "0.1.8"
win_etw_macros = "0.1.8"
windows = "0.57"
Expand Down
8 changes: 6 additions & 2 deletions sudo/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,16 @@ name = "sudo"
winres.workspace = true
cc.workspace = true
embed-manifest.workspace = true
which = { workspace = true }

[build-dependencies.windows]
workspace = true
features = [
"Win32_Storage_FileSystem",
]

[dependencies]

clap = { workspace = true, default-features = false, features = ["color", "help", "usage", "error-context"] }
which = { workspace = true }
windows-registry = { workspace = true }

sudo_events = { path = "../sudo_events" }
Expand Down
19 changes: 17 additions & 2 deletions sudo/build.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use embed_manifest::embed_manifest_file;
use std::path::PathBuf;
use std::process::Command;
use windows::{core::*, Win32::Storage::FileSystem::*};
use {
std::{env, io},
winres::WindowsResource,
Expand Down Expand Up @@ -32,6 +33,21 @@ fn get_sdk_path() -> Option<String> {
sdk_path
}

fn search_path(filename: &str) -> Result<String> {
let filename = &HSTRING::from(filename);
let len = unsafe { SearchPathW(None, filename, None, None, None) };

if len == 0 {
return Err(Error::from_win32());
}

let mut buffer = vec![0; len as usize];
let len = unsafe { SearchPathW(None, filename, None, Some(&mut buffer), None) };
buffer.truncate(len as usize);

Ok(String::from_utf16(&buffer)?)
}

fn get_sdk_tool(sdk_path: &Option<String>, tool_name: &str) -> String {
// seems like, in a VS tools prompt, midl.exe is in the path so the above
// doesn't include the path. kinda weird but okay?
Expand All @@ -47,8 +63,7 @@ fn get_sdk_tool(sdk_path: &Option<String>, tool_name: &str) -> String {
// we can just get the absolute path to the exe using the windows
// path search.

let tool_path = which::which(tool_name).expect("Failed to find tool in path");
tool_path.to_str().unwrap().to_owned()
search_path(tool_name).expect("Failed to find tool in path")
}
};
tool_path
Expand Down
67 changes: 65 additions & 2 deletions sudo/src/run_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Comment on lines +195 to +204
Copy link
Member

@lhecker lhecker Aug 9, 2024

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:

let pathext = env::var("PATHEXT");
let extensions = pathext
    .as_deref()
    // If PATHEXT isn't set, use the default
    .unwrap_or(".COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC")
    .split(';');

Copy link
Member

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.


/// 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) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fairly certain that SearchPathW doesn't take an HSTRING o_O

Is this a weird rust-win32ism?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SearchPathW expects a 16-bit wide string but you're starting with a UTF-8 string. HSTRING provides a 16-bite wide string and comprehensive conversion from various Rust Standard Library types, so it's very convenient in cases like this.

Copy link
Member

@DHowett DHowett Aug 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike BSTR, HSTRING is not compatible when passed as a LPWSTR. It does not point directly to the character data. Why is passing it like a LPWSTR to an API that expects LPWSTR OK, in contravention of all Windows API design to-date?

Copy link
Member

Choose a reason for hiding this comment

The 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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume by LPWSTR you really mean PCWSTR. 😉

In which case, HSTRING is actually a far better choice than BSTR. While BSTR is literally the pointer, that pointer is far more dangerous in general and BSTR semantics are far more nebulous than HSTRING. An HSTRING is perfectly compatible with PCWSTR and even has an API for that - WindowsGetStringRawBuffer - but I implemented it inline for both C++/WinRT and Rust and there is thus no performance penalty for pointer access. HSTRING is generally more efficient, can avoid heap allocations, and while windows-strings does provide all kinds of conveniences for both HSTRING and BSTR, the former is preferred.


// 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)?);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW you could also consider returning an OsString or even better Path here, as that would be consistent with other parts of Rust and this app. It would also allow you to revert the change to absolute_path below.

}

// 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);
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -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.
Expand All @@ -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);
Expand Down Expand Up @@ -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();
Expand Down
Loading