Follow Windows argument quoting rules

[nmisch]

Fixes https://github.com/toddr/IPC-Run/issues/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

[wchristian]

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

[nmisch]

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?

[wchristian]

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

[nmisch]

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.

[mohawk2]

I will be happy to accept this once @wchristian is also happy. @nmisch Please negotiate a mutually-acceptable solution to this very-real problem :-)

[wchristian]

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.

[mohawk2]

I feel that adding a Windows-specific dependency would be better than reproducing code from elsewhere.

[mohawk2]

By the way, @nmisch please also rebase this past current master which contains a fix for the CI problem that caused this PR to fail.

[wchristian]

oh god, yeah, this is windows-only, and there already is code for windows-specific stuff :D

[nmisch]

I've changed this as @wchristian requested. I've also expanded the test coverage to more bytes.

[mohawk2]

Looks good to me. @wchristian ?

[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?

[nmisch]

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.

[wchristian]

Fair enough, then i see nothing stopping this. Thanks a bunch! :D

[mohawk2]

Thanks, both!

[iynehz]

@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,

>type foo.bat
echo %1

Perl script. The command line generated by the new way of Win32::ShellQuote::quote_native() results a not recognized as internal or external command error. The old way is good.

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

[wchristian]

It appears Win32::Process::Create is doing something really weird, because:

d:\taiphoon>perl merp.pl
cmd_line1 = .\foo.bat hello
cmd_line2 = ".\foo.bat" "hello"

'.\foo.bat" "hello' is not recognized as an internal or external command,
operable program or batch file.

Note the missing quotes at the start and end.

[wchristian]

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

To run a batch file, you must start the command interpreter; set lpApplicationName to cmd.exe and set lpCommandLine to the following arguments: /c plus the name of the batch file.

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.

[nmisch]

I know of two ways to fix this:

1. Go back to omitting quotes for arguments not requiring them.
2. When the program name matches /\.(cmd|bat)$/i, follow the special rules for batch files.

Does anyone have a strong preference?

[wchristian]

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.

[nmisch]

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.

[wchristian]

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. On Windows IPC::Run handles executable files 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.

[mohawk2]

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.

[wchristian]

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:

https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa

It touches only windows and is constrained a module relating only to windows.

[wchristian]

Specifically:

1. check the tail of @$cmd for .bat and if so:
2. set lpApplicationName to cmd.exe
3. set lpCommandLine to the following arguments: /c plus the name of the batch file. as per haarg:
   • "path with spaces.bat" other options "with space" blah becomes cmd.exe /c ""path with spaces.bat" other options "with space" blah" sprintf 'cmd.exe /c ""%s"%s"', $cmd[0], @cmd > 1 ? ' '.quote_cmd(@cmd[1 .. $#cmd]) : ''

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.

[toddr]

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.

[iynehz]

@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,

1. You know Python subprocess's functions has a boolean "shell" parameter, when it's True they prepend cmd /c to the command line. But if you try with or without shell=True, a batch file with or without space in its name always works. And that's why I think we should leave whether to prepend cmd /c to the user, rather than decide it ourselves in IPC::Run.

2. I see they use a function called "list2cmdline" to get the command line string and that's it. Actually see all relavent code at these places. I haven't really compared this list2cmdline with Win32::ShellQuote, as I've seen the issue can be fixed in Win32::Process, see my 3rd point below.

• the function that calls CreateProcess: https://github.com/python/cpython/blob/d8389e3e50864447a74605d7ede3d14246bc633a/Lib/subprocess.py#L1348
• list2cmdline: https://github.com/python/cpython/blob/d8389e3e50864447a74605d7ede3d14246bc633a/Lib/subprocess.py#L529
• https://github.com/python/cpython/blob/d8389e3e50864447a74605d7ede3d14246bc633a/Lib/subprocess.py#L1434
• here it calls Windows CreateProcess, https://github.com/python/cpython/blob/d8389e3e50864447a74605d7ede3d14246bc633a/Lib/subprocess.py#L1434

3. And, for a batch file with space in its name, like if the command line "foo bar.bat" baz, where the batch is called foo bar.bat, it simply sends that to CreateProcess (I proved this by debug). The same command line string gives a failure with Perl Win32::Process::Create so it's a problem with Win32::Process::Create. I find the difference is that the executable or appname when Python subprocess calls CreateProcess is NULL (None on the Python level should map to NULL on the C level), but in the perl case this parameter is the quoted "foo bar.bat". And as I tried it, hacking Win32::Process to use NULL for appname can get your demo code work. So I think someone needs to get this PR merged https://github.com/jandubois/win32-process/pull/8 and release a new version of Win32::Process, and on that basis we default appname to undef.

[wchristian]

you know a batch file can be called without .bat/.cmd postfix.

I refer you to what i already wrote:

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.

Example code below.

As for point 3. Admirable, but i think even more work than the plan described above.

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"];

    $cmd->[0] =~ s/\.bat$//;

    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() );
}

[nmisch]

Both IPC::Run and Python's subprocess.run([...], shell=False) assume that the called program uses standard command line parsing rules. Programs that don't do so include batch files, Cygwin executables, cmd.exe, msg.exe, and find.exe.

Thanks for researching Python and lpApplicationName==NULL with batch files. I agree jandubois/win32-process#8 is a step in a good direction. I would not expect that to make batch files work as well as standard executables work, because Python doesn't achieve transparent argument passing to batch files:

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 via @wchristian's recipe would be tempting. Since the set of nonstandard program types is unbounded, I am wary of making IPC::Run apply a DWIM strategy here. My preference is now to make two changes:

(a) Stop quoting arguments that don't need it. This restores compatibility with released versions of IPC::Run.

(b) Document that IPC::Run calls programs as though they use standard command line parsing. Document a bug or missing feature: IPC::Run provides no way for the user to specify an exact (lpApplicationName, lpCommandLine) for nonstandard programs.

[wchristian]

If batch files were the only class of nonstandard program, solving the problem via @wchristian's recipe would be tempting. Since the set of nonstandard program types is unbounded

That's a claim. Please accompany claims with evidence, in code form.

[nmisch]

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 something different. The five kinds listed in my last message are more relevant than this.

[wchristian]

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 Win32::Process examples.

[nmisch]

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:

RUNNING: C:\Strawberry32_526\perl\bin\perl.exe perl.exe -e "binmode STDOUT; print @ARGV, qq{\n}" a\\\\\"b
WANT: a\\"b
GOT:  a\\"b

RUNNING: C:\Strawberry32_526\perl\bin\perl.exe perl.exe -e "binmode STDOUT; print @ARGV, qq{\n}" "a\\\\\"b"
WANT: a\\"b
GOT:  a\\"b

RUNNING: C:\Strawberry32_526\perl\bin\perl.exe perl.exe -e "binmode STDOUT; print @ARGV, qq{\n}" e"x"t"r"a"_"quotes
WANT: extra_quotes
GOT:  extra_quotes

RUNNING: c:/windows/system32/find.exe find.exe "za" \KiZF99btin/find_target.txt
WANT: ... xyzabc
GOT:  
---------- \KIZF99BTIN/FIND_TARGET.TXT
xyzabc

RUNNING: c:/windows/system32/find.exe find.exe za \KiZF99btin/find_target.txt
WANT: ... xyzabc
GOT:  FIND: Parameter format not correct

RUNNING: c:/windows/system32/cmd.exe cmd.exe /c echo "a\\\\\"b"
WANT: a\\"b
GOT:  "a\\\\\"b"

RUNNING: c:/cygwin64/bin/printf.exe printf.exe %s\n a\\\\\"b
WANT: a\\"b
GOT:  a\\\b

RUNNING: c:/cygwin64/bin/stat.exe stat.exe a\\\\\"b
WANT: stat: cannot stat 'a\\"b': No such file or directory
GOT:  stat: cannot stat 'a\\\b': No such file or directory

RUNNING: c:/cygwin64/bin/perl.exe perl.exe -e "binmode STDOUT; print @ARGV, qq{\n}" a\\\\\"b
WANT: a\\"b
GOT:  a\\\b

RUNNING: c:/cygwin64/bin/perl.exe perl.exe -e "binmode STDOUT; print @ARGV, qq{\n}" "a\\\\\"b"
WANT: a\\"b
GOT:  a\\"b

[nmisch]

@mohawk2 wrote:

I agree it's a bug, though I think it may just need documenting since any fix would be horrendous.

I wrote:

My preference is now to make two changes:

(a) Stop quoting arguments that don't need it. This restores compatibility with released versions of IPC::Run.

(b) Document that IPC::Run calls programs as though they use standard command line parsing. Document a bug or missing feature: IPC::Run provides no way for the user to specify an exact (lpApplicationName, lpCommandLine) for nonstandard programs.

@mohawk2, I believe my proposal is consistent with what you wrote. Do you agree or disagree?

[wchristian]

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:

1. Decide whether .bat files are supposed to be supported in exactly the same way as .exe files, or unsupported. A) YES: Implement handing them off to CreateProcess in the way described in the official Win32 API documentation. B) NO: Honestly, not sure. Probably reject them. Maybe other facets of implementation as following might even break them. Possibly leave them working \~sometimes\~ if you're feeling cruel. (In the NO case a user should have to pass cmd.exe /c "their bat file.bat" instead of their bat file.bat to IPC::Run.)
2. Implement calling executables such that with or without arguments all supported executables can have spaces both in their path and the executable name.
3. Decide, implement and document whether arguments are quoted: A) not ever and the user must handle their quoting on their own, always, before passing the string to IPC::Run B) by default with the most widely used windows argument parsing library implementation, but the user can give an argument that disables this so they can handle their quoting on their own before passing the string to IPC::Run

