[PATCH 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 -- 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 | 172 ++++++++++++++++++++++- 3 files changed, 167 insertions(+), 9 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..60502328086 100644 --- a/programs/start/start.c +++ b/programs/start/start.c @@ -412,6 +412,146 @@ static BOOL search_path(const WCHAR *firstParam, WCHAR **full_path) return FALSE; } +static BOOL parse_incorrect_path(WCHAR *path, WCHAR** remaining_args) +{ + /* start /d "c:\temp\" a.exe + * This will be parsed as 2 arguments: `/d` and `"c:\temp\" a.exe`. + * This will cause `"c:\temp\" a.exe` to be interpreted as a single path, + * which is clearly incorrect. + */ + WCHAR *quote_end = wcschr(path + 1, '\"'); + if (path[0] == '\"' && quote_end && wcslen(quote_end) > 2) { + WCHAR *p = quote_end + 1; + /* truncate path from other parameter */ + quote_end[0] = 0; + + while(*p && (*p == ' ' || *p == '\t')) + p++; + + if (*p) + *remaining_args = p; + + return TRUE; + } + + return FALSE; +} + +static WCHAR **cmdline_to_argv( const WCHAR *src, int *ret_argc ) +{ + WCHAR **argv, *arg, *dst; + int argc, in_quotes = 0, bcount = 0, len = wcslen(src) + 1; + + argc = 2 + len / 2; + argv = HeapAlloc( GetProcessHeap(), 0, argc * sizeof(*argv) + len * sizeof(WCHAR) ); + arg = dst = (WCHAR *)(argv + argc); + argc = 0; + 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; + argv[argc++] = arg; + /* start with a new argument */ + arg = dst; + bcount = 0; + } + 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++; + if (in_quotes && *src == '"') *dst++ = *src++; + else in_quotes = !in_quotes; + } + 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; + argv[argc++] = arg; + argv[argc] = NULL; + *ret_argc = argc; + return argv; +} + +static WCHAR **rebuild_argv( int old_argc, WCHAR **old_argv, WCHAR* lpDirectory, WCHAR* remain_args, int* new_argc) +{ + int argc, i, j, len; + WCHAR **argv; + WCHAR D_name[] = L"/D"; + BOOL dir_concatenated = FALSE; + int index_of_slash_D = 0; + int remain_argc; + WCHAR **remain_argv = cmdline_to_argv(remain_args, &remain_argc); + argc = old_argc + remain_argc + 2 + 1; + argv = HeapAlloc( GetProcessHeap(), 0, argc * sizeof(*argv) ); + /* Copy the arguments until encountering the `/d` parameter. */ + for (i = 0; i < old_argc; i++) + { + if (old_argv[i][0] == '/' && (old_argv[i][1] == 'd' || old_argv[i][1] == 'D')) + { + index_of_slash_D = i; + dir_concatenated = old_argv[i][2] ? TRUE : FALSE; + break; + } + argv[i] = old_argv[i]; + } + /* Build the correct `/d` parameter and path parameter. */ + argv[i++] = D_name; + argv[i++] = lpDirectory; + /* Continue adding the remaining arguments from + * the originally incorrect path parameter into the new argument list. + */ + for (j = 0; j < remain_argc; j++) + { + argv[i++] = remain_argv[j]; + } + /* Copy the arguments following the `/d` and the path parameter. */ + for (j = index_of_slash_D + (dir_concatenated ? 1 : 2); j < old_argc; j++) + { + argv[i++] = old_argv[j]; + } + /* Trim trailing whitespace from the last argument; + * otherwise, the newly constructed argument list will be incorrect. + */ + len = wcslen(argv[i - 1]); + while (argv[i - 1][len - 1] == ' ' || argv[i - 1][len - 1] == '\t') + { + argv[i - 1][len - 1] = 0; + len--; + } + argv[i] = NULL; + *new_argc = i; + + return argv; +} + static struct { SHELLEXECUTEINFOW sei; @@ -459,14 +599,32 @@ static void parse_command_line( int argc, WCHAR *argv[] ) } else opts.sei.lpDirectory = argv[++i]; - if (opts.sei.lpDirectory[0] == '\"') - { + if (opts.sei.lpDirectory[0] == '\"') { int len = wcslen(opts.sei.lpDirectory); - if (len > 1 && opts.sei.lpDirectory[len - 1] == '\"') - { - WCHAR* lpDirectory = wcsdup(opts.sei.lpDirectory); - lpDirectory[len - 1] = 0; - opts.sei.lpDirectory = lpDirectory + 1; + if (len > 1) { + WCHAR *lpDirectory = wcsdup(opts.sei.lpDirectory); + WCHAR *remaining_args = NULL; + if (opts.sei.lpDirectory[len - 1] == '\"') { + lpDirectory[len - 1] = 0; + opts.sei.lpDirectory = lpDirectory + 1; + } else if (parse_incorrect_path(lpDirectory, &remaining_args)) { + /* If `opts.sei.lpDirectory` contains something like `"c:\path\" test.exe`, + * reconstruct the entire correct argument list. + * However, if it is "c:\path\" "test.exe", it will not work on Windows either, + * and no additional handling is needed. + */ + opts.sei.lpDirectory = lpDirectory + 1; + if (remaining_args) { + int new_argc; + WCHAR **new_argv = rebuild_argv(argc, argv, lpDirectory+1, remaining_args, &new_argc); + if (new_argv) + { + argc = new_argc; + argv = new_argv; + } + } + } else + free(lpDirectory); } } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10008
hmm not sure what's the "best" solution here: * either we fix start.exe to parse args from command line as cmd.exe does * or we move start.exe as a cmd.exe builtin command (as it should) if the former, IMO we'd better clone cmd.exe builtin commands args parsing code as the parsing is quite convoluted as it is, so one implementation is likely simpler to maintain - and review -; and agreed that cmd.exe's implementation is kinda ugly as it is) if the later, that looks like a better long term solution, would remove some duplication between start & cmd (esp in shell32 invocation....) but likely requires much more work. Started thinking of it, and the list grew rapidly (and I likely missed a couple of items \<g\>) * first start.exe has some Wine only code (see ntdll/unix/env.c) that should be kept functional if moved to cmd.exe * we likely need to keep the start.exe as: * ctrl-c handling in start.exe cannot be moved to cmd.exe (as the implemented behavior is different) * ctrl-c hit when start.exe is run from unix command line kills both start.exe and it's child; that needs to be preserved (and need killing grand child; quite sure cmd.exe doesn't do it properly) * we may have some external dependancies (potentially unix scripts, but not only) that depend on start.exe (not start): * redirect start.exe invocations to cmd.exe start ... for compatibility (and ensure it never creates a recursion - it shouldn't, as cmd shall recognize 'cmd start' as builtin start, whereas 'cmd start.exe' shall lookup for an external executable) * change some Wine internal code to call cmd.exe start instead of start.exe * fix the tests results in cmd.exe that should now pass so unless you're ready to explore all of this, I'd rather suggest to go for duplicating cmd.exe arg parsing in start.exe (and anyway that's the first step if we ever want to move start.exe as an builtin cmd.exe command) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10008#note_128512
so unless you're ready to explore all of this, I'd rather suggest to go for duplicating cmd.exe arg parsing in start.exe (and anyway that's the first step if we ever want to move start.exe as an builtin cmd.exe command)
Great suggestion. I'll revise my patch by referencing how cmd handles arguments. As for integrating `start` as a built-in command into cmd, I'm not prepared to take that step yet :) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10008#note_128607
participants (3)
-
eric pouech (＠epo) -
Yeshun Ye (＠yeyeshun)
-
YeshunYe