Skip to content

Commit

Permalink
fix(edit): try harder on Windows to run editor
Browse files Browse the repository at this point in the history
This emulates more of git's Windows-specific behavior for launching
interactive editors.

When GIT_WINDOWS_NATIVE is defined, git uses `mingw_spawnvpe()` to spawn
the editor which, in turn, calls a `parse_interpreter()`. The proposed
editor command is run as-is if it has a .exe extension. Otherwise, the
command path is opened, read, and parsed to find a shebang line with an
interpreter path. This plays out in run-command.c and compat/mingw.c in
Git's source code.

The Windows CI is updated to set SHELL_PATH to something suitable for
the msys2 environment; `/bin/sh` is **not** a viable path in this
environment.

Refs: #407
  • Loading branch information
jpgrayson committed Feb 2, 2024
1 parent 048f182 commit 9329c3f
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 11 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ jobs:
STG_TEST_OPTS: "--verbose-log"
STG_PROFILE: "release"
run: |
timeout 900s make -C t prove
timeout 900s make -C t SHELL_PATH=C:/msys64/usr/bin/bash prove
- name: Show Failures
if: ${{ failure() }}
shell: msys2 {0}
Expand Down
106 changes: 96 additions & 10 deletions src/patch/edit/interactive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@
use std::{
ffi::{OsStr, OsString},
fs::File,
io::{BufWriter, Write},
io::{BufWriter, Read, Write},
path::Path,
};

use anyhow::{anyhow, Context, Result};
use bstr::BString;
use bstr::{BString, ByteSlice};

use super::description::{EditablePatchDescription, EditedPatchDescription};

Expand Down Expand Up @@ -113,14 +113,25 @@ pub(crate) fn call_editor<P: AsRef<Path>>(
}

fn run_editor<P: AsRef<Path>>(editor: &OsStr, path: P) -> Result<std::process::ExitStatus> {
let mut child = std::process::Command::from(
gix_command::prepare(editor)
.arg(path.as_ref())
.with_shell_allow_argument_splitting()
.stdout(std::process::Stdio::inherit()),
)
.spawn()
.with_context(|| format!("running editor: {}", editor.to_string_lossy()))?;
let prep = gix_command::prepare(editor)
.arg(path.as_ref())
.with_shell_allow_argument_splitting()
.stdout(std::process::Stdio::inherit());

let mut command = if cfg!(windows) && !prep.use_shell {
if let Some(interpreter) = parse_interpreter(&prep.command) {

This comment has been minimized.

Copy link
@Byron

Byron Feb 3, 2024

Contributor

I just realised that this is fortunately not what Git is doing as it probably searches command in PATH first. Otherwise an attacker can probably create files with shebangs in a poisoned repository and hope that one of these is an editor, to control which executable it will execute.

As options are ignored, it's probably much less of a problem, even though my implementation currently handles arguments as well.

I will be sure to search in PATH before opening a file, and probably ignore options as well in case I have to try a relative path.

Looking at it, it seems that Git will also open relative paths, which is probably why the options are stripped in the first place.

This comment has been minimized.

Copy link
@Byron

Byron Feb 3, 2024

Contributor

I have created a new release of gix-command which should do exactly what Git would, which should allow you to remove the extra code dealing with interpreter parsing. Here is the changes that went into this release: GitoxideLabs/gitoxide#1274 .

This comment has been minimized.

Copy link
@jpgrayson

jpgrayson Feb 5, 2024

Author Collaborator

I've pushed 03b0650 to use gix-command 0.3.4 and drop StGit's custom code.

Thank you so much for your help with this issue. I really appreciate the work you're doing with gitoxide and the responsiveness you've given to StGit. 🙏

This comment has been minimized.

Copy link
@Byron

Byron Feb 5, 2024

Contributor

Thanks you, likewise, and always a pleasure :)!

(When bugs or shortcomings come up, I always drop all feature work and address them first if feasible.)

let mut cmd = std::process::Command::new(interpreter);
cmd.arg(prep.command).arg(path.as_ref());
cmd
} else {
std::process::Command::from(prep)
}
} else {
std::process::Command::from(prep)
};
let mut child = command
.spawn()
.with_context(|| format!("running editor: {}", editor.to_string_lossy()))?;

child
.wait()
Expand Down Expand Up @@ -152,3 +163,78 @@ fn get_editor(config: &gix::config::Snapshot) -> Result<OsString> {
};
Ok(editor)
}

fn parse_interpreter(command: &OsStr) -> Option<OsString> {
let command_path = Path::new(command);
if command_path.extension().and_then(|ext| ext.to_str()) == Some("exe") {
return None;
}

let mut buffer = [0; 128];
if let Some(n) = std::fs::File::open(command_path)
.ok()
.and_then(|mut file| file.read(&mut buffer).ok())
{
parse_shebang(&buffer[..n])
.and_then(|bytes| bytes.to_os_str().ok())
.map(|osstr| osstr.to_os_string())
} else {
None
}
}

fn parse_shebang(buffer: &[u8]) -> Option<&[u8]> {
buffer
.as_bstr()
.lines()
.next()
.and_then(|line| line.strip_prefix(b"#!"))
.and_then(|shebang| {
shebang.rfind_byteset(b"/\\").map(|index| {
if let Some(space_index) = shebang[index..].find_byte(b' ') {
&shebang[..index + space_index]
} else {
shebang
}
})
})
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn plain_shebang() {
assert_eq!(parse_shebang(b"#!/bin/sh\nsome stuff").unwrap(), b"/bin/sh");
}

#[test]
fn shebang_with_options() {
assert_eq!(
parse_shebang(b"#!/bin/sh -i -o -u\nsome stuff").unwrap(),
b"/bin/sh"
);
}

#[test]
fn shebang_with_backslashes() {
assert_eq!(
parse_shebang(b"#!C:\\Program Files\\Imashell.exe\nsome stuff").unwrap(),
b"C:\\Program Files\\Imashell.exe"
);
}

#[test]
fn shebang_with_trailing_space() {
assert_eq!(
parse_shebang(b"#!/bin/sh \nsome stuff").unwrap(),
b"/bin/sh"
);
}

#[test]
fn not_a_shebang() {
assert!(parse_shebang(b"/bin/sh\nsome stuff").is_none());
}
}

0 comments on commit 9329c3f

Please sign in to comment.