[mohawk2]

I am keeping an eye on this but would like to see @nmisch and @wchristian reach a consensus before I form my own opinion :-)

[nmisch]

On Sat, May 29, 2021 at 05:45:22AM -0700, mohawk2 wrote:

I am keeping an eye on this but would like to see @nmisch and @wchristian reach a consensus before I form my own opinion :-)

Consensus may or may not materialize.

On Fri, May 28, 2021 at 03:09:57PM -0700, Christian Walde (Mithaldu) wrote:

3. Decide, implement and document whether arguments are quoted: A) not ever and the user must handle their quoting on their own, always, before passing the string to IPC::Run

Cross-platform Perl programs calling IPC::Run would then need to condition their IPC::Run inputs on the platform. Not needing such conditions is a key benefit of IPC::Run. Hence, let's rule out 3(A).

B) by default with the most widely used windows argument parsing library implementation, but the user can give an argument that disables this so they can handle their quoting on their own before passing the string to IPC::Run

Suppose I implemented the following:

a) If $cmd->[0]=~/\.(bat|cmd) *$/i then construct the command line using batch file rules. If any argument contains a character impossible to pass into a batch file (\n or \r), raise an error.

b) Otherwise, use some amount of quoting sufficient for programs using standard command line parsing rules and for Cygwin programs.

c) For programs where both (a) and (b) are inadequate, let the user control the second and third arguments of Win32::Process::Create() with syntax like this:

   # 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 behavior of IPC::Run releases, I'm skeptical of the value of (a) and (c). If including them ends debate, I'm willing to suspend skepticism.

[wchristian]

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.

[nmisch]

For (1), batch files would be supported. See (a). (2) is already true and would remain true.

[haarg]

I'm working on an update to Win32::ShellQuote so that quote_native does not unnecessarily quote arguments.

[wchristian]

For (1), batch files would be supported. See (a).

It was unclear to me whether your a) means:

Implement handing them off to CreateProcess in the way described in the official Win32 API documentation.

If it does, then yay.

(2) is already true and would remain true.

I have no idea why you say this, we already proved that the previous implementation did not support bat files with spaces.

I'm working on an update to Win32::ShellQuote so that quote_native does not unnecessarily quote arguments.

Even tho it's not a fix to "executable with spaces" here, that is useful. :)

[wchristian]

To move this from philosophizing, i've opened #147 with failing tests to be considered.

[nmisch]

batch file (.BAT or .CMD file) w/ spaces => doesn't work today executable (.EXE file) w/ spaces => works today

[wchristian]

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.