sheredom / subprocess.h

🐜 single header process launching solution for C and C++
The Unlicense
1.1k stars 97 forks source link

Do not quote argument if argument doesn't contain whitespace #61

Closed takase1121 closed 2 years ago

takase1121 commented 2 years ago

The solution proposed in #59

Fixes #59

sheredom commented 2 years ago

Is there a way to provide a test case that this fix would ensure worked? I don't want to regress this behaviour again in future!

takase1121 commented 2 years ago

Here is a simple test case I can think of:

const char *command_line[] = { "cmd", "/c", "echo hello world", NULL }

When the old behavior is used cmd will throw an error, while this PR would print hello world.

I'm not too sure how to add that to the test harness though.

sheredom commented 2 years ago

This will need a test case. Easiest way I can think is to add a variant of https://github.com/sheredom/subprocess.h/blob/master/test/process_return_argv.c which returns the strlen of argv[1] + argv[2], and then call it with command line {"process_return_argv_lens", "will be quoted", "will_not_be_quoted", 0}.

Thoughts?

takase1121 commented 2 years ago

This is a Windows specific test case, how about printing back GetCommandLineA in process_return_argv and then check if the argument is properly quoted?

EDIT: nvm, it wouldn't work, the argv[0] would be annoying to deal with

takase1121 commented 2 years ago

This will need a test case. Easiest way I can think is to add a variant of https://github.com/sheredom/subprocess.h/blob/master/test/process_return_argv.c which returns the strlen of argv[1] + argv[2], and then call it with command line {"process_return_argv_lens", "will be quoted", "will_not_be_quoted", 0}.

Thoughts?

I'm unsure if this would work. If a program is linked with MSVCRT and uses its argc and argv, these rules are followed. If a program uses CommandLineToArgvW, these slightly different rules are followed. Lastly, DOS and legacy Windows applicationes DOES NOT follow any of these conventions (which is the why this PR existed, eg. the cmd example I gave). They parse the entire lpCmdline however they want.

takase1121 commented 2 years ago

I added a test case for this PR; basically a helper process process_return_lpcmdline will return the result from GetCommandLineA verbatim. The output is compared from the back so that it skips the executable name.

The temp array was set to 32767 because that was the maximum length of lpCmdline. For non-Windows part of the test, the argument is simply quoted (not properly escaped) so the test should pass most of the time.

sheredom commented 2 years ago

Great work, thanks!