Skip to content

Commit 01706c9

Browse files
dkbennettdkbennett
authored
CLI: Fix forwarded args beginning with '-' from being a parser error (#40131)
1 parent fcd4259 commit 01706c9

4 files changed

Lines changed: 95 additions & 42 deletions

File tree

‎src/windows/wslc/arguments/ArgumentParser.cpp‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,10 @@ ParseArgumentsStateMachine::State ParseArgumentsStateMachine::ProcessAnchoredPos
173173
if ((m_executionArgs.Count(m_anchorPositional.value().Type()) < m_anchorPositional.value().Limit()) ||
174174
(m_anchorPositional.value().Limit() == NO_LIMIT))
175175
{
176-
// validate that we dont have any invalid argument specifiers.
176+
// Validate that we don't have any invalid argument specifiers.
177+
// Anchor positionals with multiple values should be order-independent, which means a
178+
// '-' at the start of the first one would be invalid, so it should also be invalid for
179+
// all other anchor positionals of the same type.
177180
if (!currArg.empty() && currArg[0] == WSLC_CLI_ARG_ID_CHAR)
178181
{
179182
return ArgumentException(Localization::WSLCCLI_InvalidArgumentSpecifierError(currArg));
@@ -192,12 +195,6 @@ ParseArgumentsStateMachine::State ParseArgumentsStateMachine::ProcessAnchoredPos
192195
const Argument* nextPositional = NextPositional();
193196
if (nextPositional)
194197
{
195-
// validate that we dont have any invalid argument specifiers.
196-
if (!currArg.empty() && currArg[0] == WSLC_CLI_ARG_ID_CHAR)
197-
{
198-
return ArgumentException(Localization::WSLCCLI_InvalidArgumentSpecifierError(currArg));
199-
}
200-
201198
m_executionArgs.Add(nextPositional->Type(), std::wstring{currArg});
202199
return {};
203200
}

‎test/windows/wslc/CommandLineTestCases.h‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,19 @@ COMMAND_LINE_TEST_CASE(L"container list --format json", L"list", true)
5555
COMMAND_LINE_TEST_CASE(L"container list --format table", L"list", true)
5656
COMMAND_LINE_TEST_CASE(L"container list --format badformat", L"list", false)
5757
COMMAND_LINE_TEST_CASE(L"run ubuntu", L"run", true)
58+
COMMAND_LINE_TEST_CASE(L"run --rm -it --entrypoint bash archlinux:latest -c \"echo 123\"", L"run", true)
59+
COMMAND_LINE_TEST_CASE(L"run --rm --entrypoint /bin/bash debian:latest -c ls", L"run", true)
60+
COMMAND_LINE_TEST_CASE(L"run jrottenberg/ffmpeg:4.4-alpine -i http://url/to/media.mp4 -stats", L"run", true)
61+
COMMAND_LINE_TEST_CASE(
62+
L"run -v ${PWD}:/data jrottenberg/ffmpeg:4.4-scratch -stats -i http://www.hevc-10bit.mkv -c:v libx265 -pix_fmt yuv420p10 -t "
63+
L"5 -f mp4 test.mp4",
64+
L"run",
65+
true)
66+
COMMAND_LINE_TEST_CASE(
67+
L"run -v ${PWD}:/data -it jrottenberg/ffmpeg:4.4-scratch -stats -i https://file-examples/file_example_MP4_480_1_5MG.mp4 -c:v "
68+
L"libx265 -pix_fmt yuv420p10 -t 5 -f mp4 /dataout.mp4",
69+
L"run",
70+
true)
5871
COMMAND_LINE_TEST_CASE(L"container run ubuntu bash -c 'echo Hello World'", L"run", true)
5972
COMMAND_LINE_TEST_CASE(L"container run ubuntu", L"run", true)
6073
COMMAND_LINE_TEST_CASE(L"container run -it --name foo ubuntu", L"run", true)
@@ -77,7 +90,6 @@ COMMAND_LINE_TEST_CASE(L"exec --workdir /app cont1 echo Hello", L"exec", true)
7790
COMMAND_LINE_TEST_CASE(L"exec -w /app cont1 echo Hello", L"exec", true)
7891
COMMAND_LINE_TEST_CASE(L"container exec --workdir /app cont1 sh", L"exec", true)
7992
COMMAND_LINE_TEST_CASE(L"exec --workdir", L"exec", false) // Missing value for --workdir
80-
COMMAND_LINE_TEST_CASE(L"exec cont1 --workdir", L"exec", false) // Invalid argument specifier after container id
8193
COMMAND_LINE_TEST_CASE(L"exec --workdir \"\" cont1 echo Hello", L"exec", false) // Empty working directory
8294
COMMAND_LINE_TEST_CASE(L"kill cont1 --signal sigkill", L"kill", true)
8395
COMMAND_LINE_TEST_CASE(L"container kill cont1 -s KILL", L"kill", true)

‎test/windows/wslc/ParserTestCases.h‎

Lines changed: 38 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@ inline std::vector<wsl::windows::wslc::Argument> GetArgumentsForSet(ArgumentSet
4444
{
4545
case ArgumentSet::Run:
4646
return {
47-
Argument::Create(ArgType::ContainerId, true), // Required positional argument
48-
Argument::Create(ArgType::Command, false), // Optional positional argument
47+
Argument::Create(ArgType::ImageId, true), // Required positional argument
48+
Argument::Create(ArgType::Command, false), // Optional positional argument
4949
Argument::Create(ArgType::ForwardArgs, false),
5050
Argument::Create(ArgType::Help),
5151
Argument::Create(ArgType::Interactive),
@@ -76,42 +76,50 @@ inline std::vector<wsl::windows::wslc::Argument> GetArgumentsForSet(ArgumentSet
7676
#define WSLC_PARSER_TEST_CASES \
7777
/* Simple case with required arg and simple other args */ \
7878
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -h)") \
79-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc cont1)") \
80-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc --verbose cont1)") \
79+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc image1)") \
80+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc --verbose image1)") \
8181
\
8282
/* Value tests, flag and non-flag, multi-value */ \
83-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc --publish=80:80 cont1)") \
84-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc --publish 80:80 cont1)") \
85-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -p=80:80 cont1)") \
86-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -p 80:80 cont1)") \
87-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -p 80:80 -p 443:443 cont1)") \
88-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -p=80:80 -p=443:443 cont1)") \
89-
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc --verbose --verbose cont1)") \
83+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc --publish=80:80 image1)") \
84+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc --publish 80:80 image1)") \
85+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -p=80:80 image1)") \
86+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -p 80:80 image1)") \
87+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -p 80:80 -p 443:443 image1)") \
88+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -p=80:80 -p=443:443 image1)") \
89+
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc --verbose --verbose image1)") \
9090
\
9191
/* Flag parse tests */ \
92-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -h cont1)") \
93-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -hi cont1)") \
94-
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc -ihp- cont1)") \
95-
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc -pih cont1)") \
96-
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc -pih=80:80 cont1)") \
97-
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc -pih 80:80 cont1)") \
98-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -ihp 80:80 cont1)") \
99-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -ihp=80:80 cont1)") \
92+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -h image1)") \
93+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -hi image1)") \
94+
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc -ihp- image1)") \
95+
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc -pih image1)") \
96+
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc -pih=80:80 image1)") \
97+
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc -pih 80:80 image1)") \
98+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -ihp 80:80 image1)") \
99+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -ihp=80:80 image1)") \
100100
\
101101
/* Validation tests */ \
102-
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc --signal FOO cont1)") \
103-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc --signal 9 cont1)") \
104-
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc -t blah)") \
105-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -t 5)") \
102+
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc --signal FOO image1)") \
103+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc --signal 9 image1)") \
104+
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc -t blah image1)") \
105+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc -t 5 image1)") \
106106
\
107107
/* Multi-positional tests */ \
108-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc cont1 command)") \
109-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc cont1 command --f -z forward hello world)") \
110-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc cont1 command forward hello world)") \
111-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc cont1 command forward"hello world")") \
112-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc cont1 command f="hello world" forward echo)") \
113-
WSLC_PARSER_TEST_CASE(Run, false, LR"(wslc cont1 -v command f="hello world" forward echo)") \
114-
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc cont1 \\command\\?"" --f -z forward hello world)") \
108+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc image1 command)") \
109+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc image1 command --f -z forward hello world)") \
110+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc image1 command forward hello world)") \
111+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc image1 command forward"hello world")") \
112+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc image1 command f="hello world" forward echo)") \
113+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc --verbose image1 command f="hello world" forward echo)") \
114+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc image1 \\command\\?"" --f -z forward hello world)") \
115+
\
116+
/* Once the image name is parsed, the next token becomes the optional <command> positional \
117+
* and everything after that goes into ForwardArgs. Neither <command> nor ForwardArgs are \
118+
* interpreted as wslc options. The second case uses '\' + newline between tokens, which \
119+
* CommandLineToArgvW passes through as literal '\' tokens that the container shell \
120+
* handles correctly. */ \
121+
WSLC_PARSER_TEST_CASE(Run, true, LR"(wslc jrottenberg/ffmpeg:4.4-alpine ffmpeg -i http://url/to/media.mp4 -stats)") \
122+
WSLC_PARSER_TEST_CASE(Run, true, L"wslc jrottenberg/ffmpeg:4.4-alpine \\\nffmpeg \\\n-i http://url/to/media.mp4 \\\n-stats") \
115123
\
116124
/* List cases with multiple args and flags that can come after the optional multi-positional. */ \
117125
WSLC_PARSER_TEST_CASE(List, true, LR"(wslc)") \

