[PATCH 0/1] MR4418: cmd: Delay variable substitution until after the if clause is parsed.
From: Alex Henrie <alexhenrie24(a)gmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55859 --- programs/cmd/builtins.c | 35 +++++++++++++++++++----- programs/cmd/tests/test_builtins.cmd | 5 ++++ programs/cmd/tests/test_builtins.cmd.exp | 1 + programs/cmd/wcmd.h | 1 + programs/cmd/wcmdmain.c | 9 +++--- 5 files changed, 40 insertions(+), 11 deletions(-) diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index ca703af52ec..85870193f55 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -2818,9 +2818,24 @@ static int evaluate_if_comparison(const WCHAR *leftOperand, const WCHAR *operato return -1; } +static void do_delayed_expansion(WCHAR *p) +{ + /* Perform delayed substitution after the if clause is parsed */ + if (!delayedsubst) return; + for (p = wcschr(p, '!'); p; p = wcschr(p, '!')) + WCMD_expand_envvar(p, '!'); +} + +static void expand_first_param(WCHAR *dst, WCHAR *src, int negate) +{ + wcscpy(dst, WCMD_parameter(src, 1 + negate, NULL, FALSE, FALSE)); + do_delayed_expansion(dst); +} + int evaluate_if_condition(WCHAR *p, WCHAR **command, int *test, int *negate) { WCHAR condition[MAX_PATH]; + WCHAR param[MAXSTRING]; int caseInsensitive = (wcsstr(quals, L"/I") != NULL); *negate = !lstrcmpiW(param1,L"not"); @@ -2828,9 +2843,10 @@ int evaluate_if_condition(WCHAR *p, WCHAR **command, int *test, int *negate) WINE_TRACE("Condition: %s\n", wine_dbgstr_w(condition)); if (!lstrcmpiW(condition, L"errorlevel")) { - WCHAR *param = WCMD_parameter(p, 1+(*negate), NULL, FALSE, FALSE); WCHAR *endptr; - long int param_int = wcstol(param, &endptr, 10); + long int param_int; + expand_first_param(param, p, *negate); + param_int = wcstol(param, &endptr, 10); if (*endptr) goto syntax_err; *test = ((long int)errorlevel >= param_int); WCMD_parameter(p, 2+(*negate), command, FALSE, FALSE); @@ -2838,8 +2854,10 @@ int evaluate_if_condition(WCHAR *p, WCHAR **command, int *test, int *negate) else if (!lstrcmpiW(condition, L"exist")) { WIN32_FIND_DATAW fd; HANDLE hff; - WCHAR *param = WCMD_parameter(p, 1+(*negate), NULL, FALSE, FALSE); - int len = lstrlenW(param); + int len; + + expand_first_param(param, p, *negate); + len = wcslen(param); if (!len) { *test = FALSE; @@ -2855,8 +2873,8 @@ int evaluate_if_condition(WCHAR *p, WCHAR **command, int *test, int *negate) WCMD_parameter(p, 2+(*negate), command, FALSE, FALSE); } else if (!lstrcmpiW(condition, L"defined")) { - *test = (GetEnvironmentVariableW(WCMD_parameter(p, 1+(*negate), NULL, FALSE, FALSE), - NULL, 0) > 0); + expand_first_param(param, p, *negate); + *test = (GetEnvironmentVariableW(param, NULL, 0) > 0); WCMD_parameter(p, 2+(*negate), command, FALSE, FALSE); } else { /* comparison operation */ @@ -2883,12 +2901,15 @@ int evaluate_if_condition(WCHAR *p, WCHAR **command, int *test, int *negate) lstrcpyW(rightOperand, WCMD_parameter(p, 0, ¶mStart, TRUE, FALSE)); if (!*rightOperand) goto syntax_err; + p = paramStart + lstrlenW(rightOperand); + + do_delayed_expansion(leftOperand); + do_delayed_expansion(rightOperand); *test = evaluate_if_comparison(leftOperand, operator, rightOperand, caseInsensitive); if (*test == -1) goto syntax_err; - p = paramStart + lstrlenW(rightOperand); WCMD_parameter(p, 0, command, FALSE, FALSE); } diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index 510a1ba5931..b362b16cea0 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -779,6 +779,11 @@ if %WINE_FOO% == foo ( set WINE_FOO=bar if !WINE_FOO! == bar (echo bar) else echo foo ) + +rem if expansion is delayed, spaces in the variable do not cause a syntax error +set WINE_FOO=test kest +if !WINE_FOO!=="" (echo empty) else echo not empty + echo %ErrorLevel% setlocal DisableDelayedExpansion echo %ErrorLevel% diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 0f48a823109..bf05f852419 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -612,6 +612,7 @@ foo foo(a)or_broken@!WINE_FOO! foo bar(a)or_broken@foo +not empty 0 0(a)or_broken@1 foo diff --git a/programs/cmd/wcmd.h b/programs/cmd/wcmd.h index 1935bbcdcd2..ae06e864a5f 100644 --- a/programs/cmd/wcmd.h +++ b/programs/cmd/wcmd.h @@ -69,6 +69,7 @@ void WCMD_echo (const WCHAR *); void WCMD_endlocal (void); void WCMD_enter_paged_mode(const WCHAR *); void WCMD_exit (CMD_LIST **cmdList); +WCHAR* WCMD_expand_envvar(WCHAR *start, WCHAR startchar); void WCMD_for (WCHAR *, CMD_LIST **cmdList); BOOL WCMD_get_fullpath(const WCHAR *, SIZE_T, WCHAR *, WCHAR **); void WCMD_give_help (const WCHAR *args); diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index 843fef8ea50..ba6f61723d2 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -551,7 +551,7 @@ static inline BOOL WCMD_is_magic_envvar(const WCHAR *s, const WCHAR *magicvar) * * Expands environment variables, allowing for WCHARacter substitution */ -static WCHAR *WCMD_expand_envvar(WCHAR *start, WCHAR startchar) +WCHAR *WCMD_expand_envvar(WCHAR *start, WCHAR startchar) { WCHAR *endOfVar = NULL, *s; WCHAR *colonpos = NULL; @@ -1366,9 +1366,10 @@ void WCMD_execute (const WCHAR *command, const WCHAR *redirects, } /* Expand variables in command line mode only (batch mode will - be expanded as the line is read in, except for 'for' loops) */ - handleExpansion(new_cmd, (context != NULL), delayedsubst); - handleExpansion(new_redir, (context != NULL), delayedsubst); + be expanded as the line is read in, except for 'for' loops). + Do delayed substitution now if it is enabled and the command is not IF. */ + handleExpansion(new_cmd, context != NULL, delayedsubst && cmd_index != WCMD_IF); + handleExpansion(new_redir, context != NULL, delayedsubst && cmd_index != WCMD_IF); /* * Changing default drive has to be handled as a special case, anything -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4418
it looks a bit strange that expand_envvar needs to behave differently if inside CMD_IF or not If I read your sample test correctly, the issue in Wine stems from 'if !WINE_FOO!==' being expanded and then parsed again in the if-command leading to a syntax error because of the space introduced in WINE_FOO expansion it rather looks like the delayed expansion is wrong as the !WINE_FOO! should rather be expanded **after** parsing that's likely a way larger change -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4418#note_52658
On Wed Nov 22 03:01:09 2023 +0000, eric pouech wrote:
it looks a bit strange that expand_envvar needs to behave differently if inside CMD_IF or not If I read your sample test correctly, the issue in Wine stems from 'if !WINE_FOO!==' being expanded and then parsed again in the if-command leading to a syntax error because of the space introduced in WINE_FOO expansion it rather looks like the delayed expansion is wrong as the !WINE_FOO! should rather be expanded **after** parsing that's likely a way larger change I think you may have misread the diff: The patch adds an exception for `WCMD_IF` to `WCMD_execute`, not `WCMD_expand_envvar`. `WCMD_expand_envvar` is exactly the same as before, minus the `static` keyword. The call to `WCMD_expand_envvar(p, '!')` is merely delayed until after the `IF` statement is parsed, exactly as you say it should be.
It doesn't surprise me that `IF` has to be treated differently when `WCMD_execute` calls `handleExpansion` because there are many places where `IF` and `FOR` are already special-cased. There's a good chance that we will eventually have to add `&& cmd_index != WCMD_FOR` to the same two lines too. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4418#note_53245
On Wed Nov 22 03:01:09 2023 +0000, Alex Henrie wrote:
I think you may have misread the diff: The patch adds an exception for `WCMD_IF` to `WCMD_execute`, not `WCMD_expand_envvar`. `WCMD_expand_envvar` is exactly the same as before, minus the `static` keyword. The call to `WCMD_expand_envvar(p, '!')` is merely delayed until after the `IF` statement is parsed, exactly as you say it should be. It doesn't surprise me that `IF` has to be treated differently when `WCMD_execute` calls `handleExpansion` because there are many places where `IF` and `FOR` are already special-cased. There's a good chance that we will eventually have to add `&& cmd_index != WCMD_FOR` to the same two lines too. if I get this right:
* 'if foo bar == other' generates a syntax error * 'if %VAR%==other' will get the same syntax error when VAR contains a space * 'if "%VAR%"=="other"' doesn't get syntax error as strings are delimited * but 'if !WINE_FOO!==other' doesn't get syntax error even if WINE_FOO contains a space this looks to me as if: * %VAR% is replaced by its value, and then the command is parsed * while !WINE_FOO! is kept as is, command is parsed, which gives !WINE_FOO! as a token, and which is then evaluated IOW, this looks to me to be only a matter of ordering of operations, and has nothing to do with being inside a IF or not but this requires to have a decent tokenizer (for which when delay expansion is enabled just transforms !VAR! into its value) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4418#note_53342
On Wed Nov 22 14:26:29 2023 +0000, eric pouech wrote:
if I get this right: * 'if foo bar == other' generates a syntax error * 'if %VAR%==other' will get the same syntax error when VAR contains a space * 'if "%VAR%"=="other"' doesn't get syntax error as strings are delimited * but 'if !WINE_FOO!==other' doesn't get syntax error even if WINE_FOO contains a space this looks to me as if: * %VAR% is replaced by its value, and then the command is parsed * while !WINE_FOO! is kept as is, command is parsed, which gives !WINE_FOO! as a token, and which is then evaluated IOW, this looks to me to be only a matter of ordering of operations, and has nothing to do with being inside a IF or not but this requires to have a decent tokenizer (for which when delay expansion is enabled just transforms !VAR! into its value) Your description of those four cases is correct, and Wine handles all four of them correctly with this patch. I'm not understanding what would be different if we had a "decent" tokenizer. Can you give me an example of a statement that would not be evaluated correctly by the code I am proposing, but that would be evaluated correctly by such a tokenizer?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4418#note_53358
On Wed Nov 22 17:14:50 2023 +0000, Alex Henrie wrote:
Your description of those four cases is correct, and Wine handles all four of them correctly with this patch. I'm not understanding what would be different if we had a "decent" tokenizer. Can you give me an example of a statement that would not be evaluated correctly by the code I am proposing, but that would be evaluated correctly by such a tokenizer? my point is more about the way to implement the fix rather if the propose fix passes the tests
cmd as of today is barely maintanable because of small adjustements here and there, added over the years your proposal is such a small adjustment by changing behavior of some helper functions, depending on context, whereas IMO the cause of the issue lies elsewhere -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4418#note_53367
participants (3)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
eric pouech (＠epo)