[PATCH v2 0/1] MR352: cmd: Don't loop infinitely on NUL.
-- v2: cmd: Don't loop infinitely on NUL. https://gitlab.winehq.org/wine/wine/-/merge_requests/352
From: Arkadiusz Hiler <ahiler(a)codeweavers.com> Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com> Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- programs/cmd/batch.c | 20 ++++++++++++-------- programs/cmd/builtins.c | 5 +++-- programs/cmd/tests/batch.c | 22 ++++++++++++++++++++++ programs/cmd/tests/test_builtins.cmd | 3 +++ programs/cmd/tests/test_builtins.cmd.exp | 2 ++ programs/cmd/wcmd.h | 2 +- 6 files changed, 43 insertions(+), 11 deletions(-) diff --git a/programs/cmd/batch.c b/programs/cmd/batch.c index 9a262c5fec5..df440c7502d 100644 --- a/programs/cmd/batch.c +++ b/programs/cmd/batch.c @@ -234,13 +234,17 @@ WCHAR *WCMD_parameter (WCHAR *s, int n, WCHAR **start, BOOL raw, * Pre: buf has size noChars * 1 <= noChars <= MAXSTRING * Post: buf is filled with at most noChars-1 characters, and gets nul-terminated - buf does not include EOL terminator + * buf does not include EOL terminator * Returns: - * buf on success - * NULL on error or EOF + * the number of chars written into buf (including \0 string termination) + * upon success (> 0) + * 0 on error or EOF + * Note: + * Upon success, and if there's a L'\0' character in the line, this character + * is returned as is. This can be detected when wcslen(buf) < returned value. */ -WCHAR *WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h) +DWORD WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h) { DWORD charsRead; BOOL status; @@ -265,11 +269,11 @@ WCHAR *WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h) status = ReadFile(h, bufA, noChars, &charsRead, NULL); if (!status || charsRead == 0) { heap_free(bufA); - return NULL; + return 0; } /* Find first EOL */ - for (p = bufA; p < (bufA + charsRead); p = CharNextExA(cp, p, 0)) { + for (p = bufA; p < (bufA + charsRead); p = *p ? CharNextExA(cp, p, 0) : p + 1) { if (*p == '\n' || *p == '\r') break; } @@ -282,7 +286,7 @@ WCHAR *WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h) heap_free(bufA); } else { - if (!charsRead) return NULL; + if (!charsRead) return 0; /* Find first EOL */ for (i = 0; i < charsRead; i++) { @@ -297,7 +301,7 @@ WCHAR *WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h) buf[i] = '\0'; - return buf; + return i + 1; } /**************************************************************************** diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index dd3ae5b509b..2f89162cd91 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -2417,9 +2417,10 @@ void WCMD_for (WCHAR *p, CMD_LIST **cmdList) { return; /* FOR loop aborts at first failure here */ } else { + DWORD r; - /* Read line by line until end of file */ - while (WCMD_fgets(buffer, ARRAY_SIZE(buffer), input)) { + /* Read line by line until end of file (but stop if there's a stray NUL character) */ + while ((r = WCMD_fgets(buffer, ARRAY_SIZE(buffer), input)) && r == wcslen(buffer) + 1) { WCMD_parse_line(cmdStart, firstCmd, &cmdEnd, variable[1], buffer, &doExecuted, &forf_skip, forf_eol, forf_delims, forf_tokens); buffer[0] = 0; diff --git a/programs/cmd/tests/batch.c b/programs/cmd/tests/batch.c index 3846693ef2e..ade6587be81 100644 --- a/programs/cmd/tests/batch.c +++ b/programs/cmd/tests/batch.c @@ -455,6 +455,24 @@ static int cmd_available(void) return FALSE; } +void create_nul_test_file(void) +{ + HANDLE file; + DWORD size; + BOOL bres; + char contents[] = "a b c\nd e\0f\ng h i"; + + file = CreateFileA("nul_test_file", GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, + FILE_ATTRIBUTE_NORMAL, NULL); + ok(file != INVALID_HANDLE_VALUE, "CreateFile failed\n"); + if(file == INVALID_HANDLE_VALUE) + return; + + bres = WriteFile(file, contents, ARRAYSIZE(contents), &size, NULL); + ok(bres, "Could not write to file: %lu\n", GetLastError()); + CloseHandle(file); +} + START_TEST(batch) { int argc; @@ -479,9 +497,13 @@ START_TEST(batch) } shortpath_len = GetShortPathNameA(path, shortpath, ARRAY_SIZE(shortpath)); + create_nul_test_file(); + argc = winetest_get_mainargs(&argv); if(argc > 2) run_from_file(argv[2]); else EnumResourceNamesA(NULL, "TESTCMD", test_enum_proc, 0); + + DeleteFileA("nul_test_file"); } diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index 3f410e55166..3829be50f1a 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -1957,6 +1957,9 @@ FOR /F "delims=. tokens=1*" %%A IN (testfile) DO @echo 5:%%A,%%B FOR /F "delims=. tokens=2*" %%A IN (testfile) DO @echo 6:%%A,%%B FOR /F "delims=. tokens=3*" %%A IN (testfile) DO @echo 7:%%A,%%B del testfile +rem file contains NUL, created by the .exe +for /f %%A in (nul_test_file) DO echo %%A +for /f "tokens=*" %%A in (nul_test_file) DO echo %%A echo ------------ Testing del ------------ echo abc > file diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 8b6e0914112..437ded18000 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -1315,6 +1315,8 @@ h=%h i=b j=c k= l= m=%m n=%n o=%o(a)or_broken@h=%h i=b j=c k= l= m= n=%n o=%o 4:3.14,%B 5:3,14 6:14, +a +a b c ------------ Testing del ------------ deleting 'file' errorlevel is 0, good diff --git a/programs/cmd/wcmd.h b/programs/cmd/wcmd.h index 234c253b49a..a083e2f4939 100644 --- a/programs/cmd/wcmd.h +++ b/programs/cmd/wcmd.h @@ -106,7 +106,7 @@ void WCMD_version (void); int WCMD_volume (BOOL set_label, const WCHAR *args); void WCMD_mklink(WCHAR *args); -WCHAR *WCMD_fgets (WCHAR *buf, DWORD n, HANDLE stream); +DWORD WCMD_fgets (WCHAR *buf, DWORD n, HANDLE stream); WCHAR *WCMD_parameter (WCHAR *s, int n, WCHAR **start, BOOL raw, BOOL wholecmdline); WCHAR *WCMD_parameter_with_delims (WCHAR *s, int n, WCHAR **start, BOOL raw, BOOL wholecmdline, const WCHAR *delims); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/352
Hi. I've updated the MR with your changes and the sign-off. Took me a while but I think I might have found something requires the UTF16 support, I'll check and follow up on it separately / file a bug. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/352#note_3945
Filed bug for the UTF16: https://bugs.winehq.org/show_bug.cgi?id=53386 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/352#note_4630
This merge request was approved by Arek Hiler. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/352
sorry to get back to this, but looking at the UTF16 bug report likely indicates that this patch (at least what I wrote <g>) doesn't go in the best direction I'd recommand waiting until some more tests are done on patch in https://bugs.winehq.org/show_bug.cgi?id=53386 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/352#note_4646
participants (3)
-
Arek Hiler (＠ivyl) -
Arkadiusz Hiler
-
eric pouech (＠epo)