[Bug] GenericContainer doesn’t properly split commands

[dhoard]

Module

Core

Testcontainers version

1.17.3

Using the latest Testcontainers version?

Yes

Host OS

ALL

Host Arch

ALL

Docker version

N/A

What happened?

GenericContainer setCommand uses Java String split to build the array of command parts. If the command contains quoted arguments/quoted arguments with escaped quotes, the container fails.

Example command:

/bin/sh test.sh “argument 1” “argument 2”

Relevant log output

No response

Additional Information

No response

[hariohmprasath]

Hi @dhoard, Container.java also provides another version of setCommand method that allows sending in an array directly

Method signature:

void setCommand(@NonNull String... commandParts);

Example usage:

setCommand("--display=true", "-driver=\"kernel-4.0 kernel-4.1\"");

Will this work?

[dhoard]

@hariohmprasath sure, that method is a workaround if the parts are already split.

In my scenario, I am reading a complex/quoted command line from a file. Ideally, I should be able to pass the whole command line to setCommand(String command) as is.

The code of setCommand(String command) is coded with the intent of passing a single command and splitting it internally.

[aidando73]

Hey @dhoard. Thanks for reporting this. This is a valid use case and it's no surprise that users would expect that setCommand(String command) to split a command properly as a normal command. Handling quotes " and ' can be done at a fairly low cost. But there are complications around escaped quotes.

From an SO post:

As documentation, translateCommandline handles both single and double quoted strings and escapes within them but does not recognize backslash in the same way as a POSIX shell because of problems that causes on DOS based systems.

I think POSIX based commands are different from DOS based commands. Though, I think it should be fine to assume POSIX based commands as docker containers are linux based. Though, there might be use cases where one might expect to run a DOS based command? I think a bit more time is required to think through this problem.

In the meantime, as a workaround - would it be possible to use one of the solutions on this SO post and then passing into the alternate method setCommand(String... commandParts)?

[dhoard]

@REslim30 Thanks for the insight. I can see where non-POSIX commands could be a probably.

I currently have a workaround in place, so it's not blocking me.

[aidando73]

On the dockerfile docs, there seems to be mention of windows containers:

RUN (shell form, the command is run in a shell, which by default is /bin/sh -c on Linux or cmd /S /C on Windows)

Windows seems to be a container that you can run that is windows based. Though there are some caveats:

Windows requires the host OS version to match the container OS version. If you want to run a container based on a newer Windows build, make sure you have an equivalent host build. Otherwise, you can use Hyper-V isolation to run older containers on new host builds. You can read more on Windows Container Version Compatibility in our Container Docs.

Your host must have the Windows container feature enabled. The Windows container feature is only available on Windows Server 2016 (Core and with Desktop Experience), Windows 10 Professional and Enterprise (Anniversary Edition) and later.

This image has 1M downloads so it is something we should consider.

[aidando73]

I believe Windows containers on windows (WCOW) isn't currently supported on Test containers. That work is currently being done to support it as per https://github.com/testcontainers/testcontainers-java/issues/5621.

I feel like it would be okay to assume that withCommand will be used with POSIX commands. Maybe in the future we deprecate this and create new methods withPosixCommand & withWindowsCommand, once windows containers become more prominent.

I feel at this stage we can just provide a basic implementation that supports quotes (and not \"). Which will support a larger subset of command strings.

Really the only difference I've found is support for escaping double quotes.

echo "\""

doesn't work in windows but it does on linux.

[dhoard]

I feel that a better approach than withPosixCommand and withWindowsCommand would be something like the following...

public SELF withCommand(String cmd, Function<String, String[]> cmdProcessor) {
    withCommand(cmdProcessor.apply(cmd));
    return this;
}

public class CommandProcessor {

    public static final Function<String, String[]> POSIX = new Posix();
    public static final Function<String, String[]> WINDOWS = new Windows();

    public static class Posix implements Function<String, String[]> {

        @Override
        public String[] apply(String s) {
            // TODO
            return null;
        }
    }

    public static class Windows implements Function<String, String[]> {

        @Override
        public String[] apply(String s) {
            // TODO
            return null;
        }
    }
}

Example usages:

GenericContainer genericContainer = new GenericContainer();
genericContainer.withCommand("testing 1 2 4", CommandProcessor.POSIX);

GenericContainer genericContainer = new GenericContainer();
genericContainer.withCommand("testing 1 2 4", CommandProcessor.WINDOWS);

This would also allow a developer to provide a custom command processor to split commands, replace values in a command, validate command format, build dynamic commands, etc.

[aidando73]

@dhoard I like that suggestion; It's better than the withPosixCommand and withWindowsCommand in that it's flexible and is open to extension.

But before going with that approach. I think we should just extend the current withCommand(String) method, to handle quotes and single quotes. And in future, if windows containers become a thing or a certain linux distro splits commands differently or we learn more about the different use cases users expect withCommand to support, we can deprecate withCommand(String) and go with a style similar to what you suggested. I think we still have incomplete knowledge of how strings are split across platforms (maybe it's not a POSIX vs Windows thing but there are differences across unix platforms and maybe across DOS platforms).

[aidando73]

It actually might be a shell thing. From man bash

DEFINITIONS
       The following definitions are used throughout the rest of this document.
       blank  A space or tab.
       word   A sequence of characters considered as a single unit by the shell.  Also known as a token.
       name   A word consisting only of alphanumeric characters and underscores, and beginning with an alphabetic character or an underscore.  Also referred to as an iden-
              tifier.
       metacharacter
              A character that, when unquoted, separates words.  One of the following:
              |  & ; ( ) < > space tab newline

QUOTING
       Quoting is used to remove the special meaning of certain characters or words to the shell.  Quoting can be used to disable special treatment for special characters,
       to prevent reserved words from being recognized as such, and to prevent parameter expansion.

       Each of the metacharacters listed above under DEFINITIONS has special meaning to the shell and must be quoted if it is to represent itself.

       When the command history expansion facilities are being used (see HISTORY EXPANSION below), the history expansion character, usually !, must be  quoted  to  prevent
       history expansion.

       There are three quoting mechanisms: the escape character, single quotes, and double quotes.
...

[eddumelendez]

Sorry for the late reply but something like this should work withCommand("sh", "-c", "this is my command, a large one)

[dhoard]

Sorry for the late reply but something like this should work withCommand("sh", "-c", "this is my command, a large one)

@eddumelendez agreed... and the approach I took.

The lack of support for quoted arguments when using setCommand(String) / withCommand(String) is not intuitive.

Would you accept a Javadoc PR to make this more apparent?

[eddumelendez]

I think an improvement in our docs make sense. Please, take into account the code snippets in our docs are concentre test examples. PR is welcome :)

[eddumelendez]

Closing as this one was solved. PR for docs is welcome.