Skip to content

Commit 0ba3147

Browse files
authored
Merge pull request #1862 from EliahKagan/run-ci/consistent-sh
Make `gix_path::env:shell()` more robust and use it in `gix-command`
2 parents 091d994 + b70cdb1 commit 0ba3147

File tree

8 files changed

+274
-56
lines changed

8 files changed

+274
-56
lines changed

Cargo.lock

+1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

gix-command/Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -24,3 +24,4 @@ shell-words = "1.0"
2424

2525
[dev-dependencies]
2626
gix-testtools = { path = "../tests/tools" }
27+
once_cell = "1.17.1"

gix-command/src/lib.rs

+2-4
Original file line numberDiff line numberDiff line change
@@ -280,10 +280,8 @@ mod prepare {
280280
cmd
281281
}
282282
None => {
283-
let mut cmd = Command::new(
284-
prep.shell_program
285-
.unwrap_or(if cfg!(windows) { "sh" } else { "/bin/sh" }.into()),
286-
);
283+
let shell = prep.shell_program.unwrap_or_else(|| gix_path::env::shell().into());
284+
let mut cmd = Command::new(shell);
287285
cmd.arg("-c");
288286
if !prep.args.is_empty() {
289287
if prep.command.to_str().map_or(true, |cmd| !cmd.contains("$@")) {

gix-command/tests/command.rs

+22-14
Original file line numberDiff line numberDiff line change
@@ -222,10 +222,13 @@ mod context {
222222
}
223223

224224
mod prepare {
225-
#[cfg(windows)]
226-
const SH: &str = "sh";
227-
#[cfg(not(windows))]
228-
const SH: &str = "/bin/sh";
225+
use once_cell::sync::Lazy;
226+
227+
static SH: Lazy<&'static str> = Lazy::new(|| {
228+
gix_path::env::shell()
229+
.to_str()
230+
.expect("`prepare` tests must be run where 'sh' path is valid Unicode")
231+
});
229232

230233
fn quoted(input: &[&str]) -> String {
231234
input.iter().map(|s| format!("\"{s}\"")).collect::<Vec<_>>().join(" ")
@@ -275,7 +278,7 @@ mod prepare {
275278
if cfg!(windows) {
276279
quoted(&["ls", "first", "second", "third"])
277280
} else {
278-
quoted(&[SH, "-c", "ls first second third", "--"])
281+
quoted(&[*SH, "-c", "ls first second third", "--"])
279282
},
280283
"with shell, this works as it performs word splitting"
281284
);
@@ -311,7 +314,8 @@ mod prepare {
311314
if cfg!(windows) {
312315
quoted(&["ls", "--foo", "a b", "additional"])
313316
} else {
314-
format!(r#""{SH}" "-c" "ls --foo \"a b\" \"$@\"" "--" "additional""#)
317+
let sh = *SH;
318+
format!(r#""{sh}" "-c" "ls --foo \"a b\" \"$@\"" "--" "additional""#)
315319
},
316320
"with shell, this works as it performs word splitting"
317321
);
@@ -334,7 +338,7 @@ mod prepare {
334338
let cmd = std::process::Command::from(
335339
gix_command::prepare("ls --foo=\"a b\"").command_may_be_shell_script_disallow_manual_argument_splitting(),
336340
);
337-
assert_eq!(format!("{cmd:?}"), quoted(&[SH, "-c", r#"ls --foo=\"a b\""#, "--"]));
341+
assert_eq!(format!("{cmd:?}"), quoted(&[*SH, "-c", r#"ls --foo=\"a b\""#, "--"]));
338342
}
339343

340344
#[test]
@@ -347,7 +351,7 @@ mod prepare {
347351
);
348352
assert_eq!(
349353
format!("{cmd:?}"),
350-
quoted(&[SH, "-c", "ls \\\"$@\\\"", "--", "--foo=a b"])
354+
quoted(&[*SH, "-c", "ls \\\"$@\\\"", "--", "--foo=a b"])
351355
);
352356
}
353357

@@ -362,7 +366,7 @@ mod prepare {
362366
);
363367
assert_eq!(
364368
format!("{cmd:?}"),
365-
quoted(&[SH, "-c", "\\'ls\\' \\\"$@\\\"", "--", "--foo=a b"]),
369+
quoted(&[*SH, "-c", "\\'ls\\' \\\"$@\\\"", "--", "--foo=a b"]),
366370
"looks strange thanks to debug printing, but is the right amount of quotes actually"
367371
);
368372
}
@@ -379,7 +383,7 @@ mod prepare {
379383
assert_eq!(
380384
format!("{cmd:?}"),
381385
quoted(&[
382-
SH,
386+
*SH,
383387
"-c",
384388
"\\'C:\\\\Users\\\\O\\'\\\\\\'\\'Shaughnessy\\\\with space.exe\\' \\\"$@\\\"",
385389
"--",
@@ -394,9 +398,10 @@ mod prepare {
394398
let cmd = std::process::Command::from(
395399
gix_command::prepare("ls --foo=~/path").command_may_be_shell_script_allow_manual_argument_splitting(),
396400
);
401+
let sh = *SH;
397402
assert_eq!(
398403
format!("{cmd:?}"),
399-
format!(r#""{SH}" "-c" "ls --foo=~/path" "--""#),
404+
format!(r#""{sh}" "-c" "ls --foo=~/path" "--""#),
400405
"splitting can also handle quotes"
401406
);
402407
}
@@ -405,9 +410,10 @@ mod prepare {
405410
fn tilde_path_and_multiple_arguments_as_part_of_command_with_shell() {
406411
let cmd =
407412
std::process::Command::from(gix_command::prepare("~/bin/exe --foo \"a b\"").command_may_be_shell_script());
413+
let sh = *SH;
408414
assert_eq!(
409415
format!("{cmd:?}"),
410-
format!(r#""{SH}" "-c" "~/bin/exe --foo \"a b\"" "--""#),
416+
format!(r#""{sh}" "-c" "~/bin/exe --foo \"a b\"" "--""#),
411417
"this always needs a shell as we need tilde expansion"
412418
);
413419
}
@@ -419,9 +425,10 @@ mod prepare {
419425
.command_may_be_shell_script()
420426
.arg("store"),
421427
);
428+
let sh = *SH;
422429
assert_eq!(
423430
format!("{cmd:?}"),
424-
format!(r#""{SH}" "-c" "echo \"$@\" >&2" "--" "store""#),
431+
format!(r#""{sh}" "-c" "echo \"$@\" >&2" "--" "store""#),
425432
"this is how credential helpers have to work as for some reason they don't get '$@' added in Git.\
426433
We deal with it by not doubling the '$@' argument, which seems more flexible."
427434
);
@@ -435,9 +442,10 @@ mod prepare {
435442
.with_quoted_command()
436443
.arg("store"),
437444
);
445+
let sh = *SH;
438446
assert_eq!(
439447
format!("{cmd:?}"),
440-
format!(r#""{SH}" "-c" "echo \"$@\" >&2" "--" "store""#)
448+
format!(r#""{sh}" "-c" "echo \"$@\" >&2" "--" "store""#)
441449
);
442450
}
443451
}

gix-credentials/tests/program/from_custom_definition.rs

+17-9
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,17 @@
11
use gix_credentials::{helper, program::Kind, Program};
2+
use once_cell::sync::Lazy;
23

3-
static GIT: once_cell::sync::Lazy<&'static str> =
4-
once_cell::sync::Lazy::new(|| gix_path::env::exe_invocation().to_str().expect("not illformed"));
4+
static GIT: once_cell::sync::Lazy<&'static str> = once_cell::sync::Lazy::new(|| {
5+
gix_path::env::exe_invocation()
6+
.to_str()
7+
.expect("some `from_custom_definition` tests must be run where 'git' path is valid Unicode")
8+
});
59

6-
#[cfg(windows)]
7-
const SH: &str = "sh";
8-
#[cfg(not(windows))]
9-
const SH: &str = "/bin/sh";
10+
static SH: Lazy<&'static str> = Lazy::new(|| {
11+
gix_path::env::shell()
12+
.to_str()
13+
.expect("some `from_custom_definition` tests must be run where 'sh' path is valid Unicode")
14+
});
1015

1116
#[test]
1217
fn empty() {
@@ -47,11 +52,12 @@ fn name_with_args() {
4752
fn name_with_special_args() {
4853
let input = "name --arg --bar=~/folder/in/home";
4954
let prog = Program::from_custom_definition(input);
55+
let sh = *SH;
5056
let git = *GIT;
5157
assert!(matches!(&prog.kind, Kind::ExternalName{name_and_args} if name_and_args == input));
5258
assert_eq!(
5359
format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
54-
format!(r#""{SH}" "-c" "{git} credential-name --arg --bar=~/folder/in/home \"$@\"" "--" "store""#)
60+
format!(r#""{sh}" "-c" "{git} credential-name --arg --bar=~/folder/in/home \"$@\"" "--" "store""#)
5561
);
5662
}
5763

@@ -73,12 +79,13 @@ fn path_with_args_that_definitely_need_shell() {
7379
let input = "/abs/name --arg --bar=\"a b\"";
7480
let prog = Program::from_custom_definition(input);
7581
assert!(matches!(&prog.kind, Kind::ExternalPath{path_and_args} if path_and_args == input));
82+
let sh = *SH;
7683
assert_eq!(
7784
format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
7885
if cfg!(windows) {
7986
r#""/abs/name" "--arg" "--bar=a b" "store""#.to_owned()
8087
} else {
81-
format!(r#""{SH}" "-c" "/abs/name --arg --bar=\"a b\" \"$@\"" "--" "store""#)
88+
format!(r#""{sh}" "-c" "/abs/name --arg --bar=\"a b\" \"$@\"" "--" "store""#)
8289
}
8390
);
8491
}
@@ -100,12 +107,13 @@ fn path_with_simple_args() {
100107
let input = "/abs/name a b";
101108
let prog = Program::from_custom_definition(input);
102109
assert!(matches!(&prog.kind, Kind::ExternalPath{path_and_args} if path_and_args == input));
110+
let sh = *SH;
103111
assert_eq!(
104112
format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
105113
if cfg!(windows) {
106114
r#""/abs/name" "a" "b" "store""#.to_owned()
107115
} else {
108-
format!(r#""{SH}" "-c" "/abs/name a b \"$@\"" "--" "store""#)
116+
format!(r#""{sh}" "-c" "/abs/name a b \"$@\"" "--" "store""#)
109117
},
110118
"a shell is used as there are arguments, and it's generally more flexible, but on windows we split ourselves"
111119
);

gix-path/src/env/auxiliary.rs

+179
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
use std::ffi::OsString;
2+
use std::path::{Path, PathBuf};
3+
4+
use once_cell::sync::Lazy;
5+
6+
/// `usr`-like directory component names that MSYS2 may provide, other than for `/usr` itself.
7+
///
8+
/// These are the values of the "Prefix" column of the "Environments" and "Legacy Environments"
9+
/// tables in the [MSYS2 Environments](https://www.msys2.org/docs/environments/) documentation,
10+
/// with the leading `/` separator removed, except that this does not list `usr` itself.
11+
///
12+
/// On Windows, we prefer to use `sh` as provided by Git for Windows, when present. To find it, we
13+
/// run `git --exec-path` to get a path that is usually `<platform>/libexec/git-core` in the Git
14+
/// for Windows installation, where `<platform>` is something like `mingw64`. It is also acceptable
15+
/// to find `sh` in an environment not provided by Git for Windows, such as an independent MSYS2
16+
/// environment in which a `git` package has been installed. However, in an unusual installation,
17+
/// or if the user has set a custom value of `GIT_EXEC_PATH`, the output of `git --exec-path` may
18+
/// take a form other than `<platform>/libexec/git-core`, such that finding shell at a location
19+
/// like `../../../bin/sh.exe` relative to it should not be attempted. We lower the risk by
20+
/// checking that `<platform>` is a plausible value that is not likely to have any other meaning.
21+
///
22+
/// This involves two tradeoffs. First, it may be reasonable to find `sh.exe` in an environment
23+
/// that is not MSYS2 at all, for which in principle the prefix could be different. But listing
24+
/// more prefixes or matching a broad pattern of platform-like strings might be too broad. So only
25+
/// prefixes that have been used in MSYS2 are considered.
26+
///
27+
/// Second, we don't recognize `usr` itself here, even though it is a plausible prefix. In MSYS2,
28+
/// it is the prefix for MSYS2 non-native programs, i.e. those that use `msys-2.0.dll`. But unlike
29+
/// the `<platform>` names we recognize, `usr` also has an effectively unbounded range of plausible
30+
/// meanings on non-Unix systems (for example, what should we take `Z:\usr` to mean?), which might
31+
/// occasionally relate to subdirectories with contents controlled by different *user accounts*.
32+
///
33+
/// If we start with a `libexec/git-core` directory that we already use and trust, and it is in a
34+
/// directory with a name like `mingw64`, we infer that this `mingw64` directory has the expected
35+
/// meaning and accordingly infer that its `usr` sibling, if present, is acceptable to treat as
36+
/// though it is a first-level directory inside an MSYS2-like tree. So we are willing to traverse
37+
/// down to `usr/sh.exe` and try to use it. But if the `libexec/git-core` we use and trust is in a
38+
/// directory named `usr`, that `usr` directory may still not have the meaning we expect of `usr`.
39+
///
40+
/// Conditions for a privilege escalation attack or other serious malfunction seem far-fetched. If
41+
/// further research finds the risk is low enough, `usr` may be added. But for now it is omitted.
42+
const MSYS_USR_VARIANTS: &[&str] = &["mingw64", "mingw32", "clangarm64", "clang64", "clang32", "ucrt64"];
43+
44+
/// Find a Git for Windows installation directory based on `git --exec-path` output.
45+
///
46+
/// Currently this is used only for finding the path to an `sh.exe` associated with Git. This is
47+
/// separate from `installation_config()` and `installation_config_prefix()` in `gix_path::env`.
48+
/// This is *not* suitable for finding the highest-scoped configuration file, because that could be
49+
/// installed in an unusual place, or customized via `GIT_CONFIG_SYSTEM` or `GIT_CONFIG_NOSYSTEM`,
50+
/// all of which `installation_config()` should reflect. Likewise, `installation_config_prefix()`
51+
/// has strong uses, such as to find a directory inside `ProgramData` containing configuration.
52+
/// But it is possible that some marginal uses of `installation_config_prefix()`, if they do not
53+
/// really relate to configuration, could be replaced with `git_for_windows_root()` in the future.
54+
fn git_for_windows_root() -> Option<&'static Path> {
55+
static GIT_ROOT: Lazy<Option<PathBuf>> = Lazy::new(|| {
56+
super::core_dir()
57+
.filter(|core| {
58+
// Only use this if the directory structure resembles a Git installation. This
59+
// accepts installations of common types that are not broken when `GIT_EXEC_PATH`
60+
// is unset, as well as values of `GIT_EXEC_PATH` that are likely to be usable.
61+
core.is_absolute() && core.ends_with("libexec/git-core")
62+
})
63+
.and_then(|core| core.ancestors().nth(2))
64+
.filter(|prefix| {
65+
// Only use `libexec/git-core` from inside something `usr`-like, such as `mingw64`.
66+
// See `MSYS_USR_VARIANTS` for details and the rationale for this restriction.
67+
MSYS_USR_VARIANTS.iter().any(|name| prefix.ends_with(name))
68+
})
69+
.and_then(|prefix| prefix.parent())
70+
.map(Into::into)
71+
});
72+
GIT_ROOT.as_deref()
73+
}
74+
75+
/// `bin` directory paths to try relative to the root of a Git for Windows or MSYS2 installation.
76+
///
77+
/// These are ordered so that a shim is preferred over a non-shim when they are tried in order.
78+
const BIN_DIR_FRAGMENTS: &[&str] = &["bin", "usr/bin"];
79+
80+
/// Obtain a path to an executable command on Windows associated with Git, if one can be found.
81+
///
82+
/// The resulting path uses only `/` separators so long as the path obtained from `git --exec-path`
83+
/// does, which is the case unless it is overridden by setting `GIT_EXEC_PATH` to an unusual value.
84+
///
85+
/// This is currently only used (and only heavily exercised in tests) for finding `sh.exe`. It may
86+
/// be used to find other executables in the future, but may need adjustment. In particular,
87+
/// depending on desired semantics, it should possibly also check a `cmd` directory; directories
88+
/// like `<platform>/bin`, for any applicable variants (such as `mingw64`); and `super::core_dir()`
89+
/// itself, which it could safely check even if its value is not safe for inferring other paths.
90+
fn find_git_associated_windows_executable(stem: &str) -> Option<OsString> {
91+
let git_root = git_for_windows_root()?;
92+
93+
BIN_DIR_FRAGMENTS
94+
.iter()
95+
.map(|bin_dir_fragment| {
96+
// Perform explicit raw concatenation with `/` to avoid introducing any `\` separators.
97+
let mut raw_path = OsString::from(git_root);
98+
raw_path.push("/");
99+
raw_path.push(bin_dir_fragment);
100+
raw_path.push("/");
101+
raw_path.push(stem);
102+
raw_path.push(".exe");
103+
raw_path
104+
})
105+
.find(|raw_path| Path::new(raw_path).is_file())
106+
}
107+
108+
/// Like `find_associated_windows_executable`, but if not found, fall back to a simple filename.
109+
pub(super) fn find_git_associated_windows_executable_with_fallback(stem: &str) -> OsString {
110+
find_git_associated_windows_executable(stem).unwrap_or_else(|| {
111+
let mut raw_path = OsString::from(stem);
112+
raw_path.push(".exe");
113+
raw_path
114+
})
115+
}
116+
117+
#[cfg(test)]
118+
mod tests {
119+
use std::path::Path;
120+
121+
/// Some commands with `.exe` files in `bin` and `usr/bin` that should be found.
122+
///
123+
/// Tests are expected to run with a full Git for Windows installation (not MinGit).
124+
const SHOULD_FIND: &[&str] = &[
125+
"sh", "bash", "dash", "diff", "tar", "less", "sed", "awk", "perl", "cygpath",
126+
];
127+
128+
/// Shouldn't find anything nonexistent, or only in PATH or in `bin`s we don't mean to search.
129+
///
130+
/// If dirs like `mingsw64/bin` are added, `git-credential-manager` should be moved to `SHOULD_FIND`.
131+
/// Likewise, if `super::core_dir()` is added, `git-daemon` should be moved to `SHOULD_FIND`.
132+
const SHOULD_NOT_FIND: &[&str] = &[
133+
"nonexistent-command",
134+
"cmd",
135+
"powershell",
136+
"explorer",
137+
"git-credential-manager",
138+
"git-daemon",
139+
];
140+
141+
#[test]
142+
#[cfg_attr(not(windows), ignore)]
143+
fn find_git_associated_windows_executable() {
144+
for stem in SHOULD_FIND {
145+
let path = super::find_git_associated_windows_executable(stem);
146+
assert!(path.is_some(), "should find {stem:?}");
147+
}
148+
}
149+
150+
#[test]
151+
#[cfg_attr(not(windows), ignore)]
152+
fn find_git_associated_windows_executable_no_extra() {
153+
for stem in SHOULD_NOT_FIND {
154+
let path = super::find_git_associated_windows_executable(stem);
155+
assert_eq!(path, None, "should not find {stem:?}");
156+
}
157+
}
158+
159+
#[test]
160+
#[cfg_attr(not(windows), ignore)]
161+
fn find_git_associated_windows_executable_with_fallback() {
162+
for stem in SHOULD_FIND {
163+
let path = super::find_git_associated_windows_executable_with_fallback(stem);
164+
assert!(Path::new(&path).is_absolute(), "should find {stem:?}");
165+
}
166+
}
167+
168+
#[test]
169+
#[cfg_attr(not(windows), ignore)]
170+
fn find_git_associated_windows_executable_with_fallback_falls_back() {
171+
for stem in SHOULD_NOT_FIND {
172+
let path = super::find_git_associated_windows_executable_with_fallback(stem)
173+
.to_str()
174+
.expect("valid Unicode")
175+
.to_owned();
176+
assert_eq!(path, format!("{stem}.exe"), "should fall back for {stem:?}");
177+
}
178+
}
179+
}

0 commit comments

Comments
 (0)