Commons Exec
  1. Commons Exec
  2. EXEC-36

CommandLine does not work with double quote or single quote correctly

    Details

    • Type: Bug Bug
    • Status: In Progress
    • Priority: Critical Critical
    • Resolution: Unresolved
    • Affects Version/s: 1.0
    • Fix Version/s: None
    • Labels:
      None
    • Environment:

      Ubuntu 704, JDK1.6

      Description

      Please review and run the following junit test, either apache ant or apache exec does not handle the qoute in the arguments correctly.

      @Test
      public void shouldHandleTheDoubelQuote() throws Exception

      { String commandline = "./script/jrake cruise:publish_installers " + "INSTALLER_VERSION=unstable_2_1 " + "INSTALLER_PATH=\"/var/lib/ cruise-agent/installers\" " + "INSTALLER_DOWNLOAD_SERVER='something'" + "WITHOUT_HELP_DOC=true"; CommandLine line = CommandLine.parse(commandline); String[] args = line.getArguments(); assertThat(args[0], is("cruise:publish_installers")); assertThat(args[1], is("INSTALLER_VERSION=unstable_2_1")); assertThat(args[2], is("INSTALLER_PATH=\"/var/lib/ cruise-agent/installers\"")); assertThat(args[3], is("INSTALLER_DOWNLOAD_SERVER='something'")); assertThat(args[4], is("WITHOUT_HELP_DOC=true")); }
      1. StringUtils.java
        9 kB
        Alessandro Riva
      2. ExecParseUtilsTest.java
        6 kB
        Siegfried Goeschl
      3. ExecParseUtils.java
        4 kB
        Siegfried Goeschl

        Activity

        Hide
        Siegfried Goeschl added a comment - - edited

        Here is your re-formatted snippet

         
        @Test
        public void shouldHandleTheDoubelQuote() throws Exception 
        { 
          String commandline = "./script/jrake cruise:publish_installers " 
            + "INSTALLER_VERSION=unstable_2_1 " 
            + "INSTALLER_PATH=\"/var/lib/ cruise-agent/installers\" " 
            + "INSTALLER_DOWNLOAD_SERVER='something'" 
            + "WITHOUT_HELP_DOC=true";
            
          CommandLine line = CommandLine.parse(commandline); 
          String[] args = line.getArguments(); 
          assertThat(args[0], is("cruise:publish_installers")); 
          assertThat(args[1], is("INSTALLER_VERSION=unstable_2_1")); 
          assertThat(args[2], is("INSTALLER_PATH=\"/var/lib/ cruise-agent/installers\"")); 
          assertThat(args[3], is("INSTALLER_DOWNLOAD_SERVER='something'")); 
          assertThat(args[4], is("WITHOUT_HELP_DOC=true")); 
        }
        

        Some quick notes

        +) args[0] should be "./script/jrake" and not "cruise:publish_installers"
        +) mixing and matching singe and double quotes using CommandLine.parse() is not recommended (http://commons.apache.org/exec/faq.html)
        +) I will add a test case to see how to generate a valid command line in your case

        Show
        Siegfried Goeschl added a comment - - edited Here is your re-formatted snippet @Test public void shouldHandleTheDoubelQuote() throws Exception { String commandline = "./script/jrake cruise:publish_installers " + "INSTALLER_VERSION=unstable_2_1 " + "INSTALLER_PATH=\"/var/lib/ cruise-agent/installers\" " + "INSTALLER_DOWNLOAD_SERVER='something'" + "WITHOUT_HELP_DOC=true"; CommandLine line = CommandLine.parse(commandline); String[] args = line.getArguments(); assertThat(args[0], is("cruise:publish_installers")); assertThat(args[1], is("INSTALLER_VERSION=unstable_2_1")); assertThat(args[2], is("INSTALLER_PATH=\"/var/lib/ cruise-agent/installers\"")); assertThat(args[3], is("INSTALLER_DOWNLOAD_SERVER='something'")); assertThat(args[4], is("WITHOUT_HELP_DOC=true")); } Some quick notes +) args [0] should be "./script/jrake" and not "cruise:publish_installers" +) mixing and matching singe and double quotes using CommandLine.parse() is not recommended ( http://commons.apache.org/exec/faq.html ) +) I will add a test case to see how to generate a valid command line in your case
        Hide
        Kai Hu added a comment -

        first, sorry for the bad format. thanks for formatting for me

        +) arg[0] should not be "./script/jrake", I think "./script/jrake" should be returned from commandLine.getExecutable() or something like this.
        +) I fully understand it is not recommented, but if this library is targeting for reduce the pain of dealing with command/process, then I think it is something this api need to provide, since I do not think there is any api can handle it correctly and that is part of the core-value of this api(Ant is using exactly same way to parse it, so it has the same bug)if you guys can ship with it, it will be the silver bullet for command/process.

        cheers

        Show
        Kai Hu added a comment - first, sorry for the bad format. thanks for formatting for me +) arg [0] should not be "./script/jrake", I think "./script/jrake" should be returned from commandLine.getExecutable() or something like this. +) I fully understand it is not recommented, but if this library is targeting for reduce the pain of dealing with command/process, then I think it is something this api need to provide, since I do not think there is any api can handle it correctly and that is part of the core-value of this api(Ant is using exactly same way to parse it, so it has the same bug)if you guys can ship with it, it will be the silver bullet for command/process. cheers
        Hide
        Siegfried Goeschl added a comment -

        Hi Kai,

        +) yes you are right regarding the arg[0]
        +) I think there is a space missing after "INSTALLER_DOWNLOAD_SERVER='something'" otherwise there is no args[4]
        +) is there really a space in "INSTALLER_PATH=\"/var/lib/ cruise-agent/installers\"", e.g. thus this define two paths '/var/lib/' and 'cruise-agent/installers'?

        Show
        Siegfried Goeschl added a comment - Hi Kai, +) yes you are right regarding the arg [0] +) I think there is a space missing after "INSTALLER_DOWNLOAD_SERVER='something'" otherwise there is no args [4] +) is there really a space in "INSTALLER_PATH=\"/var/lib/ cruise-agent/installers\"", e.g. thus this define two paths '/var/lib/' and 'cruise-agent/installers'?
        Hide
        Kai Hu added a comment -

        >>> I think there is a space missing after "INSTALLER_DOWNLOAD_SERVER='something'" otherwise there is no args[4]

        you are absolutely right

        >>> is there really a space in "INSTALLER_PATH=\"/var/lib/ cruise-agent/installers\"", e.g. thus this define two paths '/var/lib/' and 'cruise-agent/installers'?
        I just want to design a case that there is a whitespace in the argument, which makes the quotes to be necessary, do you mind to help me to change the test cases to
        "INSTALLER_PATH=\"/var/lib/cruise agent/installers\" and also change the assertion accordingly.

        thanks for you quick response. cheers.

        Show
        Kai Hu added a comment - >>> I think there is a space missing after "INSTALLER_DOWNLOAD_SERVER='something'" otherwise there is no args [4] you are absolutely right >>> is there really a space in "INSTALLER_PATH=\"/var/lib/ cruise-agent/installers\"", e.g. thus this define two paths '/var/lib/' and 'cruise-agent/installers'? I just want to design a case that there is a whitespace in the argument, which makes the quotes to be necessary, do you mind to help me to change the test cases to "INSTALLER_PATH=\"/var/lib/cruise agent/installers\" and also change the assertion accordingly. thanks for you quick response. cheers.
        Hide
        Kai Hu added a comment -

        when will this bug be fixed? is anyone looking at this one?

        Show
        Kai Hu added a comment - when will this bug be fixed? is anyone looking at this one?
        Hide
        Siegfried Goeschl added a comment -

        I'm looking at the problem but I'm not eager to break the current command line processing otherwise I'm unable to get the release out of the door. Having said that you can still build up your command line bit by bit using the API.

        Show
        Siegfried Goeschl added a comment - I'm looking at the problem but I'm not eager to break the current command line processing otherwise I'm unable to get the release out of the door. Having said that you can still build up your command line bit by bit using the API.
        Hide
        Sebb added a comment -

        I think the first thing to do is to document how the command-line parsing is intended to work.
        Should it use the same quoting as the underlying OS, or a common scheme?
        i.e. when is the parse() method going to be used?

        Show
        Sebb added a comment - I think the first thing to do is to document how the command-line parsing is intended to work. Should it use the same quoting as the underlying OS, or a common scheme? i.e. when is the parse() method going to be used?
        Hide
        Mitko Kolev added a comment - - edited

        Hello everyone,
        I am using the commons-exec library in Apache Camel (the command arguments must be parsed from a URI), came up to the same problem, and had to implement the parsing myself. I did the custom command line args parsing based on the following 3 simple rules:

        • The args in the line are space-separated tokens
        • If an argument has a space itself, this space can be quoted with " (double-qoute)
        • If an argument should preserve the double-quotes, it is quoted two times with a double-quote

        Here is a link to to the parser and a link to the unit tests.

        What do you think of this approach? Is it applicable to commons-exec?

        Regards,
        Mitko

        Show
        Mitko Kolev added a comment - - edited Hello everyone, I am using the commons-exec library in Apache Camel (the command arguments must be parsed from a URI), came up to the same problem, and had to implement the parsing myself. I did the custom command line args parsing based on the following 3 simple rules: The args in the line are space-separated tokens If an argument has a space itself, this space can be quoted with " (double-qoute) If an argument should preserve the double-quotes, it is quoted two times with a double-quote Here is a link to to the parser and a link to the unit tests . What do you think of this approach? Is it applicable to commons-exec? Regards, Mitko
        Hide
        Siegfried Goeschl added a comment -

        Hi Mitko,

        let me have a look at it at the end of next week (I have to finish a paper at the end of next week)

        Cheers,

        Siegfried Goeschl

        Show
        Siegfried Goeschl added a comment - Hi Mitko, let me have a look at it at the end of next week (I have to finish a paper at the end of next week) Cheers, Siegfried Goeschl
        Hide
        Siegfried Goeschl added a comment -

        Code based on Mitko's work in Apache Camel

        Show
        Siegfried Goeschl added a comment - Code based on Mitko's work in Apache Camel
        Hide
        Siegfried Goeschl added a comment -

        When using the API to build the command line incrementally (and not parsing the command line string) it is possible to recreate Kai's command line as shown in DefaultExecutorTest#testExec36_1() under Unix.

        Show
        Siegfried Goeschl added a comment - When using the API to build the command line incrementally (and not parsing the command line string) it is possible to recreate Kai's command line as shown in DefaultExecutorTest#testExec36_1() under Unix.
        Hide
        Jakob Steltner added a comment -

        Hello,

        I'm experiencing the same issue using CommandLine.parse() with commons-exec-1.1. The code works fine with Windows, gracefully handling quotes around file names with spaces but breaks when running in GNU/Linux. So this seems to affect only Linux systems.

        Regards,
        Jakob

        Show
        Jakob Steltner added a comment - Hello, I'm experiencing the same issue using CommandLine.parse() with commons-exec-1.1. The code works fine with Windows, gracefully handling quotes around file names with spaces but breaks when running in GNU/Linux. So this seems to affect only Linux systems. Regards, Jakob
        Hide
        Siegfried Goeschl added a comment -

        Hi Jakob - can you provide more details

        +) example of command line
        +) what application are you starting (executable versus shell script)

        Sigi

        Show
        Siegfried Goeschl added a comment - Hi Jakob - can you provide more details +) example of command line +) what application are you starting (executable versus shell script) Sigi
        Hide
        Kedar Raybagkar added a comment -

        StringUtils also needs a change to handle the issue of quote...
        Trip the ending as well as starting double or single quote if they start or end with quotes.

        method static String quoteArgument...

        while((argument.trim().endsWith(SINGLE_QUOTE) || argument.trim().endsWith(DOUBLE_QUOTE))
        && cleanedArgument.startsWith(SINGLE_QUOTE) || cleanedArgument.startsWith(DOUBLE_QUOTE))

        { cleanedArgument = cleanedArgument.substring(1); }

        while((argument.trim().startsWith(SINGLE_QUOTE) || argument.trim().startsWith(DOUBLE_QUOTE))
        && cleanedArgument.endsWith(SINGLE_QUOTE) || cleanedArgument.endsWith(DOUBLE_QUOTE))

        { cleanedArgument = cleanedArgument.substring(0, cleanedArgument.length() - 1); }
        Show
        Kedar Raybagkar added a comment - StringUtils also needs a change to handle the issue of quote... Trip the ending as well as starting double or single quote if they start or end with quotes. method static String quoteArgument... while((argument.trim().endsWith(SINGLE_QUOTE) || argument.trim().endsWith(DOUBLE_QUOTE)) && cleanedArgument.startsWith(SINGLE_QUOTE) || cleanedArgument.startsWith(DOUBLE_QUOTE)) { cleanedArgument = cleanedArgument.substring(1); } while((argument.trim().startsWith(SINGLE_QUOTE) || argument.trim().startsWith(DOUBLE_QUOTE)) && cleanedArgument.endsWith(SINGLE_QUOTE) || cleanedArgument.endsWith(DOUBLE_QUOTE)) { cleanedArgument = cleanedArgument.substring(0, cleanedArgument.length() - 1); }
        Hide
        Kedar Raybagkar added a comment -

        Another thing that I noticed was that Argument inner class trims the value data therefore if a command line has "ABC " then the space is removed. The same problem is also found in StringUtils which to trims the data. After commenting the two trim portions, it works for any string that has a space inside a double quote. All tests were executed successfully.

        Show
        Kedar Raybagkar added a comment - Another thing that I noticed was that Argument inner class trims the value data therefore if a command line has "ABC " then the space is removed. The same problem is also found in StringUtils which to trims the data. After commenting the two trim portions, it works for any string that has a space inside a double quote. All tests were executed successfully.
        Hide
        Hendy Irawan added a comment -

        The string

        web adoh '
        

        is quoted into

        "web adoh"
        

        where it should be

        "web adoh '"
        
        Show
        Hendy Irawan added a comment - The string web adoh ' is quoted into "web adoh" where it should be "web adoh '"
        Hide
        Lars Corneliussen added a comment -

        Hi,

        I'm facing the same problem from StringUtils.quoteArgument() in Apache NPanday (incubator, Maven for .NET)

        _
        Lars

        Show
        Lars Corneliussen added a comment - Hi, I'm facing the same problem from StringUtils.quoteArgument() in Apache NPanday (incubator, Maven for .NET) _ Lars
        Hide
        Alessandro Riva added a comment -

        Proposed fix

        Show
        Alessandro Riva added a comment - Proposed fix

          People

          • Assignee:
            Siegfried Goeschl
            Reporter:
            Kai Hu
          • Votes:
            2 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development