[PATCH v2 0/2] MR10008: start: Fix arg parsing for quoted paths ending with backslash.
When using the `start` command with a `/d` switch specifying a quoted path that ends with a backslash (e.g., `start /d "c:\path\" app.exe`), subsequent arguments were incorrectly concatenated into the path string due to parsing ambiguity. The error I encountered was with `start /d "c:\temp\" test.exe`, which was parsed into the arguments `/d` and `"c:\temp\" test.exe`. The path and the executable were merged into a single argument as a new path, preventing the child process from launching. In my first patch, I attempted to identify and split this combined argument, but this caused test failures in `kernel32`. I then retested on Windows and found that Windows also parses it as `/d` and `"c:\temp\" test.exe`—Wine's parsing is consistent with Windows. However, `start` works correctly on Windows because it is a built-in command, whereas in Wine, `start` is an external executable, leading to different behavior. Signed-off-by: YeshunYe yeyeshun@uniontech.com -- v2: start: Refactor command line argument parsing functions. https://gitlab.winehq.org/wine/wine/-/merge_requests/10008
From: YeshunYe <yeyeshun@uniontech.com> Added a test case to verify the scenario where the path following `/d` ends with a backslash (`\`). Signed-off-by: YeshunYe <yeyeshun@uniontech.com> --- programs/cmd/tests/test_builtins.bat | 1 + programs/cmd/tests/test_builtins.bat.exp | 1 + programs/cmd/tests/test_builtins.cmd | 1 + programs/cmd/tests/test_builtins.cmd.exp | 1 + 4 files changed, 4 insertions(+) diff --git a/programs/cmd/tests/test_builtins.bat b/programs/cmd/tests/test_builtins.bat index 4c3d0e7107e..19cf4f202aa 100644 --- a/programs/cmd/tests/test_builtins.bat +++ b/programs/cmd/tests/test_builtins.bat @@ -96,6 +96,7 @@ mkdir foo & cd foo set "FOO_PATH=%cd%" > NUL cd .. call :setError 666 & (start /B /WAIT /d "%FOO_PATH%" cmd /s /c "if /I \"%%cd%%\"==\"%FOO_PATH%\" (exit 0) else (exit 1)" >nul &&echo !errorlevel!) +call :setError 666 & (start /B /WAIT /d "%FOO_PATH%\" cmd /s /c "if /I \"%%cd%%\"==\"%FOO_PATH%\" (exit 0) else (exit 1)" >nul &&echo !errorlevel!) rd /q /s foo echo --- success/failure for TYPE command mkdir foo & cd foo diff --git a/programs/cmd/tests/test_builtins.bat.exp b/programs/cmd/tests/test_builtins.bat.exp index 36e14d5be91..20dda0c9ca7 100644 --- a/programs/cmd/tests/test_builtins.bat.exp +++ b/programs/cmd/tests/test_builtins.bat.exp @@ -63,6 +63,7 @@ foo@space@ SUCCESS 1024 @todo_wine@SUCCESS 666 0 +@todo_wine@0 --- success/failure for TYPE command FAILURE 1 SUCCESS 0 diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index f65a301d914..91af319b686 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -679,6 +679,7 @@ mkdir foo & cd foo set "FOO_PATH=%cd%" > NUL cd .. call :setError 666 & (start /B /WAIT /d "%FOO_PATH%" cmd /s /c "if /I \"%%cd%%\"==\"%FOO_PATH%\" (exit 0) else (exit 1)" >nul &&echo !errorlevel!) +call :setError 666 & (start /B /WAIT /d "%FOO_PATH%\" cmd /s /c "if /I \"%%cd%%\"==\"%FOO_PATH%\" (exit 0) else (exit 1)" >nul &&echo !errorlevel!) rd /q /s foo echo --- success/failure for TYPE command mkdir foo & cd foo diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 0f6d9bdb494..f5358709f83 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -582,6 +582,7 @@ foo@space@ SUCCESS 1024 @todo_wine@SUCCESS 666 0 +@todo_wine@0 --- success/failure for TYPE command FAILURE 1 SUCCESS 0 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10008
From: YeshunYe <yeyeshun@uniontech.com> When using the `start` command with a `/d` switch specifying a quoted path that ends with a backslash (e.g., `start /d "c:\path\" app.exe`), subsequent arguments were incorrectly concatenated into the path string due to parsing ambiguity. Signed-off-by: YeshunYe <yeyeshun@uniontech.com> --- programs/cmd/tests/test_builtins.bat.exp | 2 +- programs/cmd/tests/test_builtins.cmd.exp | 2 +- programs/start/start.c | 270 ++++++++++------------- 3 files changed, 122 insertions(+), 152 deletions(-) diff --git a/programs/cmd/tests/test_builtins.bat.exp b/programs/cmd/tests/test_builtins.bat.exp index 20dda0c9ca7..383c237b684 100644 --- a/programs/cmd/tests/test_builtins.bat.exp +++ b/programs/cmd/tests/test_builtins.bat.exp @@ -63,7 +63,7 @@ foo@space@ SUCCESS 1024 @todo_wine@SUCCESS 666 0 -@todo_wine@0 +0 --- success/failure for TYPE command FAILURE 1 SUCCESS 0 diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index f5358709f83..c88dee42c16 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -582,7 +582,7 @@ foo@space@ SUCCESS 1024 @todo_wine@SUCCESS 666 0 -@todo_wine@0 +0 --- success/failure for TYPE command FAILURE 1 SUCCESS 0 diff --git a/programs/start/start.c b/programs/start/start.c index 38aafe870a9..2ff0c177f96 100644 --- a/programs/start/start.c +++ b/programs/start/start.c @@ -116,85 +116,6 @@ static void usage(void) fatal_string(STRING_USAGE); } -/*********************************************************************** - * build_command_line - * - * Build the command line of a process from the argv array. - * - * We must quote and escape characters so that the argv array can be rebuilt - * from the command line: - * - spaces and tabs must be quoted - * 'a b' -> '"a b"' - * - quotes must be escaped - * '"' -> '\"' - * - if '\'s are followed by a '"', they must be doubled and followed by '\"', - * resulting in an odd number of '\' followed by a '"' - * '\"' -> '\\\"' - * '\\"' -> '\\\\\"' - * - '\'s are followed by the closing '"' must be doubled, - * resulting in an even number of '\' followed by a '"' - * ' \' -> '" \\"' - * ' \\' -> '" \\\\"' - * - '\'s that are not followed by a '"' can be left as is - * 'a\b' == 'a\b' - * 'a\\b' == 'a\\b' - */ -static WCHAR *build_command_line( WCHAR **wargv ) -{ - int len; - WCHAR **arg, *ret; - LPWSTR p; - - len = 1; - for (arg = wargv; *arg; arg++) len += 3 + 2 * wcslen( *arg ); - if (!(ret = malloc( len * sizeof(WCHAR) ))) return NULL; - - p = ret; - for (arg = wargv; *arg; arg++) - { - BOOL has_space, has_quote; - int i, bcount; - WCHAR *a; - - /* check for quotes and spaces in this argument */ - has_space = !**arg || wcschr( *arg, ' ' ) || wcschr( *arg, '\t' ); - has_quote = wcschr( *arg, '"' ) != NULL; - - /* now transfer it to the command line */ - if (has_space) *p++ = '"'; - if (has_quote || has_space) - { - bcount = 0; - for (a = *arg; *a; a++) - { - if (*a == '\\') bcount++; - else - { - if (*a == '"') /* double all the '\\' preceding this '"', plus one */ - for (i = 0; i <= bcount; i++) *p++ = '\\'; - bcount = 0; - } - *p++ = *a; - } - } - else - { - wcscpy( p, *arg ); - p += wcslen( p ); - } - if (has_space) - { - /* Double all the '\' preceding the closing quote */ - for (i = 0; i < bcount; i++) *p++ = '\\'; - *p++ = '"'; - } - *p++ = ' '; - } - if (p > ret) p--; /* remove last space */ - *p = 0; - return ret; -} - static WCHAR *get_parent_dir(const WCHAR* path) { WCHAR *last_slash; @@ -214,17 +135,24 @@ static WCHAR *get_parent_dir(const WCHAR* path) return result; } -static BOOL is_option(const WCHAR* arg, const WCHAR* opt) +static BOOL is_option(WCHAR *arg, WCHAR **next_arg, const WCHAR* opt) { - return CompareStringW(LOCALE_USER_DEFAULT, NORM_IGNORECASE, - arg, -1, opt, -1) == CSTR_EQUAL; + int opt_len = wcslen(opt); + BOOL result = (arg[opt_len] == ' ' || arg[opt_len] == '\t' || arg[opt_len] == 0) && + CompareStringW(LOCALE_USER_DEFAULT, NORM_IGNORECASE, + arg, opt_len, opt, -1) == CSTR_EQUAL; + if (result) + *next_arg = arg + opt_len; + return result; } -static BOOL is_option_with_arg(WCHAR **argv, int *pos, const WCHAR* opt) +static BOOL is_option_with_arg(WCHAR *arg, WCHAR **next_arg, const WCHAR* opt) { - if (!is_option( argv[*pos], opt )) return FALSE; - (*pos)++; - if (!argv[*pos]) + if (!is_option( arg, next_arg, opt )) return FALSE; + while(arg[0] == ' ' || arg[0] == '\t') + arg++; + *next_arg = arg; + if (!arg[0]) { WINE_ERR("missing argument for %s\n", debugstr_w(opt)); usage(); @@ -232,21 +160,62 @@ static BOOL is_option_with_arg(WCHAR **argv, int *pos, const WCHAR* opt) return TRUE; } -static WCHAR *parse_title(const WCHAR *arg) +static WCHAR *parse_next_arg( WCHAR *src, WCHAR **next_arg ) { - /* See: - * WCMD_start() in programs/cmd/builtins.c - * WCMD_parameter_with_delims() in programs/cmd/batch.c - * The shell has already tokenized the command line for us. - * All we need to do is filter out all the quotes. - */ - int next; - const WCHAR *p = arg; - WCHAR *title = malloc( wcslen(arg) * sizeof(WCHAR) ); + WCHAR *arg, *dst; + int in_quotes = 0, bcount = 0; - for (next = 0; *p; p++) if (*p != '"') title[next++] = *p; - title[next] = 0; - return title; + arg = dst = wcsdup(src); + + while (*src == ' ' || *src == '\t') src++; + + while (*src) + { + if ((*src == ' ' || *src == '\t') && !in_quotes) + { + /* skip the remaining spaces */ + while (*src == ' ' || *src == '\t') src++; + if (!*src) break; + /* close the argument and copy it */ + *dst++ = 0; + break; + } + else if (*src == '\\') + { + *dst++ = *src++; + bcount++; + } + else if (*src == '"') + { + if ((bcount & 1) == 0) + { + /* Preceded by an even number of '\', this is half that + * number of '\', plus a '"' which we discard. + */ + dst -= bcount / 2; + src++; + in_quotes = !in_quotes; + if (bcount) bcount--; + } + else + { + /* Preceded by an odd number of '\', this is half that + * number of '\' followed by a '"' + */ + dst -= bcount / 2 + 1; + *dst++ = *src++; + bcount = 0; + } + } + else /* a regular character */ + { + *dst++ = *src++; + bcount = 0; + } + } + *dst = 0; + *next_arg = src; + return arg; } static WCHAR *get_dos_filename( const WCHAR *unix_file ) @@ -421,12 +390,12 @@ static struct BOOL cp_inherit; } opts; -static void parse_command_line( int argc, WCHAR *argv[] ) +static void parse_command_line( void ) { - int i; const WCHAR *file = NULL; BOOL unix_mode = FALSE; BOOL progid_open = FALSE; + WCHAR *arg; opts.sei.cbSize = sizeof(opts.sei); opts.sei.lpVerb = L"open"; @@ -436,115 +405,116 @@ static void parse_command_line( int argc, WCHAR *argv[] ) opts.creation_flags = CREATE_NEW_CONSOLE; opts.cp_inherit = FALSE; + arg = wcsdup(GetCommandLineW()); + /* skip argv[0] */ + parse_next_arg(arg, &arg); + /* Canonical Microsoft commandline flag processing: * flags start with / and are case insensitive. */ - for (i = 1; i < argc; i++) + while (*arg) { /* parse first quoted argument as console title */ - if (!opts.title && argv[i][0] == '"') { - opts.title = parse_title(argv[i]); + if (!opts.title && arg[0] == '"') { + opts.title = parse_next_arg(arg, &arg); continue; } - if (argv[i][0] != '/') + if (arg[0] == ' ' || arg[0] == '\t') { + arg++; + continue; + } + if (arg[0] != '/') break; - if (argv[i][1] == 'd' || argv[i][1] == 'D') { - if (argv[i][2]) - /* The start directory was concatenated to the option */ - opts.sei.lpDirectory = argv[i]+2; - else if (i+1 == argc) { - WINE_ERR("you must specify a directory path for the /d option\n"); - usage(); - } else - opts.sei.lpDirectory = argv[++i]; - - if (opts.sei.lpDirectory[0] == '\"') - { + if (arg[1] == 'd' || arg[1] == 'D') { + arg += 2; + opts.sei.lpDirectory = parse_next_arg(arg, &arg); + + if (opts.sei.lpDirectory[0] == '\"') { int len = wcslen(opts.sei.lpDirectory); - if (len > 1 && opts.sei.lpDirectory[len - 1] == '\"') - { + if (len > 1 && opts.sei.lpDirectory[len - 1] == '\"') { WCHAR* lpDirectory = wcsdup(opts.sei.lpDirectory); lpDirectory[len - 1] = 0; opts.sei.lpDirectory = lpDirectory + 1; } } } - else if (is_option(argv[i], L"/b")) + else if (is_option(arg, &arg, L"/b")) opts.creation_flags &= ~CREATE_NEW_CONSOLE; - else if (argv[i][0] == '/' && (argv[i][1] == 'i' || argv[i][1] == 'I')) + else if (arg[0] == '/' && (arg[1] == 'i' || arg[1] == 'I')) { + arg += 2; TRACE("/i is ignored\n"); /* FIXME */ - else if (is_option(argv[i], L"/min")) + } + else if (is_option(arg, &arg, L"/min")) opts.sei.nShow = SW_SHOWMINIMIZED; - else if (is_option(argv[i], L"/max")) + else if (is_option(arg, &arg, L"/max")) opts.sei.nShow = SW_SHOWMAXIMIZED; - else if (is_option(argv[i], L"/low")) + else if (is_option(arg, &arg, L"/low")) opts.creation_flags |= IDLE_PRIORITY_CLASS; - else if (is_option(argv[i], L"/normal")) + else if (is_option(arg, &arg, L"/normal")) opts.creation_flags |= NORMAL_PRIORITY_CLASS; - else if (is_option(argv[i], L"/high")) + else if (is_option(arg, &arg, L"/high")) opts.creation_flags |= HIGH_PRIORITY_CLASS; - else if (is_option(argv[i], L"/realtime")) + else if (is_option(arg, &arg, L"/realtime")) opts.creation_flags |= REALTIME_PRIORITY_CLASS; - else if (is_option(argv[i], L"/abovenormal")) + else if (is_option(arg, &arg, L"/abovenormal")) opts.creation_flags |= ABOVE_NORMAL_PRIORITY_CLASS; - else if (is_option(argv[i], L"/belownormal")) + else if (is_option(arg, &arg, L"/belownormal")) opts.creation_flags |= BELOW_NORMAL_PRIORITY_CLASS; - else if (is_option(argv[i], L"/separate")) + else if (is_option(arg, &arg, L"/separate")) opts.creation_flags |= CREATE_SEPARATE_WOW_VDM; - else if (is_option(argv[i], L"/shared")) + else if (is_option(arg, &arg, L"/shared")) opts.creation_flags |= CREATE_SHARED_WOW_VDM; - else if (is_option(argv[i], L"/w") || is_option(argv[i], L"/wait")) + else if (is_option(arg, &arg, L"/w") || is_option(arg, &arg, L"/wait")) opts.sei.fMask |= SEE_MASK_NOCLOSEPROCESS; - else if (is_option(argv[i], L"/?")) + else if (is_option(arg, &arg, L"/?")) usage(); - else if (is_option_with_arg(argv, &i, L"/node")) + else if (is_option_with_arg(arg, &arg, L"/node")) TRACE("/node is ignored\n"); /* FIXME */ - else if (is_option_with_arg(argv, &i, L"/affinity")) + else if (is_option_with_arg(arg, &arg, L"/affinity")) TRACE("/affinity is ignored\n"); /* FIXME */ - else if (is_option_with_arg(argv, &i, L"/machine")) + else if (is_option_with_arg(arg, &arg, L"/machine")) { - if (is_option( argv[i], L"x86" )) opts.machine = IMAGE_FILE_MACHINE_I386; - else if (is_option( argv[i], L"amd64" )) opts.machine = IMAGE_FILE_MACHINE_AMD64; - else if (is_option( argv[i], L"arm" )) opts.machine = IMAGE_FILE_MACHINE_ARMNT; - else if (is_option( argv[i], L"arm64" )) opts.machine = IMAGE_FILE_MACHINE_ARM64; + if (is_option(arg, &arg, L"x86")) opts.machine = IMAGE_FILE_MACHINE_I386; + else if (is_option(arg, &arg, L"amd64")) opts.machine = IMAGE_FILE_MACHINE_AMD64; + else if (is_option(arg, &arg, L"arm")) opts.machine = IMAGE_FILE_MACHINE_ARMNT; + else if (is_option(arg, &arg, L"arm64")) opts.machine = IMAGE_FILE_MACHINE_ARM64; else usage(); } /* Wine extensions */ - else if (is_option_with_arg(argv, &i, L"/unix")) { + else if (is_option_with_arg(arg, &arg, L"/unix")) { unix_mode = TRUE; break; } - else if (is_option(argv[i], L"/exec")) { + else if (is_option(arg, &arg, L"/exec")) { opts.creation_flags = 0; opts.sei.fMask = SEE_MASK_NOCLOSEPROCESS | SEE_MASK_NO_CONSOLE | SEE_MASK_FLAG_NO_UI; opts.cp_inherit = TRUE; - i++; break; } - else if (is_option_with_arg(argv, &i, L"/progIDOpen")) { + else if (is_option_with_arg(arg, &arg, L"/progIDOpen")) { progid_open = TRUE; - opts.sei.lpClass = argv[i++]; + opts.sei.lpClass = parse_next_arg(arg, &arg); opts.sei.fMask |= SEE_MASK_CLASSNAME; break; } else { - WINE_ERR("Unknown option '%s'\n", wine_dbgstr_w(argv[i])); + WINE_ERR("Unknown option '%s'\n", wine_dbgstr_w(arg)); usage(); } } - if (i == argc) + if (arg[0] == 0) { if (progid_open) usage(); file = L"cmd.exe"; } - else file = argv[i++]; + else file = parse_next_arg(arg, &arg); - opts.sei.lpParameters = build_command_line( &argv[i] ); + opts.sei.lpParameters = arg; if ((unix_mode || progid_open) && file[0] == '/') { @@ -566,7 +536,7 @@ int __cdecl wmain (int argc, WCHAR *argv[]) { DWORD binary_type; - parse_command_line( argc, argv ); + parse_command_line(); if (GetBinaryTypeW(opts.sei.lpFile, &binary_type)) { WCHAR *commandline; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10008
actually I was more thinking of copying as is cmd arg parsing code into programs/start rather than creating a new version of it this new version that contains bits that are arguable at best, and wrong at worst * is_option can read after end of arg string (it uses blindly applies option length) * is_option doesn't follow cmd 'logic' (eg one can pass options without spaces between them -` start "foo"/min/wait/d"mydir" notepad`) * iswspace is more readable IMO than _ptr == ' ' ||_ ptr == '\\t' * ... (didn't do all the way)... but I'm not sure how bad it is to inject all the cmd' relevant code into programs/start on the other hand, WCMD_parameter has a poor design (reiterating over first N-1 parameters to get Nth is a bit overkill IMO; if need to discriminate arg from /qualifier, need to keep quotes and then test); WCMD_parse better separates args from qualifiers, but is limited in number of args... in both cases, using global variables to store result should be avoided I'm reluctant to push to programs/start arg handling code which is not the very same as (the) one from program/cmd's (what you've shown is that the standard argc/argv handling isn't what's expected) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10008#note_128963
replying to myself I think the (at least the first one \<g\>) issue lies in cmd's arg parsing when handling the builtin START command using your test with /d "foo\\" cmd.exe, and looking at argv in start.exe, last argv is `"foo\" cmd.exe` which is not what's expected (we want two args instead of one) and looking at arg parsing result in builtin start command in cmd.exe, we do get `"foo\" foo.exe` as a single arg so another approach could be to fix the arg parsing in cmd.exe (and likely afterwards, the strings passed to external start.exe shall be properly escaped - as shown by trying from cmd.exe `start "foo\" notepad.exe` - doesn't start notepad \<g\>) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10008#note_129041
On Mon Feb 9 01:59:00 2026 +0000, eric pouech wrote:
replying to myself I think the (at least the first one \<g\>) issue lies in cmd's arg parsing when handling the builtin START command using your test with /d "foo\\" cmd.exe, and looking at argv in start.exe, last argv is `"foo\" cmd.exe` which is not what's expected (we want two args instead of one) and looking at arg parsing result in builtin start command in cmd.exe, we do get `"foo\" foo.exe` as a single arg so another approach could be to fix the arg parsing in cmd.exe (and likely afterwards, the strings passed to external start.exe shall be properly escaped - as shown by trying from cmd.exe `start "foo\" notepad.exe` - doesn't start notepad \<g\>) I think I understand what you mean. I will copy the `WCMD_parameter` function and use it to parse the command line obtained from `GetCommandLineW`, then reconstruct the `argv` for the `start` command.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10008#note_129119
participants (3)
-
eric pouech (＠epo) -
Yeshun Ye (＠yeyeshun)
-
YeshunYe