[PATCH v4 0/2] MR11094: cmd: Add wildcard expansion for type command.
Fixes: https://bugs.winehq.org/show_bug.cgi?id=59816 Signed-off-by: Barath Kannan barathrk11@gmail.com -- v4: cmd: Add wildcard expansion for the type command's arguments. cmd: Add tests for the expansion of wildcards in the type command's arguments. https://gitlab.winehq.org/wine/wine/-/merge_requests/11094
From: Barath Kannan <barathrk11@gmail.com> Signed-off-by: Barath Kannan <barathrk11@gmail.com> --- programs/cmd/tests/test_builtins.bat | 15 +++++++++++---- programs/cmd/tests/test_builtins.bat.exp | 11 +++++++++-- programs/cmd/tests/test_builtins.cmd | 15 +++++++++++---- programs/cmd/tests/test_builtins.cmd.exp | 11 +++++++++-- 4 files changed, 40 insertions(+), 12 deletions(-) diff --git a/programs/cmd/tests/test_builtins.bat b/programs/cmd/tests/test_builtins.bat index 4c3d0e7107e..2b9f2eb5fb1 100644 --- a/programs/cmd/tests/test_builtins.bat +++ b/programs/cmd/tests/test_builtins.bat @@ -99,16 +99,23 @@ call :setError 666 & (start /B /WAIT /d "%FOO_PATH%" cmd /s /c "if /I \"%%cd%%\" rd /q /s foo echo --- success/failure for TYPE command mkdir foo & cd foo +mkdir bar +mkdir spam echo a > fileA echo b > fileB call :setError 666 & (type &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & (type NUL &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & (type i\dont\exist\at\all.txt &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) -call :setError 666 & (type file* i\dont\exist\at\all.txt &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) echo --- -call :setError 666 & (type i\dont\exist\at\all.txt file* &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) -cd .. && rd /q /s foo - +call :setError 666 & (type file* i\dont\exist\at\all.txt &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +call :setError 666 & (type file* idontexistatall.txt &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +call :setError 666 & (type idontexistatall.txt file* &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +call :setError 666 & (type i\dont\exist\at\all.txt file*) +echo FAILURE !errorlevel! +cd .. +del foo\file* +call :setError 666 & (type foo\* &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +rd /q /s foo echo --- success/failure for COPY command mkdir foo & cd foo echo a > fileA diff --git a/programs/cmd/tests/test_builtins.bat.exp b/programs/cmd/tests/test_builtins.bat.exp index 36e14d5be91..0738a87f6b8 100644 --- a/programs/cmd/tests/test_builtins.bat.exp +++ b/programs/cmd/tests/test_builtins.bat.exp @@ -67,11 +67,18 @@ SUCCESS 1024 FAILURE 1 SUCCESS 0 FAILURE 1 +--- @todo_wine@a@space@ @todo_wine@b@space@ -@todo_wine@FAILURE 1 -@todo_wine@--- FAILURE 1 +@todo_wine@a@space@ +@todo_wine@b@space@ +@todo_wine@FAILURE 1 +@todo_wine@a@space@ +@todo_wine@b@space@ +@todo_wine@FAILURE 1 +@todo_wine@FAILURE 1 +@todo_wine@FAILURE 1 --- success/failure for COPY command FAILURE 1 SUCCESS 0 diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index f65a301d914..9b659c8c82a 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -682,16 +682,23 @@ call :setError 666 & (start /B /WAIT /d "%FOO_PATH%" cmd /s /c "if /I \"%%cd%%\" rd /q /s foo echo --- success/failure for TYPE command mkdir foo & cd foo +mkdir bar +mkdir spam echo a > fileA echo b > fileB call :setError 666 & (type &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & (type NUL &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & (type i\dont\exist\at\all.txt &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) -call :setError 666 & (type file* i\dont\exist\at\all.txt &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) echo --- -call :setError 666 & (type i\dont\exist\at\all.txt file* &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) -cd .. && rd /q /s foo - +call :setError 666 & (type file* i\dont\exist\at\all.txt &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +call :setError 666 & (type file* idontexistatall.txt &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +call :setError 666 & (type idontexistatall.txt file* &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +call :setError 666 & (type i\dont\exist\at\all.txt file*) +echo FAILURE !errorlevel! +cd .. +del foo\file* +call :setError 666 & (type foo\* &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +rd /q /s foo echo --- success/failure for COPY command mkdir foo & cd foo echo a > fileA diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 0f6d9bdb494..79defed5cdb 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -586,11 +586,18 @@ SUCCESS 1024 FAILURE 1 SUCCESS 0 FAILURE 1 +--- @todo_wine@a@space@ @todo_wine@b@space@ -@todo_wine@FAILURE 1 -@todo_wine@--- FAILURE 1 +@todo_wine@a@space@ +@todo_wine@b@space@ +@todo_wine@FAILURE 1 +@todo_wine@a@space@ +@todo_wine@b@space@ +@todo_wine@FAILURE 1 +@todo_wine@FAILURE 1 +@todo_wine@FAILURE 1 --- success/failure for COPY command FAILURE 1 SUCCESS 0 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11094
From: Barath Kannan <barathrk11@gmail.com> Signed-off-by: Barath Kannan <barathrk11@gmail.com> --- programs/cmd/builtins.c | 136 +++++++++++++++++++++-- programs/cmd/tests/test_builtins.bat.exp | 20 ++-- programs/cmd/tests/test_builtins.cmd.exp | 20 ++-- 3 files changed, 144 insertions(+), 32 deletions(-) diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index 1c9074b75d3..eb0ae15633e 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -30,6 +30,7 @@ #include "wcmd.h" #include <shellapi.h> +#include <shlwapi.h> #include "winternl.h" #include "winioctl.h" #include "ddk/ntifs.h" @@ -3458,22 +3459,133 @@ RETURN_CODE WCMD_type(WCHAR *args) if (!argN) break; WINE_TRACE("type: Processing arg '%s'\n", wine_dbgstr_w(thisArg)); - hIn = CreateFileW(thisArg, GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, NULL, OPEN_EXISTING, - FILE_ATTRIBUTE_NORMAL, NULL); - if (hIn == INVALID_HANDLE_VALUE) { - WCMD_print_error (); - WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), thisArg); - return errorlevel = ERROR_INVALID_FUNCTION; - } - hOut = GetStdHandle(STD_OUTPUT_HANDLE); - if (writeHeaders) { - WCMD_output_stderr(L"\n%1\n\n\n", thisArg); + if (wcspbrk(thisArg, L"*?")) + { + BOOL foundOnlyDirectories = TRUE; + WIN32_FIND_DATAW fd; + HANDLE hff = INVALID_HANDLE_VALUE; + WCHAR *fileNamePart; + DWORD till_file_name_part_len; + DWORD file_name_len; + + hff = FindFirstFileW(thisArg, &fd); + + if (hff == INVALID_HANDLE_VALUE) { + return_code = ERROR_INVALID_FUNCTION; + WCMD_print_error (); + WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), thisArg); + continue; + } + + writeHeaders = TRUE; + + do { + WCHAR srcpath[MAX_PATH]; + + if(fd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) { + WINE_TRACE("Skipping directories\n"); + continue; + } + + foundOnlyDirectories = FALSE; + + fileNamePart = PathFindFileNameW(thisArg); + + /* Calculate length till fileNamePart */ + till_file_name_part_len = wcslen(thisArg) - wcslen(fileNamePart); + file_name_len = wcslen(fd.cFileName); + + if ((till_file_name_part_len + file_name_len) > MAX_PATH) { + return_code = ERROR_INVALID_FUNCTION; + WCMD_print_error (); + WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), thisArg); + + FindClose(hff); + continue; + } + + lstrcpyW(srcpath, thisArg); + lstrcpyW(srcpath + till_file_name_part_len, fd.cFileName); + + hIn = CreateFileW(srcpath, GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, NULL, OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL, NULL); + + if (hIn == INVALID_HANDLE_VALUE) { + DWORD is_dir_error; + + /* Store GetLastError right after CreateFileW so that it isn't overwritten */ + is_dir_error = GetLastError(); + + return_code = ERROR_INVALID_FUNCTION; + WCMD_print_error (); + WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), srcpath); + + FindClose(hff); + + /* Invalid directory path; Return immediately */ + if(is_dir_error == ERROR_PATH_NOT_FOUND) { + return errorlevel = ERROR_INVALID_FUNCTION; + } + + continue; + } + + hOut = GetStdHandle(STD_OUTPUT_HANDLE); + + if (writeHeaders) { + WCMD_output_stderr(L"\n%1\n\n\n", srcpath); + } + + WCMD_copy_loop(hIn, hOut, GetConsoleMode(hIn, &console_mode) || GetConsoleMode(hOut, &console_mode)); + + CloseHandle (hIn); + + } while (FindNextFileW(hff, &fd) != 0); + + FindClose (hff); + + if (foundOnlyDirectories) { + return_code = ERROR_INVALID_FUNCTION; + WCMD_print_error (); + WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), thisArg); + + errorlevel = ERROR_INVALID_FUNCTION; + } } + else + { + hIn = CreateFileW(thisArg, GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, NULL, OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL, NULL); + + if (hIn == INVALID_HANDLE_VALUE) { + DWORD is_dir_error; + + /* Store GetLastError right after CreateFileW so that it isn't overwritten */ + is_dir_error = GetLastError(); + + return_code = ERROR_INVALID_FUNCTION; + WCMD_print_error (); + WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), thisArg); + + /* Invalid directory path; Return immediately */ + if(is_dir_error == ERROR_PATH_NOT_FOUND) { + return errorlevel = ERROR_INVALID_FUNCTION; + } - WCMD_copy_loop(hIn, hOut, GetConsoleMode(hIn, &console_mode) || GetConsoleMode(hOut, &console_mode)); + continue; + } - CloseHandle (hIn); + hOut = GetStdHandle(STD_OUTPUT_HANDLE); + + if (writeHeaders) { + WCMD_output_stderr(L"\n%1\n\n\n", thisArg); + } + + WCMD_copy_loop(hIn, hOut, GetConsoleMode(hIn, &console_mode) || GetConsoleMode(hOut, &console_mode)); + + CloseHandle (hIn); + } } return errorlevel = return_code; diff --git a/programs/cmd/tests/test_builtins.bat.exp b/programs/cmd/tests/test_builtins.bat.exp index 0738a87f6b8..77ca71c8778 100644 --- a/programs/cmd/tests/test_builtins.bat.exp +++ b/programs/cmd/tests/test_builtins.bat.exp @@ -68,17 +68,17 @@ FAILURE 1 SUCCESS 0 FAILURE 1 --- -@todo_wine@a@space@ -@todo_wine@b@space@ +a@space@ +b@space@ +FAILURE 1 +a@space@ +b@space@ +FAILURE 1 +a@space@ +b@space@ +FAILURE 1 +FAILURE 1 FAILURE 1 -@todo_wine@a@space@ -@todo_wine@b@space@ -@todo_wine@FAILURE 1 -@todo_wine@a@space@ -@todo_wine@b@space@ -@todo_wine@FAILURE 1 -@todo_wine@FAILURE 1 -@todo_wine@FAILURE 1 --- success/failure for COPY command FAILURE 1 SUCCESS 0 diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 79defed5cdb..dc6420e1432 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -587,17 +587,17 @@ FAILURE 1 SUCCESS 0 FAILURE 1 --- -@todo_wine@a@space@ -@todo_wine@b@space@ +a@space@ +b@space@ +FAILURE 1 +a@space@ +b@space@ +FAILURE 1 +a@space@ +b@space@ +FAILURE 1 +FAILURE 1 FAILURE 1 -@todo_wine@a@space@ -@todo_wine@b@space@ -@todo_wine@FAILURE 1 -@todo_wine@a@space@ -@todo_wine@b@space@ -@todo_wine@FAILURE 1 -@todo_wine@FAILURE 1 -@todo_wine@FAILURE 1 --- success/failure for COPY command FAILURE 1 SUCCESS 0 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11094
```c if (hIn == INVALID_HANDLE_VALUE) { DWORD is_dir_error; /* Store GetLastError right after CreateFileW so that it isn't overwritten */ is_dir_error = GetLastError(); return_code = ERROR_INVALID_FUNCTION; WCMD_print_error (); WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), srcpath); FindClose(hff); /* Invalid directory path; Return immediately */ if(is_dir_error == ERROR_PATH_NOT_FOUND) { return errorlevel = ERROR_INVALID_FUNCTION; } continue; } ``` A small note on the above code snippet. Calling `GetLastError` in the if like `if (GetLastError() == ERROR_PATH_NOT_FOUND)` succeeded when I tried the test scripts manually, but failed while running the test, because GetLastError at that point was returning `ERROR_INVALID_HANDLE`. Thus, I store the results of a GetLastError made right after the CreateFileW call in a variable. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11094#note_143662
On Fri Jun 19 16:04:51 2026 +0000, Barath Kannan wrote:
changed this line in [version 4 of the diff](/wine/wine/-/merge_requests/11094/diffs?diff_id=276441&start_sha=03325336d8b933f22bb488436d047fb0e5316362#5da6ff77f01131f15f0584e3da4bfbcdb6790735_3509_3508) As discussed, the less than `MAX_PATH` checking is not done for `thisArg` but only after wildcard substitution.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11094#note_144344
On Wed Jun 10 17:44:30 2026 +0000, Barath Kannan wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/11094/diffs?diff_id=274117&start_sha=b4d9df4ec33eca0f9fe3346e221d78f182ef92d1#5da6ff77f01131f15f0584e3da4bfbcdb6790735_3503_3533) Fixed by adding `continue` statement where necessary, instead of bluntly returning.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11094#note_144345
On Fri Jun 19 16:04:52 2026 +0000, Barath Kannan wrote:
changed this line in [version 4 of the diff](/wine/wine/-/merge_requests/11094/diffs?diff_id=276441&start_sha=03325336d8b933f22bb488436d047fb0e5316362#5da6ff77f01131f15f0584e3da4bfbcdb6790735_3563_3572) Fixed it in the way mentioned in the previous comment.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11094#note_144346
On Sat Jun 27 14:14:29 2026 +0000, eric pouech wrote:
I'm not sure this is fully correct from your explanations, you want to distinguish from directory that doesn't exist from error when opening the file (in an existing directory) testing for '\\' doesn't seem fully correct could you discriminate based on GetLastError() value? it may return ERROR_PATH_NOT_FOUND when directory doesn't exist (and some other errors FILE_NOT_FOUND is file doesn't exist whilst directory exists, and other errors (access denied...)) I have applied the following change in both places where the checking for `\` happens:
```c if (hIn == INVALID_HANDLE_VALUE) { DWORD is_dir_error; /* Store GetLastError right after CreateFileW so that it isn't overwritten */ is_dir_error = GetLastError(); return_code = ERROR_INVALID_FUNCTION; WCMD_print_error (); WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), srcpath); FindClose(hff); /* Invalid directory path; Return immediately */ if(is_dir_error == ERROR_PATH_NOT_FOUND) { return errorlevel = ERROR_INVALID_FUNCTION; } continue; } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11094#note_144347
You had asked me to reflect on the code carefully and had subsequently mentioned that there are 2 potential buffer overflows in the code. Are you referring to this part of the code? ```c /* Calculate length till fileNamePart */ till_file_name_part_len = wcslen(thisArg) - wcslen(fileNamePart); file_name_len = wcslen(fd.cFileName); if ((till_file_name_part_len + file_name_len) > MAX_PATH) { return_code = ERROR_INVALID_FUNCTION; WCMD_print_error (); WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), thisArg); FindClose(hff); continue; } lstrcpyW(srcpath, thisArg); lstrcpyW(srcpath + till_file_name_part_len, fd.cFileName); ``` 1. Taking into consideration the first `lstrcpyW` , say the command is `type *abcd...` where the argument is 261 characters long, including the asterisk, but the file itself is just `abcd...` which is 260 characters long, I suppose the check would pass but the 261 characters of `thisArg` would be copied into the buffer blindly. So, the right way to do it should be to copy `thisArg` into `srcpath` upto `till_file_name_part_len` . 2. One other place which could be changed is the if condition. Should the wchar string that is passed to `CreateFileW` be null terminated? In that case, the condition should be `>= MAX_PATH` , limiting the expanded argument to 259 wchars + a null terminator. 3. Another (smaller) issue is that `wcslen(thisArg)` must be called and stored before the do-while loop as right now it is being called every iteration without reason, for calculating `file_name_part_len`. I shall iron out these kinks and push again, although I require some advice about point 2. Looking through the code, I couldn't find the other potential buffer overflow that you mentioned. Could you point me to the right spot, in case I missed something? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11094#note_144350
are you using AI to generate the MR? if not, 1 & 2 are correct (which makes 2 two potential buffer overflows) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11094#note_144369
are you using AI to generate the MR?
OK, not going to lie, this literally saddens me lol. One of the reasons I am trying to contribute to wine is particularly its no AI policy because I don't like the necessary usage of AI "for productivity" in my day job(I am interning at a web dev company right now). Maybe you have reason to think that considering the prosaic nature of my comments in the MR. I try to do things, but sometimes miss out on some things, then I go back and try doing them again after getting external input(in this case you). There isn't any AI involved in any code I have written for this MR. But I can't really blame you for thinking that I used AI, considering the age we live in I guess. TLDR; I haven't used AI now, won't use AI ever for my OSS contributions. I''ll make the necessary changes and push them. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11094#note_144370
participants (3)
-
Barath Kannan -
Barath Kannan (＠barath_kannan)
-
eric pouech (＠epo)