‎test/windows/wslc/WSLCCLIParserUnitTests.cpp‎

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,8 @@ class WSLCCLIParserUnitTests
5858

5959
for (const auto& testCase : testCases)
6060
{
61+
bool succeeded = false;
62+
6163
try
6264
{
6365
Log::Comment(String().Format(L"Testing: %ls", testCase.commandLine.c_str()));
@@ -73,16 +75,48 @@ class WSLCCLIParserUnitTests
7375
stateMachine.ThrowIfError();
7476
}
7577

76-
if (testCase.commandLine.find(L"cont1") != std::wstring::npos)
78+
// Validate count limits and required arguments, mirroring Command::ValidateArguments.
79+
// Skip all validation if --help is present, as Command::ValidateArguments does.
80+
if (!args.Contains(ArgType::Help))
81+
{
82+
for (const auto& arg : GetArgumentsForSet(testCase.argumentSet))
83+
{
84+
if (arg.Required() && !args.Contains(arg.Type()))
85+
{
86+
throw ArgumentException(std::wstring(L"Required argument missing: ") + arg.Name());
87+
}
88+
89+
if ((arg.Limit() > 0) && (arg.Limit() < args.Count(arg.Type())))
90+
{
91+
throw ArgumentException(std::wstring(L"Too many values for argument: ") + arg.Name());
92+
}
93+
94+
if (args.Contains(arg.Type()))
95+
{
96+
arg.Validate(args);
97+
}
98+
}
99+
}
100+
101+
succeeded = true;
102+
103+
if (testCase.commandLine.find(L"image1") != std::wstring::npos && testCase.argumentSet == ArgumentSet::Run)
104+
{
105+
VERIFY_IS_TRUE(args.Contains(ArgType::ImageId));
106+
auto imageId = args.Get<ArgType::ImageId>();
107+
VERIFY_ARE_EQUAL(L"image1", imageId);
108+
}
109+
110+
if (testCase.commandLine.find(L"cont1") != std::wstring::npos && testCase.argumentSet == ArgumentSet::List)
77111
{
78112
VERIFY_IS_TRUE(args.Contains(ArgType::ContainerId));
79113
auto containerId = args.Get<ArgType::ContainerId>();
80114
VERIFY_ARE_EQUAL(L"cont1", containerId);
81115
}
82116

83-
if (testCase.commandLine.find(L"rm") != std::wstring::npos)
117+
if (testCase.commandLine.find(L"--rm") != std::wstring::npos)
84118
{
85-
// Ensure 'rm' was parsed wherever it was found.
119+
// Ensure '--rm' was parsed wherever it was found.
86120
VERIFY_IS_TRUE(args.Contains(ArgType::Remove));
87121
}
88122

@@ -99,7 +133,7 @@ class WSLCCLIParserUnitTests
99133
auto forwardArgs = args.Get<ArgType::ForwardArgs>();
100134
std::wstring forwardArgsConcat = wsl::shared::string::Join(forwardArgs, L' ');
101135
VERIFY_IS_TRUE(forwardArgsConcat.find(L"hello world") != std::wstring::npos); // Forward args should contain hello world
102-
VERIFY_IS_TRUE(forwardArgsConcat.find(L"cont1") == std::wstring::npos); // Forward args should not contain the containerId
136+
VERIFY_IS_TRUE(forwardArgsConcat.find(L"image1") == std::wstring::npos); // Forward args should not contain the imageId
103137
VERIFY_IS_TRUE(forwardArgsConcat.find(L"command") == std::wstring::npos); // Forward args should not contain the command
104138
LogComment(L"Forwarded Args: " + forwardArgsConcat);
105139
}
@@ -134,6 +168,8 @@ class WSLCCLIParserUnitTests
134168
Log::Comment(String().Format(L"Test case threw expected exception: %hs", ex.what()));
135169
}
136170
}
171+
172+
VERIFY_ARE_EQUAL(testCase.expectedResult, succeeded, String().Format(L"Command line: %ls", testCase.commandLine.c_str()));
137173
}
138174
}
139175
};

0 commit comments

Comments
 (0)