Skip to content

Commit 55ea3fd

Browse files
Stop wrapping commands in cmd.exe on Windows (#898)
An attempt at Issue #847. This isn't a corner case: `pre-commit` and `commit-msg` hooks both stash unstaged changes before running hooks, by default, so any Windows user doing a partial commit (staging some but not all changes) hits this — hooks fail outright with `error: unknown switch '0'` instead of running at all. I [commented on the issue](#847 (comment)) a couple of months ago; I've since tested this fix end to end on Windows 11 / Ruby 3.3, and while the suggestion still looks right, my description of the cause back then was off. Here's what I think is actually going on. ## The problem `win32_prepare_args` escapes metacharacters per argument, then joins everything into one string behind `cmd.exe /c`: ```ruby %w[cmd.exe /c] + [args.join(' ')] ``` The escaping doesn't seem to be what breaks; the join is. It collapses the command into a single argv element, and childprocess 5.x on Windows is a thin shim over `Process.spawn`, which sees the embedded spaces, wraps the element in quotes, and backslash-escapes the quotes already inside it. cmd.exe strips the outer pair, and the child's C runtime then reads the surviving `\"` as a literal quote rather than a delimiter — so the argument re-splits on whitespace. With the pre-commit stash message (`Overcommit: Stash of repo state before hook run at 2024-04-10 12:34:56 -0700`), `git stash save` receives the UTC offset as a switch of its own and aborts with `error: unknown switch '0'`. On a throwaway repo I get exit 129 with that message on `main`, and exit 0 with the message intact on this branch. I couldn't find an escaping scheme that fixes this while keeping the wrapper: inside double quotes `& | < > ( )` are already literal, `%VAR%` still expands and can't be caret-escaped, and carets inside quotes reach the child as literal carets. ## The change Drop the wrapper and hand the argument vector to `ChildProcess.build` unmodified. `Process.spawn` appears to do the command lookup the wrapper was standing in for — `.bat`, `.cmd`, extensionless RubyGems shims and `PATHEXT` names all launched fine in my testing, including the real `bundle.bat` — and arguments stay atomic because no shell re-parses them. Both call sites were already gated on `OS.windows?`, so this is Windows-only in effect; macOS and Linux behaviour is unchanged. That `Process.spawn` backend arrived in childprocess 5.0.0, which replaced the Windows FFI layer. Before that, Windows used raw `CreateProcess`, which does no `PATHEXT` resolution, so I've raised the gemspec floor from `>= 0.6.3` to `>= 5.0.0`. I realise that's the debatable part and entirely your call — for context, the `< 6` ceiling was already there, 5.0.0 is from 2022, this repo's `Gemfile.lock` resolves to 5.x, and 4.1.0/5.0.0/5.1.0 all declare `required_ruby_version >= 2.4.0`. One wrinkle: `mklink` and `dir` (used by `FileUtils` for symlinks) are cmd.exe built-ins with no standalone executable, so dropping the wrapper broke `FileUtils.symlink`. Those two call sites now pass `cmd.exe`, `/c`, and the command as separate argv elements, which avoids reintroducing the join-based bug above. ## Tests Adds `spec/overcommit/subprocess_spec.rb` — there wasn't coverage for `Subprocess` before. It uses `Gem.ruby` and a NUL-joined `ARGV` dump so it should be platform-agnostic and pass on the current Linux CI. It covers the #847 case directly, plus results and exit status, `options[:input]`, metacharacter and empty-string arguments, and that `ChildProcess.build` receives the argv verbatim from both `.spawn` and `.spawn_detached`. On Windows locally, `bundle exec overcommit --run` passes and rubocop is clean. `bundle exec rspec` gives 1643 examples / 2 failures, the same as on `main` (`git_repo_spec.rb:341`, and `utils_spec.rb:142`, which needs `wmic`); `git_repo_spec.rb:218` fails on `main` and passes here. ## Notes - I also dropped the now-unused `require 'overcommit/os'`; happy to put it back if you'd rather keep the diff minimal. - No spec coverage for the `mklink`/`dir` fix — `FileUtils` has no existing spec file and Windows CI is disabled (#836). - Arguments containing both whitespace *and* a trailing backslash are still mangled, but that reproduces on bare `Process.spawn`, so it looks unrelated. - Anyone relying on cmd.exe expansion inside a hook `command` would lose it. The API has always been argv-based and `execute_in_background` already raises on `|`, so I don't think it'll be an issue, but the reviewer will probably know better.
1 parent 4ddaec6 commit 55ea3fd

4 files changed

Lines changed: 147 additions & 22 deletions

File tree

‎lib/overcommit/subprocess.rb‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
require 'childprocess'
44
require 'tempfile'
5-
require 'overcommit/os'
65

76
module Overcommit
87
# Manages execution of a child process, collecting the exit status and
@@ -28,8 +27,6 @@ class << self
2827
# @option options [String] input string to pass via standard input stream
2928
# @return [Result]
3029
def spawn(args, options = {})
31-
args = win32_prepare_args(args) if OS.windows?
32-
3330
process = ChildProcess.build(*args)
3431

3532
out, err = assign_output_streams(process)
@@ -58,8 +55,6 @@ def spawn(args, options = {})
5855
# Spawns a new process in the background using the given array of
5956
# arguments (the first element is the command).
6057
def spawn_detached(args)
61-
args = win32_prepare_args(args) if OS.windows?
62-
6358
process = ChildProcess.build(*args)
6459
process.detach = true
6560

@@ -70,20 +65,6 @@ def spawn_detached(args)
7065

7166
private
7267

73-
# Necessary to run commands in the cmd.exe context.
74-
# Args are joined to properly handle quotes and special characters.
75-
def win32_prepare_args(args)
76-
args = args.map do |arg|
77-
# Quote args that contain whitespace
78-
arg = "\"#{arg}\"" if arg =~ /\s/
79-
80-
# Escape cmd.exe metacharacters
81-
arg.gsub(/[()%!^"<>&|]/, '^\0')
82-
end
83-
84-
%w[cmd.exe /c] + [args.join(' ')]
85-
end
86-
8768
# @param process [ChildProcess]
8869
# @return [Array<IO>]
8970
def assign_output_streams(process)

‎lib/overcommit/utils/file_utils.rb‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,13 +49,13 @@ def readlink(link_name)
4949

5050
def win32_dir_cmd(file_name)
5151
Overcommit::Subprocess.spawn(
52-
%W[dir #{win32_fix_pathsep(file_name)}]
52+
%W[cmd.exe /c dir #{win32_fix_pathsep(file_name)}]
5353
)
5454
end
5555

5656
def win32_mklink_cmd(old_name, new_name)
5757
Overcommit::Subprocess.spawn(
58-
%W[mklink #{win32_fix_pathsep(new_name)} #{win32_fix_pathsep(old_name)}]
58+
%W[cmd.exe /c mklink #{win32_fix_pathsep(new_name)} #{win32_fix_pathsep(old_name)}]
5959
)
6060
end
6161

‎overcommit.gemspec‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ Gem::Specification.new do |s|
3131

3232
s.required_ruby_version = '>= 2.6'
3333

34-
s.add_dependency 'childprocess', '>= 0.6.3', '< 6'
34+
s.add_dependency 'childprocess', '>= 5.0.0', '< 6'
3535
s.add_dependency 'iniparse', '~> 1.4'
3636
s.add_dependency 'rexml', '>= 3.4.2'
3737
end

‎spec/overcommit/subprocess_spec.rb‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
# frozen_string_literal: true
2+
3+
require 'spec_helper'
4+
5+
describe Overcommit::Subprocess do
6+
# Absolute path to the Ruby currently running the specs. Using this instead of
7+
# a bare `ruby` keeps these specs working regardless of what is on the PATH.
8+
let(:ruby) { Gem.ruby }
9+
10+
# Script which writes its own ARGV to standard output, using NUL as the
11+
# separator so that arguments containing whitespace remain distinguishable.
12+
let(:argv_dump) { 'STDOUT.print ARGV.join("\0")' }
13+
14+
# Splits the output of `argv_dump` back into the array the child received.
15+
def received_argv(result)
16+
result.stdout.split("\0", -1)
17+
end
18+
19+
describe '.spawn' do
20+
context 'when the command succeeds' do
21+
subject do
22+
described_class.spawn([ruby, '-e', 'STDOUT.print "hello"; STDERR.print "world"'])
23+
end
24+
25+
it 'returns a successful result containing the captured output' do
26+
subject.should be_a described_class::Result
27+
subject.should be_success
28+
subject.status.should == 0
29+
subject.stdout.should == 'hello'
30+
subject.stderr.should == 'world'
31+
end
32+
end
33+
34+
context 'when the command fails' do
35+
subject do
36+
described_class.spawn([ruby, '-e', 'STDERR.print "boom"; exit 42'])
37+
end
38+
39+
it 'returns an unsuccessful result containing the exit status' do
40+
subject.should_not be_success
41+
subject.status.should == 42
42+
subject.stdout.should == ''
43+
subject.stderr.should == 'boom'
44+
end
45+
end
46+
47+
context 'when given input' do
48+
subject do
49+
described_class.spawn([ruby, '-e', 'STDOUT.print STDIN.read'], input: 'from-stdin')
50+
end
51+
52+
it 'passes the input to the standard input stream of the process' do
53+
subject.should be_success
54+
subject.stdout.chomp.should == 'from-stdin'
55+
end
56+
end
57+
58+
context 'when an argument contains whitespace and a dash-prefixed token' do
59+
# Regression test for https://github.com/sds/overcommit/issues/847, where
60+
# arguments were joined into a single string before being handed to the
61+
# shell, causing the child to re-split them on whitespace. This made
62+
# `git stash save` interpret the tail of the stash message (the UTC offset)
63+
# as a switch of its own, failing with `unknown switch '0'`.
64+
let(:stash_message) do
65+
'Overcommit: Stash of repo state before hook run at 2024-04-10 12:34:56 -0700'
66+
end
67+
68+
subject do
69+
described_class.spawn([ruby, '-e', argv_dump, 'save', stash_message])
70+
end
71+
72+
it 'delivers the argument to the child as a single atomic argument' do
73+
subject.should be_success
74+
received_argv(subject).should == ['save', stash_message]
75+
end
76+
end
77+
78+
context 'when arguments contain shell metacharacters' do
79+
let(:tricky_args) do
80+
[
81+
'a&b',
82+
'a|b',
83+
'a>b',
84+
'a<b',
85+
'(parens)',
86+
'caret^escape',
87+
'say "hi" now',
88+
"it's quoted",
89+
'literal%PATH%'
90+
]
91+
end
92+
93+
subject { described_class.spawn([ruby, '-e', argv_dump, *tricky_args]) }
94+
95+
it 'delivers the arguments to the child untouched' do
96+
subject.should be_success
97+
received_argv(subject).should == tricky_args
98+
end
99+
end
100+
101+
context 'when an argument is empty' do
102+
subject { described_class.spawn([ruby, '-e', argv_dump, '', 'after']) }
103+
104+
it 'preserves the empty argument' do
105+
subject.should be_success
106+
received_argv(subject).should == ['', 'after']
107+
end
108+
end
109+
end
110+
111+
# Subprocess no longer branches on platform, but this guards against
112+
# reintroducing a shell wrapper -- if one came back, this would catch it
113+
# from CI (which only runs Linux -- see
114+
# https://github.com/sds/overcommit/issues/836).
115+
describe 'the argument vector handed to ChildProcess' do
116+
let(:args) do
117+
['git', 'stash', 'save', '--quiet', 'Overcommit: Stash at 2024-04-10 12:34:56 -0700']
118+
end
119+
120+
let(:io) { double('io', :stdout= => nil, :stderr= => nil) }
121+
122+
let(:process) do
123+
double(
124+
'process',
125+
io: io,
126+
:duplex= => nil,
127+
:detach= => nil,
128+
start: nil,
129+
wait: nil,
130+
exit_code: 0
131+
)
132+
end
133+
134+
it 'passes the arguments through verbatim from .spawn' do
135+
ChildProcess.should_receive(:build).with(*args).and_return(process)
136+
described_class.spawn(args)
137+
end
138+
139+
it 'passes the arguments through verbatim from .spawn_detached' do
140+
ChildProcess.should_receive(:build).with(*args).and_return(process)
141+
described_class.spawn_detached(args)
142+
end
143+
end
144+
end

0 commit comments

Comments
 (0)