This is part XX of cmd engine rewrite.
It provides: - a couple of more internal cleanups, - avoids leaks on some error paths, - fixes a couple of redirection related issues
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- programs/cmd/wcmd.h | 7 +---- programs/cmd/wcmdmain.c | 57 +++++++++++++++++------------------------ 2 files changed, 24 insertions(+), 40 deletions(-)
diff --git a/programs/cmd/wcmd.h b/programs/cmd/wcmd.h index 6c25b86f7d1..8e870993b06 100644 --- a/programs/cmd/wcmd.h +++ b/programs/cmd/wcmd.h @@ -108,18 +108,13 @@ typedef struct _CMD_FOR_CONTROL }; } CMD_FOR_CONTROL;
-typedef struct _CMD_COMMAND -{ - WCHAR *command; /* Command string to execute */ -} CMD_COMMAND; - typedef struct _CMD_NODE { CMD_OPERATOR op; /* operator */ CMD_REDIRECTION *redirects; /* Redirections */ union { - CMD_COMMAND *command; /* CMD_SINGLE */ + WCHAR *command; /* CMD_SINGLE */ struct /* binary operator (CMD_CONCAT, ONFAILURE, ONSUCCESS, PIPE) */ { struct _CMD_NODE *left; diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index d44a505dc47..66ae1c145d9 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -986,23 +986,12 @@ static const char *debugstr_redirection(const CMD_REDIRECTION *redir) } }
-static CMD_COMMAND *command_create(const WCHAR *ptr, size_t len) +static WCHAR *command_create(const WCHAR *ptr, size_t len) { - CMD_COMMAND *ret = xalloc(sizeof(CMD_COMMAND)); - - ret->command = xalloc((len + 1) * sizeof(WCHAR)); - memcpy(ret->command, ptr, len * sizeof(WCHAR)); - ret->command[len] = L'\0'; - return ret; -} - -static void command_dispose(CMD_COMMAND *cmd) -{ - if (cmd) - { - free(cmd->command); - free(cmd); - } + WCHAR *command = xalloc((len + 1) * sizeof(WCHAR)); + memcpy(command, ptr, len * sizeof(WCHAR)); + command[len] = L'\0'; + return command; }
static void for_control_dispose(CMD_FOR_CONTROL *for_ctrl) @@ -1263,7 +1252,7 @@ void node_dispose_tree(CMD_NODE *node) switch (node->op) { case CMD_SINGLE: - command_dispose(node->command); + free(node->command); break; case CMD_CONCAT: case CMD_PIPE: @@ -1286,7 +1275,7 @@ void node_dispose_tree(CMD_NODE *node) free(node); }
-static CMD_NODE *node_create_single(CMD_COMMAND *c) +static CMD_NODE *node_create_single(WCHAR *c) { CMD_NODE *new = xalloc(sizeof(CMD_NODE));
@@ -2198,7 +2187,7 @@ syntax_error: /* used to store additional information dedicated a given token */ union token_parameter { - CMD_COMMAND *command; + WCHAR *command; CMD_REDIRECTION *redirection; void *none; }; @@ -2228,7 +2217,7 @@ static const char* debugstr_token(enum builder_token tkn, union token_parameter if (tkn >= ARRAY_SIZE(tokens)) return "<<<>>>"; switch (tkn) { - case TKN_COMMAND: return wine_dbg_sprintf("%s {{%ls}}", tokens[tkn], tkn_pmt.command ? tkn_pmt.command->command : L"<<nul>>"); + case TKN_COMMAND: return wine_dbg_sprintf("%s {{%s}}", tokens[tkn], debugstr_w(tkn_pmt.command)); case TKN_REDIRECTION: return wine_dbg_sprintf("%s {{%s}}", tokens[tkn], debugstr_redirection(tkn_pmt.redirection)); default: return wine_dbg_sprintf("%s", tokens[tkn]); } @@ -2446,15 +2435,15 @@ static BOOL node_builder_parse(struct node_builder *builder, unsigned precedence node_builder_consume(builder); tkn = node_builder_peek_next_token(builder, &pmt); ERROR_IF(tkn != TKN_COMMAND); - if (!wcscmp(pmt.command->command, L"/?")) + if (!wcscmp(pmt.command, L"/?")) { node_builder_consume(builder); - command_dispose(pmt.command); + free(pmt.command); left = node_create_single(command_create(L"help if", 7)); break; } - ERROR_IF(!if_condition_parse(pmt.command->command, &end, &cond)); - command_dispose(pmt.command); + ERROR_IF(!if_condition_parse(pmt.command, &end, &cond)); + free(pmt.command); node_builder_consume(builder); if (!node_builder_parse(builder, 0, &then_block)) { @@ -2487,16 +2476,16 @@ static BOOL node_builder_parse(struct node_builder *builder, unsigned precedence node_builder_consume(builder); tkn = node_builder_peek_next_token(builder, &pmt); ERROR_IF(tkn != TKN_COMMAND); - if (!wcscmp(pmt.command->command, L"/?")) + if (!wcscmp(pmt.command, L"/?")) { node_builder_consume(builder); - command_dispose(pmt.command); + free(pmt.command); left = node_create_single(command_create(L"help for", 8)); break; } node_builder_consume(builder); - for_ctrl = for_control_parse(pmt.command->command); - command_dispose(pmt.command); + for_ctrl = for_control_parse(pmt.command); + free(pmt.command); ERROR_IF(for_ctrl == NULL); ERROR_IF(!node_builder_expect_token(builder, TKN_IN)); ERROR_IF(!node_builder_expect_token(builder, TKN_OPENPAR)); @@ -2506,8 +2495,8 @@ static BOOL node_builder_parse(struct node_builder *builder, unsigned precedence switch (tkn) { case TKN_COMMAND: - for_control_append_set(for_ctrl, pmt.command->command); - command_dispose(pmt.command); + for_control_append_set(for_ctrl, pmt.command); + free(pmt.command); break; case TKN_EOL: case TKN_CLOSEPAR: @@ -2569,7 +2558,7 @@ static BOOL node_builder_generate(struct node_builder *builder, CMD_NODE **node) switch (tkn) { case TKN_COMMAND: - tknstr = tkn_pmt.command->command; + tknstr = tkn_pmt.command; break; case TKN_EOF: tknstr = WCMD_LoadMessage(WCMD_ENDOFFILE); @@ -2608,7 +2597,7 @@ static BOOL node_builder_generate(struct node_builder *builder, CMD_NODE **node) { tkn = node_builder_peek_next_token(builder, &tkn_pmt); if (tkn == TKN_EOF) break; - if (tkn == TKN_COMMAND) command_dispose(tkn_pmt.command); + if (tkn == TKN_COMMAND) free(tkn_pmt.command); if (tkn == TKN_REDIRECTION) redirection_dispose_list(tkn_pmt.redirection); node_builder_consume(builder); } @@ -3673,8 +3662,8 @@ RETURN_CODE node_execute(CMD_NODE *node) switch (node->op) { case CMD_SINGLE: - if (node->command->command[0] != ':') - return_code = execute_single_command(node->command->command); + if (node->command[0] != ':') + return_code = execute_single_command(node->command); else return_code = NO_ERROR; break; case CMD_CONCAT:
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- programs/cmd/wcmdmain.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index 66ae1c145d9..eee9270582c 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -2316,6 +2316,7 @@ static BOOL node_builder_parse(struct node_builder *builder, unsigned precedence CMD_REDIRECTION *redir = NULL; unsigned bogus_line; CMD_NODE *left = NULL, *right; + CMD_FOR_CONTROL *for_ctrl = NULL; union token_parameter pmt; enum builder_token tkn; BOOL done; @@ -2326,7 +2327,7 @@ static BOOL node_builder_parse(struct node_builder *builder, unsigned precedence tkn = node_builder_peek_next_token(builder, &pmt); done = FALSE;
- TRACE("\t%u/%u) %s", builder->pos, builder->num, debugstr_token(tkn, pmt)); + TRACE("\t%u/%u) %s\n", builder->pos, builder->num, debugstr_token(tkn, pmt)); switch (tkn) { case TKN_EOF: @@ -2470,7 +2471,6 @@ static BOOL node_builder_parse(struct node_builder *builder, unsigned precedence ERROR_IF(left); ERROR_IF(redir); { - CMD_FOR_CONTROL *for_ctrl; CMD_NODE *do_block;
node_builder_consume(builder); @@ -2506,13 +2506,10 @@ static BOOL node_builder_parse(struct node_builder *builder, unsigned precedence } node_builder_consume(builder); } while (tkn != TKN_CLOSEPAR); - if (!node_builder_expect_token(builder, TKN_DO) || - !node_builder_parse(builder, 0, &do_block)) - { - for_control_dispose(for_ctrl); - ERROR_IF(TRUE); - } + ERROR_IF(!node_builder_expect_token(builder, TKN_DO)); + ERROR_IF(!node_builder_parse(builder, 0, &do_block)); left = node_create_for(for_ctrl, do_block); + for_ctrl = NULL; } break; case TKN_REDIRECTION: @@ -2529,6 +2526,7 @@ error_handling: TRACE("Parser failed at line %u:token %s\n", bogus_line, debugstr_token(tkn, pmt)); node_dispose_tree(left); redirection_dispose_list(redir); + if (for_ctrl) for_control_dispose(for_ctrl);
return FALSE; }
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- programs/cmd/tests/test_builtins.cmd | 14 +++++++++++ programs/cmd/tests/test_builtins.cmd.exp | 6 +++-- programs/cmd/wcmdmain.c | 30 +++++++++++++++--------- 3 files changed, 37 insertions(+), 13 deletions(-)
diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index 66d5e894662..c67fc84988a 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -217,6 +217,20 @@ type C (if 1==0 (echo A) else echo B) > C type C (if 1==0 (echo A > B) else echo C) +echo --- multiredirections +erase /q a b & (echo >a >b) +if exist a echo a shouldn't exist +if not exist b echo b should exist +erase /q a b & (echo >a >>b) +if exist a echo a shouldn't exist +if not exist b echo b should exist +erase /q a b & (echo >a | (echo b > b)) +if not exist a echo a should exist +if not exist b echo b should exist +erase /q a b & (echo cc1 2>a 1>&2 2>b) +if exist a echo a shouldn't exist +if not exist b (echo b should exist) else (echo cc2 & type b) + cd .. & rd /s/q foobar
echo ------------ Testing circumflex escape character ------------ diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index a96c7dde10b..84bcdcdf581 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -202,8 +202,8 @@ fooc@space@ food1 food2 food21 -@todo_wine@foo7@space@@space@@or_broken@not supported@space@ -@todo_wine@foo@or_broken@not supported +foo7@space@@space@@or_broken@not supported@space@ +foo@or_broken@not supported --- redirect at beginning of line foo foo1 @@ -229,6 +229,8 @@ foo A B C +--- multiredirections +cc2@space@ ------------ Testing circumflex escape character ------------ hello, world hello, world diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index eee9270582c..5f55e4c8d9e 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -1649,12 +1649,6 @@ RETURN_CODE WCMD_run_program(WCHAR *command, BOOL called) return ERROR_INVALID_FUNCTION; }
-/* this is obviously wrong... will require more work to be fixed */ -static inline unsigned clamp_fd(unsigned fd) -{ - return fd <= 2 ? fd : 1; -} - static BOOL set_std_redirections(CMD_REDIRECTION *redir) { static DWORD std_index[3] = {STD_INPUT_HANDLE, STD_OUTPUT_HANDLE, STD_ERROR_HANDLE}; @@ -1664,6 +1658,12 @@ static BOOL set_std_redirections(CMD_REDIRECTION *redir)
for (; redir; redir = redir->next) { + CMD_REDIRECTION *next; + + /* if we have several elements changing same std stream, only use last one */ + for (next = redir->next; next; next = next->next) + if (redir->fd == next->fd) break; + if (next) continue; switch (redir->kind) { case REDIR_READ_FROM: @@ -1697,17 +1697,26 @@ static BOOL set_std_redirections(CMD_REDIRECTION *redir) } break; case REDIR_WRITE_CLONE: + if (redir->clone > 2 || redir->clone == redir->fd) + { + WARN("Can't duplicate %d from %d\n", redir->fd, redir->clone); + return FALSE; + } if (!DuplicateHandle(GetCurrentProcess(), - GetStdHandle(std_index[clamp_fd(redir->clone)]), + GetStdHandle(std_index[redir->clone]), GetCurrentProcess(), &h, 0, TRUE, DUPLICATE_SAME_ACCESS)) { WARN("Duplicating handle failed with gle %ld\n", GetLastError()); + return FALSE; } break; } - SetStdHandle(std_index[clamp_fd(redir->fd)], h); + if (redir->fd > 2) + CloseHandle(h); + else + SetStdHandle(std_index[redir->fd], h); } return TRUE; } @@ -3654,10 +3663,9 @@ RETURN_CODE node_execute(CMD_NODE *node) if (!set_std_redirections(node->redirects)) { WCMD_print_error(); - /* FIXME potentially leaking here (if first redir created ok, and second failed */ - return ERROR_INVALID_FUNCTION; + return_code = ERROR_INVALID_FUNCTION; } - switch (node->op) + else switch (node->op) { case CMD_SINGLE: if (node->command[0] != ':')
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=147017
Your paranoid android.
=== debian11b (64 bit WoW report) ===
ddraw: ddraw1.c:3645: Test failed: Expected (0,0)-(640,480), got (-32000,-32000)-(-31840,-31969).
-
Eric Pouech -
eric pouech (＠epo)
-
Marvin