Follow Windows argument quoting rules #143
Conversation
|
I appreciate the effort. However i would ask you please consider take another path that would avoid duplication. https://metacpan.org/pod/Win32::ShellQuote already implements, to the best of my knowledge, full win32 shell quoting rules. Can you please use that instead of a duplicate implementation? :) (Note, i'm nobody special here, just trying to help.) |
|
On Sun, Mar 14, 2021 at 02:31:07PM -0700, Christian Walde (Mithaldu) wrote:
I appreciate the effort. However i would ask you please consider take another path that would avoid duplication.
https://metacpan.org/pod/Win32::ShellQuote already implements, to the best of my knowledge, full win32 shell quoting rules.
- That would save only a single-digit number of duplicate code lines.
- IPC::Run currently has zero dependencies outside core Perl. Users value that.
- Once correct, this code will stay frozen. (Microsoft is extreme about
backward compatibility.)
Hence, I'm choosing to duplicate. I'm happy to modify one of the comments to
mention that this code should be interchangeable with a certain function in
Win32::ShellQuote. Is that reasonable?
|
|
in that case it might be best to include a direct copy of that module with the package line broken or something like that i must admit i did not expect any weird considerations and only thought about correctness and duplication e: and it also already relies on something outside of core: IO::Pty |
|
On Sun, Mar 14, 2021 at 06:55:03PM -0700, Christian Walde (Mithaldu) wrote:
in that case it might be best to include a direct copy of that module with the package line broken or something like that
I don't think so. That would add 250+ lines, which is a lot, relatively.
|
|
I will be happy to accept this once @wchristian is also happy. @nmisch Please negotiate a mutually-acceptable solution to this very-real problem :-) |
|
The problem is simple: Win32::ShellQuote is currently the implementation of windows shell quoting rules that is most known to be reliable. If this module needs to do windows shell quoting, it should use the most reliable logic. That means either the proposed code needs to prove it's equivalent to Win32::ShellQuote or W::SQ should be used. (If it's better, it should be used to propose patches to it.) Personally i don't understand why the completely arbitrary metric of dependencies is invoked here, given it already pulls in a CPAN dependency for linux, 3 for windows, and 1 for older perls, and nowhere in its docs lays out a philosophy or a promise of few dependencies. At the same time i don't understand why the line count matters. Afaict this module isn't size-limited and maintenance is not a concern as maintaining that shell quoting code amounts to "copypaste whatever is most recent in Win32::ShellQuote". That said that's just my opinion and i'm not the master of the house here. |
|
I feel that adding a Windows-specific dependency would be better than reproducing code from elsewhere. |
|
By the way, @nmisch please also rebase this past current |
|
oh god, yeah, this is windows-only, and there already is code for windows-specific stuff :D |
|
I've changed this as @wchristian requested. I've also expanded the test |
|
Looks good to me. @wchristian ? |
|
That looks pretty good, thanks very much. :) Two questions: Does it make sense to keep the TODO entries? The tests assign to $r, but never do anything with it, i think that can be removed? |
|
On Mon, Mar 22, 2021 at 02:24:13AM -0700, Christian Walde (Mithaldu) wrote:
Does it make sense to keep the TODO entries?
Yes. win32_parse_cmd_line() operates on each harness specification that
consists of a single string, which IPC::Run specifies as "a single string to be
passed to the systems' shell". So long as win32_parse_cmd_line() is parsing
strings in service of that specification, these TODOs are applicable. (However,
if I were changing that area, I'd probably just alter the specification to match
today's behavior. If I were starting from scratch, I'd choose neither the
current specification nor the current behavior.)
The tests assign to $r, but never do anything with it, i think that can be removed?
Removing it wouldn't be wrong. I did not remove it. run.t has other dead
stores to $r.
|
|
Fair enough, then i see nothing stopping this. Thanks a bunch! :D |
|
Thanks, both! |
|
@nmisch, @mohawk2 Sorry but I find this change seems to break calls to batch file. For example, a simple batch file can be like this, Perl script. The command line generated by the new way of use 5.012;
use warnings;
use Win32::ShellQuote;
use Win32::Process;
use Carp;
my $cmd = [".\\foo.bat", "hello"];
my $cmd_line1 = join( ' ', @$cmd ); # old way
my $cmd_line2 = Win32::ShellQuote::quote_native(@$cmd); # new way
say "cmd_line1 = $cmd_line1";
say "cmd_line2 = $cmd_line2";
sub spawn {
my $cmd_line = shift;
my $process;
Win32::Process::Create( # this is like how IPC::Run::Win32Helper creates process
$process,
$cmd->[0],
$cmd_line,
1, ## Inherit handles
0, ## Inherit parent priortiy class. Was NORMAL_PRIORITY_CLASS
".",
) or croak "$!: Win32::Process::Create()";
}
spawn($cmd_line2); # not recognized as internal or external command blah
#spawn($cmd_line1); # this is good |
|
It appears Win32::Process::Create is doing something really weird, because: Note the missing quotes at the start and end. |
|
The issues go deeper. As the script below shows, having a space in the batch filename causes both to break. I believe this is due to special handling required for batch files as described in: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa
use 5.012;
use warnings;
use Win32::ShellQuote;
use Win32::Process;
use Carp;
use Win32;
use IO::All -binary;
use Time::HiRes 'sleep';
run();
sub run {
run_bat("foo.bat");
run_bat("fo o.bat");
}
sub run_bat {
my ($executable) = @_;
io($executable)->print("\@ECHO OFF\r\necho %1\r\n");
die "failed to create batch file" if not -e $executable;
my $cmd = [".\\$executable", "hello"];
spawn($cmd, join ' ', @$cmd); # old way
spawn($cmd, Win32::ShellQuote::quote_native(@$cmd)); # modern way
}
sub spawn {
my ( $cmd, $cmd_line ) = @_;
my $process;
say "\n--- running ---\ncmd_line = $cmd_line";
Win32::Process::Create( # this is like how IPC::Run::Win32Helper creates process
$process,
$cmd->[0],
$cmd_line,
1, ## Inherit handles
0, ## Inherit parent priortiy class. Was NORMAL_PRIORITY_CLASS
".",
) || die ErrorReport();
sleep 0.2;
}
sub ErrorReport{
print Win32::FormatMessage( Win32::GetLastError() );
}
__END__
--- running ---
cmd_line = .\foo.bat hello
hello
--- running ---
cmd_line = ".\foo.bat" "hello"
'.\foo.bat" "hello' is not recognized as an internal or external command,
operable program or batch file.
--- running ---
cmd_line = .\fo o.bat hello
'.\fo' is not recognized as an internal or external command,
operable program or batch file.
--- running ---
cmd_line = ".\fo o.bat" "hello"
'.\fo' is not recognized as an internal or external command,
operable program or batch file. |
|
I know of two ways to fix this:
Does anyone have a strong preference? |
This would only be useful as a temporary interim measure because that implementation is still broken when asked to handle spaces. |
|
On Wed, Apr 28, 2021 at 12:32:00AM -0700, Christian Walde (Mithaldu) wrote:
> Go back to omitting quotes for arguments not requiring them.
This would only be useful as a temporary interim measure because that implementation is still broken when asked to handle spaces.
I disagree. IPC::Run has never handled batch files with spaces, and it's not a
given that it should. One may argue that the caller should provide the
"cmd.exe","/c" rather than IPC::Run injecting it magically. That's not to say I
prefer this option, but it is a sound option that can serve indefinitely.
|
|
I'm only here to advise on windows issues. @mohawk2 has the last call. That said, to my knowledge: On other systems IPC::Run handles executables with space in path. We now discovered that it fails on batch files with space in path on windows. Batch files are a type of executable. As such we found an inconsistency with other previous behavior. That makes it a bug. A new one, but a bug. |
|
I agree it's a bug, though I think it may just need documenting since any fix would be horrendous. I am not yet in a position to release anything since @toddr has not yet seen fit to make that possible. |
|
What do you mean horrendous? The fix here is to, in IPC::Run::Win32Helper::win32_spawn, before handing it off to Win32::Process::Create, to follow the logic laid out in: It touches only windows and is constrained a module relating only to windows. |
|
Specifically:
Addendum: Typically there is some capability of leaving the executable extension optional, but with the way Win32::Process::Create is used in IPC::Run that capability is not exposed to the user even for .exe executables, so that's not required to work. |
|
You only need to ask. I’m happy to release. I’m happy to give pause bits.
Either way, I need a nudge.
…On Sat, May 1, 2021 at 8:22 AM mohawk2 ***@***.***> wrote:
I agree it's a bug, though I think it may just need documenting since any
fix would be horrendous. I am not yet in a position to release anything
since @toddr <https://github.com/toddr> has not yet seen fit to make that
possible.
|
|
@wchristian I don't think it a good idea to check for a .bat or else: you know a batch file can be called without .bat/.cmd postfix. I just looked at Python subprocess implementation. I have some findings here,
|
I refer you to what i already wrote:
Example code below. As for point 3. Admirable, but i think even more work than the plan described above. |
|
Both IPC::Run and Python's Thanks for researching Python and import os
import subprocess
import tempfile
bat = tempfile.NamedTemporaryFile(suffix=b'.bat', delete=False)
bat.write(b"echo %1")
bat.close()
find_target = tempfile.NamedTemporaryFile(suffix=b'.txt', delete=False)
find_target.write(br'a\\"b')
find_target.close()
# Batch file prints excess backslashes.
subprocess.call([bat.name, r'a"b'], shell=False)
subprocess.call([bat.name, r'a\"b'], shell=False)
subprocess.call([bat.name, r'a\\"b'], shell=False)
# '\"findstr\"' is not recognized as an internal or external command,
subprocess.call(["cmd.exe", "/c", b'"findstr" a ' + find_target.name], shell=False)
# standard-behavior exe works
subprocess.call(['nslookup.exe', r'-class=a\\"b', 'example.com'], shell=False)
os.unlink(bat.name)
os.unlink(find_target.name)If batch files were the only class of nonstandard program, solving the problem (a) Stop quoting arguments that don't need it. This restores compatibility (b) Document that IPC::Run calls programs as though they use standard command |
That's a claim. Please accompany claims with evidence, in code form. |
|
Code showing the absence of a bound? Okay: # The output is a batch script that fills the current working directory with
# copies of find.exe, a program that use nonstandard command line parsing rules.
# This produces over 26^200 usable names; translating from bound=26^200 to
# truly-unbounded is left as an exercise for the reader.
print "copy c:\\windows\\system32\\find.exe find_$_.exe\n"
for 'a' .. ('z' x 200);I wasn't sure I parsed your request correctly, so let me know if you meant |
|
I thought you were aiming for "Win32::Process can run programs with extensions other than .bat and .exe" or something like that. Your answer appears even more strange and i have no idea what you're trying to say. Please talk in terms of |
use Win32::Process;
use Win32::ShellQuote;
use strict;
use warnings;
use File::Temp qw(tempdir);
my $dir = tempdir(CLEANUP => 1);
my $payload = "a\\\\\"b";
my $for_cmdline = 'a\\\\\\\\\\"b';
my $for_cmdline_dquoted = "\"$for_cmdline\"";
die "$for_cmdline_dquoted ne " . Win32::ShellQuote::quote_native($payload)
unless $for_cmdline_dquoted eq Win32::ShellQuote::quote_native($payload);
# Run one command. $want is the output the program would have if the program
# used standard command line parsing rules.
sub try {
my($exe, $cmdline, $want) = @_;
print "RUNNING: $exe $cmdline\nWANT: ${want}GOT: ";
STDOUT->flush;
Win32::Process::Create(my $p, $exe, $cmdline, 1, 0, ".")
or die "$!: Win32::Process::Create()";
$p->Wait(INFINITE) || die;
print "\n";
}
# Perl uses standard command line parsing rules. Both $for_cmdline and
# $for_cmdline_dquoted suffice for conveying the payload.
try($^X, 'perl.exe -e "binmode STDOUT; print @ARGV, qq{\n}" ' . $for_cmdline,
"$payload\n");
try($^X, 'perl.exe -e "binmode STDOUT; print @ARGV, qq{\n}" ' . $for_cmdline_dquoted,
"$payload\n");
try($^X, 'perl.exe -e "binmode STDOUT; print @ARGV, qq{\n}" e"x"t"r"a"_"quotes',
"extra_quotes\n");
# find.exe wants its first argument quoted, no matter how trivial. A program
# using standard command line parsing rules would behave the same with or wthout
# the double quotes. Hence, find.exe is nonstandard.
my $find_target = "$dir/find_target.txt";
open(my $fh, '>', $find_target) || die "open";
print $fh "xyzabc\n";
close $fh;
try('c:/windows/system32/find.exe', "find.exe \"za\" $find_target",
"... xyzabc\n");
try('c:/windows/system32/find.exe', "find.exe za $find_target",
"... xyzabc\n");
# cmd.exe does not strip the quotes and backslashes that a standard program
# would strip.
try('c:/windows/system32/cmd.exe', 'cmd.exe /c echo ' . $for_cmdline_dquoted,
"$payload\n");
# msg.exe strips quotes, but it does not strip the backslashes that a standard
# program would strip. Disabled since this creates a popup window.
if (0) {
try('c:/windows/system32/msg.exe', 'msg.exe * ' . $for_cmdline_dquoted,
"[popup window should show] $payload\n");
}
# Cygwin programs handle $for_cmdline differently than a standard program would.
# This includes Cygwin Perl.
try('c:/cygwin64/bin/printf.exe', "printf.exe %s\\n $for_cmdline",
"$payload\n");
try('c:/cygwin64/bin/stat.exe', "stat.exe $for_cmdline",
"stat: cannot stat '$payload': No such file or directory\n");
try('c:/cygwin64/bin/perl.exe',
'perl.exe -e "binmode STDOUT; print @ARGV, qq{\n}" ' . $for_cmdline,
"$payload\n");
# (Cygwin programs do handle $for_cmdline_dquoted in the standard way.)
try('c:/cygwin64/bin/perl.exe',
'perl.exe -e "binmode STDOUT; print @ARGV, qq{\n}" ' . $for_cmdline_dquoted,
"$payload\n");Output: |
|
@mohawk2 wrote:
I wrote:
@mohawk2, I believe my proposal is consistent with what you wrote. Do you |
|
Sorry, i got distracted by other things. And thanks for your example, it brought up something entirely else that i didn't even have on the radar. This is the fact that windows programs don't get a list of arguments, but get their arguments as one single string, which they then run through whatever parser they prefer, with most using a specific windows arg parsing library that Win32::ShellQuote is implemented for. As such, what you're aiming for is a temporary measure to alleviate a regression, but not a proper fix. Here's how it needs to work to be correct on windows:
|
|
I am keeping an eye on this but would like to see @nmisch and @wchristian reach a consensus before I form my own opinion :-) |
|
On Sat, May 29, 2021 at 05:45:22AM -0700, mohawk2 wrote:
Consensus may or may not materialize. On Fri, May 28, 2021 at 03:09:57PM -0700, Christian Walde (Mithaldu) wrote:
Cross-platform Perl programs calling IPC::Run would then need to condition
Suppose I implemented the following: a) If b) Otherwise, use some amount of quoting sufficient for programs using c) For programs where both (a) and (b) are inadequate, let the user control # instead of IPC::Run::run(['cmd.exe', '/c', 'echo', '""']) ...
IPC::Run::run(IPC::Run::Win32Process->new(
'c:/windows/system32/cmd.exe', qq{cmd.exe /c echo ""}));Would that meet your needs, or not? Given the lack of bug reports about the |
|
That doesn't answer 1 or confirm 2 above, but for 3 that seems good. When documenting i'd recommend actually using an example where the default behaviors won't work and manual construction is mandatory. Also, just as a note: For a) Win32::ShellQuote will be fine (and do the newline thing), and i do suspect for b) you'll end up finding it to be fine too, assuming the answer to 1 was yes, but i might end up wrong. |
|
For (1), batch files would be supported. See (a). |
|
I'm working on an update to Win32::ShellQuote so that quote_native does not unnecessarily quote arguments. |
It was unclear to me whether your a) means:
If it does, then yay.
I have no idea why you say this, we already proved that the previous implementation did not support bat files with spaces.
Even tho it's not a fix to "executable with spaces" here, that is useful. :) |
|
To move this from philosophizing, i've opened #147 with failing tests to be considered. |
|
batch file (.BAT or .CMD file) w/ spaces => doesn't work today |
|
Right, so it is in fact not already true under a combined interpretation of my three points wherein as per 1 a) bat files are to be considered equivalent to exe executables. In any case, see #147. Unless there can be a clear explanation for why any of the given test cases therein shouldn't work under Windows, a correct implementation should have all of them working. I'd hope all future comments be based on said code. |
6 participants
Fixes #142
In the interest of full disclosure, this contains a few lines of code that I
also pushed to PostgreSQL. That push did not involve a copyright assignment,
so the copyright status of this pull request is the same it would be if I had
not done that push. Here is the corresponding PostgreSQL commit:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=fcd15f13581f6d75c63d213220d5a94889206c1b#patch20