[PATCH 0/2] MR4504: cmd: fix for bug 55401
Visual Studio's native tool command prompt uses rare for loop variables: %%1, %%2. This test covers this case. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55401 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4504
From: Dmitry Sokolov <mr.dmitry.sokolov(a)gmail.com> Visual Studio's native tool command prompt uses rare for loop variables: %%1, %%2. This test covers this case. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55401 --- programs/cmd/tests/test_builtins.cmd | 47 ++++++++++++++++++++++++ programs/cmd/tests/test_builtins.cmd.exp | 9 +++++ 2 files changed, 56 insertions(+) diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index 510a1ba5931..2a617b2225b 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -1604,6 +1604,53 @@ for %%i in (test) do ( ) echo d4 ) + +echo --- for loop with rare var names +set "WINE_LOG_LEVEL=" +:test_for_loop_params +set "WINE_ARGS= -sdkver=10.0.22000.0 -type=MD" +:test_for_loop_params_parse +for /F "tokens=1,* delims= " %%a in ("%WINE_ARGS%") do ( + for /F "tokens=1,2 delims==" %%1 in ("%%a") do ( + if "%WINE_LOG_LEVEL%" GEQ "2" ( + echo [DEBUG] inner argument {%%1, %%2} + ) + call :test_for_loop_params_inner %%1 %%2 + ) + set "WINE_ARGS=%%b" + goto :test_for_loop_params_parse +) +if not defined WINE_LOG_LEVEL set "WINE_LOG_LEVEL=1" & goto :test_for_loop_params +if "%WINE_LOG_LEVEL%" LSS "2" set "WINE_LOG_LEVEL=2" & goto :test_for_loop_params +goto :test_for_loop_params_end + +:test_for_loop_params_inner +set "__arg_found=" +if /I "%1"=="-sdkver" ( + echo SDKVER=%2 + set "__arg_found=1" +) +if /I "%1"=="-type" ( + echo TYPE=%2 + set "__arg_found=1" +) +if "%__arg_found%" NEQ "1" ( + if "%2"=="" ( + echo [ERROR] Invalid command line argument: '%1' + ) else ( + echo [ERROR] Invalid command line argument: '%1=%2' + ) + set "__arg_found=" + goto :eof +) +set "__arg_found=" +goto :eof + +:test_for_loop_params_end +set "WINE_ARGS=" +set "WINE_LOG_LEVEL=" + + echo --- set /a goto :testseta diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 0f48a823109..6a90f9d7584 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -1065,6 +1065,15 @@ d3 a4 c4 d4 +--- for loop with rare var names +SDKVER=10.0.22000.0 +TYPE=MD +SDKVER=10.0.22000.0 +TYPE=MD +[DEBUG] inner argument {-sdkver, 10.0.22000.0} +SDKVER=10.0.22000.0 +[DEBUG] inner argument {-type, MD} +TYPE=MD --- set /a ------ individual operations WINE_foo correctly 3 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4504
From: Dmitry Sokolov <mr.dmitry.sokolov(a)gmail.com> Visual Studio's native tool command prompt uses rare FOR loop variables: %%1, %%2. This fix allows to use %%1 var in FOR loops. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55401 --- programs/cmd/builtins.c | 34 ++++++++++++++++++++++++++++++---- programs/cmd/wcmd.h | 18 ++++++++++++++---- programs/cmd/wcmdmain.c | 7 ++++--- 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index ca703af52ec..c8700a412a3 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -37,6 +37,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(cmd); extern int defaultColor; extern BOOL echo_mode; extern BOOL interactive; +extern void handleExpansion(WCHAR *cmd, BOOL atExecute, BOOL delayed); struct env_stack *pushd_directories; const WCHAR inbuilt[][10] = { @@ -1914,6 +1915,28 @@ static int WCMD_for_nexttoken(int lasttoken, WCHAR *tokenstr, return nexttoken; } +/************************************************************************** + * WCMD_check_varidx + * + * Checks if the given varidx and varidx+varoffset are in suitable range, + * i.e. (0 <= X <= 25) || (26 <= X <= 51) || (52 <= X <= 61) + * a z A Z 0 9 + * + * Parameters: + * varidx [I] - Var index to check + * varoffset [I] - Var offset to check + * + * Returns TRUE on success, FALSE otherwise + */ +inline BOOL WCMD_check_varidx(int varidx, int varoffset) { + int X = varidx + varoffset; + return (varoffset >= 0) && ( + (0 <= varidx && varidx <= 25 && 0 <= X && X <= 25) || + (26 <= varidx && varidx <= 51 && 26 <= X && X <= 51) || + (52 <= varidx && varidx <= 61 && 52 <= X && X <= 61) + ); +} + /************************************************************************** * WCMD_parse_line * @@ -1983,11 +2006,14 @@ static void WCMD_parse_line(CMD_LIST *cmdStart, /* Empty out variables */ for (varoffset=0; - varidx >= 0 && varoffset<totalfound && (((varidx%26) + varoffset) < 26); + varidx >= 0 && varoffset<totalfound && WCMD_check_varidx(varidx, varoffset); varoffset++) { forloopcontext.variable[varidx + varoffset] = emptyW; } + /* Expand outer loop variables */ + if (wcschr(buffer, '%')) handleExpansion(buffer, (context != NULL), FALSE); + /* Loop extracting the tokens Note: nexttoken of 0 means there were no tokens requested, to handle the special case of tokens=* */ @@ -2003,7 +2029,7 @@ static void WCMD_parse_line(CMD_LIST *cmdStart, if (varidx >=0) { if (parm) forloopcontext.variable[varidx + varoffset] = xstrdupW(parm); varoffset++; - if (((varidx%26)+varoffset) >= 26) break; + if (!WCMD_check_varidx(varidx, varoffset)) break; } /* Find the next token */ @@ -2014,7 +2040,7 @@ static void WCMD_parse_line(CMD_LIST *cmdStart, /* If all the rest of the tokens were requested, and there is still space in the variable range, write them now */ - if (!anyduplicates && starfound && varidx >= 0 && (((varidx%26) + varoffset) < 26)) { + if (!anyduplicates && starfound && varidx >= 0 && WCMD_check_varidx(varidx, varoffset)) { nexttoken++; WCMD_parameter_with_delims(buffer, (nexttoken-1), &parm, FALSE, FALSE, forf_delims); WINE_TRACE("Parsed allremaining tokens (%d) as parameter %s\n", @@ -2215,8 +2241,8 @@ void WCMD_for (WCHAR *p, CMD_LIST **cmdList) { /* Variable should follow */ lstrcpyW(variable, thisArg); - WINE_TRACE("Variable identified as %s\n", wine_dbgstr_w(variable)); varidx = FOR_VAR_IDX(variable[1]); + WINE_TRACE("Variable identified as %s (idx: %d)\n", wine_dbgstr_w(variable), varidx); /* Ensure line continues with IN */ thisArg = WCMD_parameter(p, parameterNo++, NULL, FALSE, FALSE); diff --git a/programs/cmd/wcmd.h b/programs/cmd/wcmd.h index 1935bbcdcd2..53e3883683b 100644 --- a/programs/cmd/wcmd.h +++ b/programs/cmd/wcmd.h @@ -185,12 +185,22 @@ typedef struct _DIRECTORY_STACK /* Data structure to for loop variables during for body execution, bearing in mind that for loops can be nested */ -#define MAX_FOR_VARIABLES 52 -#define FOR_VAR_IDX(c) (((c)>='a'&&(c)<='z')?((c)-'a'):\ - ((c)>='A'&&(c)<='Z')?(26+(c)-'A'):-1) +#define MAX_FOR_VARIABLES 62 +#define FOR_VAR_IDX(c) ( ((c)>='a'&&(c)<='z')\ + ?((c)-'a')\ + :( ((c)>='A'&&(c)<='Z')\ + ?(26+(c)-'A')\ + :( ((c)>='0'&&(c)<='9')\ + ?(52+(c)-'0')\ + :-1 ) ) ) +#define FOR_VAR_NAME(idx) ( (idx)>=52\ + ?('0'+((idx)-52))\ + :( (idx)>=26\ + ?('A'+((idx)-26))\ + :('a'+(idx)) ) ) typedef struct _FOR_CONTEXT { - WCHAR *variable[MAX_FOR_VARIABLES]; /* a-z then A-Z */ + WCHAR *variable[MAX_FOR_VARIABLES]; /* a-z then A-Z then 0-9 */ } FOR_CONTEXT; /* diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index 843fef8ea50..8e7601b863d 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -794,7 +794,7 @@ static WCHAR *WCMD_expand_envvar(WCHAR *start, WCHAR startchar) * rather than at parse time, i.e. delayed expansion and for loops need to be * processed */ -static void handleExpansion(WCHAR *cmd, BOOL atExecute, BOOL delayed) { +void handleExpansion(WCHAR *cmd, BOOL atExecute, BOOL delayed) { /* For commands in a context (batch program): */ /* Expand environment variables in a batch file %{0-9} first */ @@ -813,10 +813,10 @@ static void handleExpansion(WCHAR *cmd, BOOL atExecute, BOOL delayed) { WCHAR *normalp; /* Display the FOR variables in effect */ - for (i=0;i<52;i++) { + for (i=0;i<MAX_FOR_VARIABLES;i++) { if (forloopcontext.variable[i]) { WINE_TRACE("FOR variable context: %c = '%s'\n", - i<26?i+'a':(i-26)+'A', + FOR_VAR_NAME(i), wine_dbgstr_w(forloopcontext.variable[i])); } } @@ -868,6 +868,7 @@ static void handleExpansion(WCHAR *cmd, BOOL atExecute, BOOL delayed) { int forvaridx = FOR_VAR_IDX(*(p+1)); if (startchar == '%' && forvaridx != -1 && forloopcontext.variable[forvaridx]) { /* Replace the 2 characters, % and for variable character */ + WINE_TRACE("FOR variable substitute %%%c with '%s'\n", *(p+1), wine_dbgstr_w(forloopcontext.variable[forvaridx])); WCMD_strsubstW(p, p + 2, forloopcontext.variable[forvaridx], -1); } else if (!atExecute || startchar == '!') { p = WCMD_expand_envvar(p, startchar); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4504
Hi Dmitry, thanks for the proposal some generic comments though: you're in fact tackling two different issues: * %1 as FOR loop variable * FOR variables' expansion in nested FOR loops I think it would be simpler to separate the two (even if the two are needed to fix the mentioned bug). This could be kept in a single MR (4 commits: test for %1 in loop, fix for %1 in loop, test for expansion in nested loops) or spread across two. I'll post also some more detailed comments in the commits. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4504#note_53868
eric pouech (@epo) commented about programs/cmd/tests/test_builtins.cmd:
+:test_for_loop_params_parse +for /F "tokens=1,* delims= " %%a in ("%WINE_ARGS%") do ( + for /F "tokens=1,2 delims==" %%1 in ("%%a") do ( + if "%WINE_LOG_LEVEL%" GEQ "2" ( + echo [DEBUG] inner argument {%%1, %%2} + ) + call :test_for_loop_params_inner %%1 %%2 + ) + set "WINE_ARGS=%%b" + goto :test_for_loop_params_parse +) +if not defined WINE_LOG_LEVEL set "WINE_LOG_LEVEL=1" & goto :test_for_loop_params +if "%WINE_LOG_LEVEL%" LSS "2" set "WINE_LOG_LEVEL=2" & goto :test_for_loop_params +goto :test_for_loop_params_end + +:test_for_loop_params_inner I assume you want to use an external (to the loops) function to check how %1 behave (is properly saved... )
but a simple 'echo %1 %2' would be much simpler (unless I missed something) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4504#note_53872
eric pouech (@epo) commented about programs/cmd/tests/test_builtins.cmd:
) echo d4 ) + +echo --- for loop with rare var names +set "WINE_LOG_LEVEL=" +:test_for_loop_params +set "WINE_ARGS= -sdkver=10.0.22000.0 -type=MD"
please don't use direct samples from proprietary samples, ie "-foo=bar -platform=wine" would be sufficient -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4504#note_53873
eric pouech (@epo) commented about programs/cmd/builtins.c:
return nexttoken; }
+/************************************************************************** I wonder if we'd rather use something like
```plaintext static int get_varidx_upper_boundary(int varidx) { if (varidx < 26) return 26; if (varidx < 52) return 52; return 62; } ``` and rather use ```plaintext varoffset < get_varidx_upper_boundary(varidx) ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4504#note_53875
eric pouech (@epo) commented about programs/cmd/wcmd.h:
/* Data structure to for loop variables during for body execution, bearing in mind that for loops can be nested */ -#define MAX_FOR_VARIABLES 52 -#define FOR_VAR_IDX(c) (((c)>='a'&&(c)<='z')?((c)-'a'):\ - ((c)>='A'&&(c)<='Z')?(26+(c)-'A'):-1) +#define MAX_FOR_VARIABLES 62
could use the opportunity to get rid of macro (and its potential side effects) and use a static inline function instead -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4504#note_53876
participants (3)
-
Dmitry Sokolov -
Dmitry Sokolov (＠DmitrySokolov)
-
eric pouech (＠epo)