[PATCH 0/2] MR10629: cmd: Add additional bounds checking in search_command.
From: Spencer Wallace <spencerwallace@esri.com> --- programs/cmd/tests/test_cmdline.cmd | 23 +++++++++++++++++++++++ programs/cmd/tests/test_cmdline.cmd.exp | 8 ++++++++ 2 files changed, 31 insertions(+) diff --git a/programs/cmd/tests/test_cmdline.cmd b/programs/cmd/tests/test_cmdline.cmd index 8f3143d0700..1c657b2ef2f 100644 --- a/programs/cmd/tests/test_cmdline.cmd +++ b/programs/cmd/tests/test_cmdline.cmd @@ -52,6 +52,29 @@ echo No prompts or I would not get here1 rem - Try cmd.exe /k as well cmd.exe /k "copy file1 file2 >nul && exit" echo No prompts or I would not get here2 + +echo --- Test 19 +rem test cmd.exe /c with absolute path to executable containing a space, exceeding MAX_PATH +cmd.exe /c "Z:\foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoo bar.bat" >nul 2>nul +echo errorlevel: %ERRORLEVEL% + +echo --- Test 20 +rem test cmd.exe /c with relative path to executable containing a space, exceeding MAX_PATH +cmd.exe /c "foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoo bar.bat" >nul 2>nul +echo errorlevel: %ERRORLEVEL% + +echo --- Test 21 +rem test cmd.exe /c with absolute path including long directory + executable containing a space, exceeding MAX_PATH +rem crashes, returns 0xc0000005 +rem cmd.exe /c "Z:\foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar\foo bar.bat" >nul 2>nul +echo errorlevel: %ERRORLEVEL% + +echo --- Test 22 +rem test cmd.exe /c with relative path including long directory + executable containing a space, exceeding MAX_PATH +rem crashes, returns 0xc0000005 +rem cmd.exe /c "foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar\foo bar.bat" >nul 2>nul +echo errorlevel: %ERRORLEVEL% + rem Directories are ignored when searching for executable files mkdir cmd.exe cmd.exe /c echo alabaster diff --git a/programs/cmd/tests/test_cmdline.cmd.exp b/programs/cmd/tests/test_cmdline.cmd.exp index cb9d306b487..a00041bd430 100644 --- a/programs/cmd/tests/test_cmdline.cmd.exp +++ b/programs/cmd/tests/test_cmdline.cmd.exp @@ -32,6 +32,14 @@ No whitespace --- Test 18 No prompts or I would not get here1 No prompts or I would not get here2 +--- Test 19 +errorlevel: 1 +--- Test 20 +errorlevel: 1 +--- Test 21 +errorlevel: 1 +--- Test 22 +errorlevel: 1 alabaster chrome %hello1% -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10629
From: Spencer Wallace <spencerwallace@esri.com> --- programs/cmd/tests/test_cmdline.cmd | 6 ++---- programs/cmd/wcmdmain.c | 19 ++++++++++++++++--- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/programs/cmd/tests/test_cmdline.cmd b/programs/cmd/tests/test_cmdline.cmd index 1c657b2ef2f..10fc46bf60b 100644 --- a/programs/cmd/tests/test_cmdline.cmd +++ b/programs/cmd/tests/test_cmdline.cmd @@ -65,14 +65,12 @@ echo errorlevel: %ERRORLEVEL% echo --- Test 21 rem test cmd.exe /c with absolute path including long directory + executable containing a space, exceeding MAX_PATH -rem crashes, returns 0xc0000005 -rem cmd.exe /c "Z:\foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar\foo bar.bat" >nul 2>nul +cmd.exe /c "Z:\foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar\foo bar.bat" >nul 2>nul echo errorlevel: %ERRORLEVEL% echo --- Test 22 rem test cmd.exe /c with relative path including long directory + executable containing a space, exceeding MAX_PATH -rem crashes, returns 0xc0000005 -rem cmd.exe /c "foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar\foo bar.bat" >nul 2>nul +cmd.exe /c "foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar\foo bar.bat" >nul 2>nul echo errorlevel: %ERRORLEVEL% rem Directories are ignored when searching for executable files diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index 4aec2e882e9..16fb900f917 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -2009,7 +2009,7 @@ static RETURN_CODE search_command(WCHAR *command, struct search_command *sc, BOO if (len == 0 || len >= ARRAY_SIZE(pathtosearch) - 2) wcscpy(pathtosearch, L"."); sc->has_extension = wcschr(firstParam, L'.') != NULL; - if (wcslen(firstParam) >= MAX_PATH) + if (wcslen(firstParam) >= ARRAY_SIZE(stemofsearch)) { WCMD_output_asis_stderr(WCMD_LoadMessage(WCMD_LINETOOLONG)); return ERROR_INVALID_FUNCTION; @@ -2020,12 +2020,19 @@ static RETURN_CODE search_command(WCHAR *command, struct search_command *sc, BOO } else { + WCHAR* stem; /* Convert eg. ..\fred to include a directory by removing file part */ if (!WCMD_get_fullpath(firstParam, ARRAY_SIZE(pathtosearch), pathtosearch, NULL)) return ERROR_INVALID_FUNCTION; lastSlash = wcsrchr(pathtosearch, L'\\'); - sc->has_extension = wcschr(lastSlash ? lastSlash + 1 : firstParam, L'.') != NULL; - wcscpy(stemofsearch, lastSlash ? lastSlash + 1 : firstParam); + stem = lastSlash ? lastSlash + 1 : firstParam; + if (wcslen(stem) >= ARRAY_SIZE(stemofsearch)) + { + WCMD_output_asis_stderr(WCMD_LoadMessage(WCMD_LINETOOLONG)); + return ERROR_INVALID_FUNCTION; + } + sc->has_extension = wcschr(stem, L'.') != NULL; + wcscpy(stemofsearch, stem); /* Reduce pathtosearch to a path with trailing '\' to support c:\a.bat and c:\windows\a.bat syntax */ @@ -2048,6 +2055,8 @@ static RETURN_CODE search_command(WCHAR *command, struct search_command *sc, BOO if (sc->has_path) { + if (wcslen(pathposn) >= ARRAY_SIZE(sc->path)) + return ERROR_INVALID_FUNCTION; wcscpy(sc->path, pathposn); pathposn = NULL; } @@ -2064,12 +2073,16 @@ static RETURN_CODE search_command(WCHAR *command, struct search_command *sc, BOO if (*pos) /* Reached semicolon */ { + if ((pos - pathposn) >= ARRAY_SIZE(sc->path)) + return ERROR_INVALID_FUNCTION; memcpy(sc->path, pathposn, (pos-pathposn) * sizeof(WCHAR)); sc->path[(pos-pathposn)] = 0x00; pathposn = pos+1; } else /* Reached string end */ { + if (wcslen(pathposn) >= ARRAY_SIZE(sc->path)) + return ERROR_INVALID_FUNCTION; wcscpy(sc->path, pathposn); pathposn = NULL; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10629
eric pouech (@epo) commented about programs/cmd/wcmdmain.c:
} else { + WCHAR* stem; /* Convert eg. ..\fred to include a directory by removing file part */ if (!WCMD_get_fullpath(firstParam, ARRAY_SIZE(pathtosearch), pathtosearch, NULL)) return ERROR_INVALID_FUNCTION; lastSlash = wcsrchr(pathtosearch, L'\\'); - sc->has_extension = wcschr(lastSlash ? lastSlash + 1 : firstParam, L'.') != NULL; - wcscpy(stemofsearch, lastSlash ? lastSlash + 1 : firstParam); + stem = lastSlash ? lastSlash + 1 : firstParam; + if (wcslen(stem) >= ARRAY_SIZE(stemofsearch)) + { + WCMD_output_asis_stderr(WCMD_LoadMessage(WCMD_LINETOOLONG));
this isn't what native does: * line too long is when input overflows the MAXSTRING limit, not the max_path * and the error message printed is about not being able to launch command which is currently handled in callers to this function) so just returning INVALID_FUNCTION should be fine -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10629#note_135860
thanks the MR the fixes look overall ok (except some error handling nitpick) the tests shall be reworked: * these tests should be moved into test_builtind.cmd (and its clone in test_builtind.bat); in the "echo --- success/failure for basics" section * you shall not call cmd recursively (and the drawback of it is that the ERRORLEVEL you test at the end is the one of the crash in cmd.exe subprocess, not the result of a too long path, which is AFAICS 9009 (RETURN_CODE_CANT_LAUNCH) (I guess you did it thay way so that the first tests could run) * this is a (rare) case when you shall squash test + fixes in a single commit (as tests could either crash or corrupt stack with side effects) * do you actually need the 4 instances of the tests? AFAICS the changes only concern the length of the strings, and the presence of space (or not) looks orthogonal to the issue you're targetting; a single instance with a relative path \> MAX_PATH (this avoids having the test relying on a drive that you're not sure it exists -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10629#note_135862
eric pouech (@epo) commented about programs/cmd/wcmdmain.c:
if (len == 0 || len >= ARRAY_SIZE(pathtosearch) - 2) wcscpy(pathtosearch, L"."); sc->has_extension = wcschr(firstParam, L'.') != NULL; - if (wcslen(firstParam) >= MAX_PATH) + if (wcslen(firstParam) >= ARRAY_SIZE(stemofsearch)) { WCMD_output_asis_stderr(WCMD_LoadMessage(WCMD_LINETOOLONG));
(see next comment), but I guess you cloned that part which is also wrong -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10629#note_135861
participants (3)
-
eric pouech (＠epo) -
Spencer Wallace
-
Spencer Wallace (＠spencerwallace)