Binary PATH execution regression on Windows (since 3.0.15)

Hi,

Upgrading plexus-utils from 3.0.15 to last version in some application using Commandline occurs problems on Windows for executed binaries when on Windows PATH.

With v3.0.24, this snippet (not the best stream management, this is just a sample):

String bin = "mvn";
String arg = "-version";

Commandline cmd = new Commandline();
cmd.setExecutable( bin );
cmd.createArg().setValue( arg );
Process p = cmd.execute();
p.waitFor();
System.out.println( "Out: " + IOUtil.toString( p.getInputStream() ) );
System.out.println( "Err: " + IOUtil.toString( p.getErrorStream() ) );

Fails with (tested with java 1.7.0_51, 1.8.0_31, 1.8.0_91):

Exception in thread "main" org.codehaus.plexus.util.cli.CommandLineException: Error while executing process.
	at org.codehaus.plexus.util.cli.Commandline.execute(Commandline.java:675)
	at Main.main(Main.java:21)
Caused by: java.io.IOException: Cannot run program "mvn": CreateProcess error=2, Le fichier spécifié est introuvable
	at java.lang.ProcessBuilder.start(ProcessBuilder.java:1041)
	at java.lang.Runtime.exec(Runtime.java:617)
	at org.codehaus.plexus.util.cli.Commandline.execute(Commandline.java:655)
	... 1 more
Caused by: java.io.IOException: CreateProcess error=2, Le fichier spécifié est introuvable
	at java.lang.ProcessImpl.create(Native Method)
	at java.lang.ProcessImpl.<init>(ProcessImpl.java:385)
	at java.lang.ProcessImpl.start(ProcessImpl.java:136)
	at java.lang.ProcessBuilder.start(ProcessBuilder.java:1022)
	... 3 more

Same with cmd embedded command:

String bin = "echo";
String arg = "foo";

NB: These snippets worked fine in v3.0.15 (or in pure cmd command-line).

Is there some upgrades to do for this code continuing to work ? (Environment items to fill, ...)

I have tried to use cmd.addSystemEnvironment(); (containing PATH), but no change.

I'm not able to establish if it should be considered as a bug or not, because executing all plexus-utils unit tests (on Windows 10) for :

v3.0.15, all works (with UT Fix : Quote Java binary when contain space (on Windows) #16).
v3.0.25-SNAPSHOT: some fails, mainly on CommandlineTest :

Some fails are similar to my PATH binary execution problem:

testExecute(org.codehaus.plexus.util.cli.CommandlineTest)
junit.framework.AssertionFailedError: Error while executing process.

testDollarSignInArgumentPath(org.codehaus.plexus.util.cli.CommandlineTest)
java.lang.Exception: Unable to execute command: Error while executing process.

Some others seems more to be a forgot unit test expected results update about quotes management improvements (perhaps except the last/third):

testGetShellCommandLineBash(org.codehaus.plexus.util.cli.CommandlineTest)
junit.framework.ComparisonFailure: expected:<[\bin\echo] 'hello world'> but was:<['\bin\echo'] 'hello world'>

testGetShellCommandLineNonWindows(org.codehaus.plexus.util.cli.CommandlineTest)
junit.framework.ComparisonFailure: expected:<[\usr\bin a b]> but was:<['\usr\bin' 'a' 'b']>

testGetShellCommandLineBash_WithSingleQuotedArg(org.codehaus.plexus.util.cli.CommandlineTest)
junit.framework.ComparisonFailure: expected:<[\bin\echo 'hello world]'> but was:<['\bin\echo' ''"'"'hello world'"'"']'>

I could work on PR (fix or UT update), but some opinion/feedback/advice would be useful.

Thanks in advance.
Best regards

I can confirm these issues on my machines. Can you create PRs for that?

@michael-o

@michael-o : Thanks for the bump ^^, it was out of my mind. I had a look on problem when I have opened the bug, but the root cause wasn't trivial (for me). I will try to found the time todo that.

@krosenvold

Problem is due to Charles Duffy / @krosenvold improvment (b38a1b3 : Commandline shell injection problems).
IMO, re- add for Windows the cmd /c when executable is a cmd internal command or when the prefix path doesn't exist (=> on PATH) is mandatory ...

Hummm ... after reflexion, if conditional processing or redirection is used, it is perhaps more sustainable to use getShellCommandline in all cases (for Windows).

Issue #17: Binary PATH execution regression on Windows (since 3.0.15)

@michael-o

@michael-o (or a maintainer): Could you please update the milestone to 3.1.0 ? Version has been released today, the changelog should be relevant (this fix/change is major for Windows usage). Thank you.

Done.

@khmarbaise

@khmarbaise: Thanks !

Apparently this breaks Ctrl+C on Windows, which will kill the cmd but not the actual command.
See tbroyer/gwt-maven-plugin#110

@tbroyer

Apparently this breaks Ctrl+C on Windows, which will kill the cmd but not the actual command.
See tbroyer/gwt-maven-plugin#110

@tbroyer : Do you have a simple use case to reproduce the problem ? Because using this simple Main:

import java.io.File;
import org.codehaus.plexus.util.cli.Commandline;

public class Main {

    public static void main(String[] args) throws Exception {

        Commandline cmd = new Commandline();
        cmd.setWorkingDirectory(new File("."));
        cmd.setExecutable("ping");
        cmd.createArg().setValue("-n");
        cmd.createArg().setValue("1000");
        cmd.createArg().setValue("127.0.0.1");
        Process process = cmd.execute();
        System.out.println("Executed");
        while (process.isAlive()) {
            System.out.println("Wait");
            Thread.sleep(5000);
        }
    }
}

And execute it in a cmd (java -cp C:/[maven-repo]/org/codehaus/plexus/plexus-utils/3.1.0/plexus-utils-3.1.0.jar;. Main), the result is the same with v3.10 or v3.0.16 when CTRL+C : The ping command is always ran and attached to cmd.

Same behavior with native Java Runtime:

Runtime runtime = Runtime.getRuntime();
Process process = runtime.exec(new String[] { "ping", "-n", "1000", "127.0.0.1" });

Is the problem not more linked to differences between a Linux Shell and Windows cmd ?

I agree with you that this behavior could be a problem, but is it not more to Java code to manage that, by adding a hook (for sample).

        Process process = cmd.execute();
        Runtime.getRuntime().addShutdownHook(new Thread() {
            public void run() {
                process.destroyForcibly();
            }
        });

Perhaps could be a "feature" of Commandline (e.g: if Windows, add this shutdown hook by default) ... but in all case addressed in a new issue and validated by a maintainer.

Do you have a simple use case to reproduce the problem ?

Best I can provide is:

> mvn archetype:generate ^
   -DarchetypeGroupId=net.ltgt.gwt.archetypes ^
   -DarchetypeArtifactId=modular-webapp ^
   -DarchetypeVersion=2018.5.2
[…]
> mvn gwt:codeserver -pl *-client -am

I agree with you that this behavior could be a problem, but is it not more to Java code to manage that, by adding a hook (for sample).

There is a shutdown hook already:

plexus-utils/src/main/java/org/codehaus/plexus/util/cli/CommandLineUtils.java

Lines 136 to 154 in ac81ee2

    
           final Process p = cl.execute(); 
        
           final Thread processHook = new Thread() 
        
           { 
        
               { 
        
                   this.setName( "CommandLineUtils process shutdown hook" ); 
        
                   this.setContextClassLoader( null ); 
        
               } 
        
               @Override 
        
               public void run() 
        
               { 
        
                   p.destroy(); 
        
               } 
        
           }; 
        
           ShutdownHookUtils.addShutDownHook( processHook );

@tbroyer

@tbroyer : The root cause is the usage of cmd.exe /X /C for Windows Shell. The problem could be reproduced with this simple snippet (in IDE or native command line) :

        final String[] cmdNative = new String[] { "ping", "-n", "1000", "127.0.0.1" }; // --> Will be killed
        final String[] cmdSlashC = new String[] { "cmd.exe", "/X", "/C", "ping", "-n", "1000", "127.0.0.1" }; // Will not be killed

        Runtime runtime = Runtime.getRuntime();
        Process process = runtime.exec(cmdSlashC);
        Thread.sleep(5000);
        process.destroy();

But I have not (now) found any solution to destroy/kill the sub-process started by cmd.
Please open a new issue for this problem, this one is closed and not really same perimeter (although linked).

@khmarbaise

I personally do not understand why we have to launch a subshell at all. Why can't we simply pass the command as-is? @khmarbaise

@michael-o

I personally do not understand why we have to launch a subshell at all. Why can't we simply pass the command as-is?

@michael-o : Without that, on Windows, any binaries on PATH or any embedded commands (like echo) doesn't work 😢 (More details in beginning of this issue).

I don't see the bug, if you want some builtin command of an executable, you have to run that executable. Executables work, cmd scripts work for me:

public static void main(String[] args) throws IOException {

	ProcessBuilder builder = new ProcessBuilder("svn", "--version");
	builder.redirectOutput(Redirect.INHERIT);
	builder.start();

}

output:

svn, version 1.9.7 (r1800392)
   compiled Aug  8 2017, 22:14:48 on x86-microsoft-windows

Copyright (C) 2017 The Apache Software Foundation.
This software consists of contributions made by many people;
see the NOTICE file for more information.
Subversion is open source software, see http://subversion.apache.org/

The following repository access (RA) modules are available:

* ra_svn : Module for accessing a repository using the svn network protocol.
  - with Cyrus SASL authentication
  - handles 'svn' scheme
* ra_local : Module for accessing a repository on local disk.
  - handles 'file' scheme
* ra_serf : Module for accessing a repository via WebDAV protocol using serf.
  - using serf 1.3.9 (compiled with 1.3.9)
  - handles 'http' scheme
  - handles 'https' scheme

The following authentication credential caches are available:

* Wincrypt cache in C:\Users\mosipov\AppData\Roaming\Subversion

another one:

public static void main(String[] args) throws IOException {

	ProcessBuilder builder = new ProcessBuilder("mvn.cmd", "--version");
	builder.redirectOutput(Redirect.INHERIT);
	builder.start();

}

output:

Apache Maven 3.5.3 (3383c37e1f9e9b3bc3df5050c29c8aff9f295297; 2018-02-24T20:49:05+01:00)
Maven home: D:\Entwicklung\Programme\apache-maven-3.5.3\bin\..
Java version: 1.8.0_172, vendor: Oracle Corporation
Java home: C:\Program Files\Java\jdk1.8.0_172\jre
Default locale: de_DE, platform encoding: Cp1252
OS name: "windows 10", version: "10.0", arch: "amd64", family: "windows"

@michael-o

@michael-o : The problem is mainly with Windows shell interpretations ... if you open a shell on Linux or Windows (cmd), the commands echo foobar or mvn -version will work.

These interpretation are not native on Windows when invoking:

ProcessBuilder builder = new ProcessBuilder("mvn", "--version");
builder.redirectOutput(Redirect.INHERIT);
builder.start();
--> Will fail, no '.bat' or '.cmd'

CommandLineUtils is here to be cross-platform ... so if we should manage OS specific usage (extensions, ...) the interest is less.
(On another side I'm agree that the not subprocess killed when CTRL+C is used is a problem ... )

echo is a shell builtin. Don't rely on. The missing suffix is purely a Windows problem. I see no issue passing mvn.cmd. Yes, it is cumbersome, but Microsoft ist to blame.

So this commit can be purely removed ... or we can to deal with these corners cases, with:

When binary can't be found (ex: mvn, IOException with message contains CreateProcess error=2), trying to find it on PATH with where command and retry the command with complete binary path if found
For cmd builtins (hardcoded list), use cmd /X /C as prefix

I agree it is not a really beautiful solution, but I did not see other solution to be cross-platform as much as it can.

I wouldn't handle both at all:

This is purely a client issue to pass correct parameters for his env. It is not our task to search for.
Again, this is a client issue. If the client wants echo let him pass cmd.exe, /X, /C on his own.

All we do is simply to abstract. As soon we leave Java with Process we cannot be platform-neutral.

OK. So commit a49305fb52674cfec3a180cb5fae11baea1423e3 should be revert.

Yes, and in my opinion it needs a redesign. At the end, it is not our task to invoke cmd or sh, it Java one's.

Quick proposal for this revert (keeping unit tests): #41

Navigation Menu

Repository navigation

Binary PATH execution regression on Windows (since 3.0.15) #17

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	final Process p = cl.execute();

	final Thread processHook = new Thread()
	{

	{
	this.setName( "CommandLineUtils process shutdown hook" );
	this.setContextClassLoader( null );
	}

	@Override
	public void run()
	{
	p.destroy();
	}

	};

	ShutdownHookUtils.addShutDownHook( processHook );

Binary PATH execution regression on Windows (since 3.0.15) #17

Description

Activity

michael-o commented on Feb 6, 2017

axel3rd commented on Feb 6, 2017

axel3rd commented on Feb 7, 2017

axel3rd commented on Feb 7, 2017

axel3rd commented on Aug 3, 2017

khmarbaise commented on Aug 4, 2017

axel3rd commented on Aug 4, 2017

tbroyer commented on May 18, 2018

axel3rd commented on May 19, 2018

tbroyer commented on May 20, 2018

axel3rd commented on May 21, 2018

michael-o commented on May 21, 2018

axel3rd commented on May 21, 2018

michael-o commented on May 21, 2018

axel3rd commented on May 21, 2018

michael-o commented on May 21, 2018

axel3rd commented on May 21, 2018

michael-o commented on May 22, 2018

axel3rd commented on May 22, 2018

michael-o commented on May 22, 2018

axel3rd commented on May 22, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions