From 2b5bf78fafdf027624ca88e1f703bc9e577f4690 Mon Sep 17 00:00:00 2001 From: Matt Coffin Date: Tue, 11 Jun 2019 12:10:17 -0600 Subject: [PATCH] Fix segfaults caused by faulty command parsing This patch fixes faulty command parsing introduced by f0f5de9a9e87ca1f0d74e7cbf82ffceba51ffbe6. When that commit allowed criteria reset on ';' delimeters in commands lists, it failed to account for its inner ','-parsing loop eating threw the entire rest of the string. This patch refactors argsep to use a list of multiple separators, and (optionally) return the separator that it matched against in this iteration via a pointer. This allows it to hint at the command parser which separator was used at the end of the last command, allowing it to trigger a potential secondary read of the criteria. Fixes #4239 --- common/stringop.c | 56 +++++++++++++++------ include/stringop.h | 2 +- sway/commands.c | 113 ++++++++++++++++++++---------------------- sway/tree/workspace.c | 4 +- 4 files changed, 98 insertions(+), 77 deletions(-) diff --git a/common/stringop.c b/common/stringop.c index dea152cc..ac7df296 100644 --- a/common/stringop.c +++ b/common/stringop.c @@ -251,37 +251,61 @@ char *join_args(char **argv, int argc) { return res; } -char *argsep(char **stringp, const char *delim) { +static inline char *argsep_next_interesting(const char *src, const char *delim) { + char *special = strpbrk(src, "\"'\\"); + char *next_delim = strpbrk(src, delim); + if (!special) { + return next_delim; + } + if (!next_delim) { + return special; + } + return (next_delim < special) ? next_delim : special; +} + +char *argsep(char **stringp, const char *delim, char *matched) { char *start = *stringp; char *end = start; bool in_string = false; bool in_char = false; bool escaped = false; - while (1) { - if (*end == '"' && !in_char && !escaped) { + char *interesting = NULL; + + while ((interesting = argsep_next_interesting(end, delim))) { + if (escaped && interesting != end) { + escaped = false; + } + if (*interesting == '"' && !in_char && !escaped) { in_string = !in_string; - } else if (*end == '\'' && !in_string && !escaped) { + end = interesting + 1; + } else if (*interesting == '\'' && !in_string && !escaped) { in_char = !in_char; - } else if (*end == '\\') { + end = interesting + 1; + } else if (*interesting == '\\') { escaped = !escaped; - } else if (*end == '\0') { - *stringp = NULL; - break; - } else if (!in_string && !in_char && !escaped && strchr(delim, *end)) { + end = interesting + 1; + } else if (!in_string && !in_char && !escaped) { + // We must have matched a separator + end = interesting; + if (matched) { + *matched = *end; + } if (end - start) { *(end++) = 0; - *stringp = end + strspn(end, delim);; - if (!**stringp) *stringp = NULL; + *stringp = end; break; } else { - ++start; - end = start; + end = ++start; } + } else { + end++; } - if (*end != '\\') { - escaped = false; + } + if (!interesting) { + *stringp = NULL; + if (matched) { + *matched = '\0'; } - ++end; } return start; } diff --git a/include/stringop.h b/include/stringop.h index 6f920999..2aabcee7 100644 --- a/include/stringop.h +++ b/include/stringop.h @@ -24,6 +24,6 @@ int unescape_string(char *string); char *join_args(char **argv, int argc); // Split string into 2 by delim, handle quotes -char *argsep(char **stringp, const char *delim); +char *argsep(char **stringp, const char *delim, char *matched_delim); #endif diff --git a/sway/commands.c b/sway/commands.c index 377f2d01..a670f813 100644 --- a/sway/commands.c +++ b/sway/commands.c @@ -211,8 +211,8 @@ list_t *execute_command(char *_exec, struct sway_seat *seat, list_t *res_list = create_list(); char *exec = strdup(_exec); char *head = exec; - char *cmdlist; char *cmd; + char matched_delim = ';'; list_t *views = NULL; if (seat == NULL) { @@ -227,16 +227,13 @@ list_t *execute_command(char *_exec, struct sway_seat *seat, head = exec; do { - // Split command list - cmdlist = argsep(&head, ";"); - do { - // Skip leading whitespace - for (; isspace(*cmdlist); ++cmdlist) {} - // Extract criteria (valid for this command chain only). + for (; isspace(*head); ++head) {} + // Extract criteria (valid for this command list only). + if (matched_delim == ';') { config->handler_context.using_criteria = false; - if (*cmdlist == '[') { + if (*head == '[') { char *error = NULL; - struct criteria *criteria = criteria_parse(cmdlist, &error); + struct criteria *criteria = criteria_parse(head, &error); if (!criteria) { list_add(res_list, cmd_results_new(CMD_INVALID, "%s", error)); @@ -245,71 +242,71 @@ list_t *execute_command(char *_exec, struct sway_seat *seat, } list_free(views); views = criteria_get_views(criteria); - cmdlist += strlen(criteria->raw); + head += strlen(criteria->raw); criteria_destroy(criteria); config->handler_context.using_criteria = true; // Skip leading whitespace - for (; isspace(*cmdlist); ++cmdlist) {} - } - // Split command chain into commands - cmd = argsep(&cmdlist, ","); - for (; isspace(*cmd); ++cmd) {} - if (strcmp(cmd, "") == 0) { - sway_log(SWAY_INFO, "Ignoring empty command."); - continue; + for (; isspace(*head); ++head) {} } - sway_log(SWAY_INFO, "Handling command '%s'", cmd); - //TODO better handling of argv - int argc; - char **argv = split_args(cmd, &argc); - if (strcmp(argv[0], "exec") != 0 && - strcmp(argv[0], "exec_always") != 0 && - strcmp(argv[0], "mode") != 0) { - int i; - for (i = 1; i < argc; ++i) { - if (*argv[i] == '\"' || *argv[i] == '\'') { - strip_quotes(argv[i]); - } + } + // Split command list + cmd = argsep(&head, ";,", &matched_delim); + for (; isspace(*cmd); ++cmd) {} + + if (strcmp(cmd, "") == 0) { + sway_log(SWAY_INFO, "Ignoring empty command."); + continue; + } + sway_log(SWAY_INFO, "Handling command '%s'", cmd); + //TODO better handling of argv + int argc; + char **argv = split_args(cmd, &argc); + if (strcmp(argv[0], "exec") != 0 && + strcmp(argv[0], "exec_always") != 0 && + strcmp(argv[0], "mode") != 0) { + for (int i = 1; i < argc; ++i) { + if (*argv[i] == '\"' || *argv[i] == '\'') { + strip_quotes(argv[i]); } } - struct cmd_handler *handler = find_handler(argv[0], NULL, 0); - if (!handler) { - list_add(res_list, cmd_results_new(CMD_INVALID, - "Unknown/invalid command '%s'", argv[0])); + } + struct cmd_handler *handler = find_handler(argv[0], NULL, 0); + if (!handler) { + list_add(res_list, cmd_results_new(CMD_INVALID, + "Unknown/invalid command '%s'", argv[0])); + free_argv(argc, argv); + goto cleanup; + } + + // Var replacement, for all but first argument of set + for (int i = handler->handle == cmd_set ? 2 : 1; i < argc; ++i) { + argv[i] = do_var_replacement(argv[i]); + } + + if (!config->handler_context.using_criteria) { + // The container or workspace which this command will run on. + struct sway_node *node = con ? &con->node : + seat_get_focus_inactive(seat, &root->node); + set_config_node(node); + struct cmd_results *res = handler->handle(argc-1, argv+1); + list_add(res_list, res); + if (res->status == CMD_INVALID) { free_argv(argc, argv); goto cleanup; } - - // Var replacement, for all but first argument of set - for (int i = handler->handle == cmd_set ? 2 : 1; i < argc; ++i) { - argv[i] = do_var_replacement(argv[i]); - } - - if (!config->handler_context.using_criteria) { - // The container or workspace which this command will run on. - struct sway_node *node = con ? &con->node : - seat_get_focus_inactive(seat, &root->node); - set_config_node(node); + } else { + for (int i = 0; i < views->length; ++i) { + struct sway_view *view = views->items[i]; + set_config_node(&view->container->node); struct cmd_results *res = handler->handle(argc-1, argv+1); list_add(res_list, res); if (res->status == CMD_INVALID) { free_argv(argc, argv); goto cleanup; } - } else { - for (int i = 0; i < views->length; ++i) { - struct sway_view *view = views->items[i]; - set_config_node(&view->container->node); - struct cmd_results *res = handler->handle(argc-1, argv+1); - list_add(res_list, res); - if (res->status == CMD_INVALID) { - free_argv(argc, argv); - goto cleanup; - } - } } - free_argv(argc, argv); - } while(cmdlist); + } + free_argv(argc, argv); } while(head); cleanup: free(exec); diff --git a/sway/tree/workspace.c b/sway/tree/workspace.c index 1a1f5c49..e1ef40f4 100644 --- a/sway/tree/workspace.c +++ b/sway/tree/workspace.c @@ -212,9 +212,9 @@ static void workspace_name_from_binding(const struct sway_binding * binding, char *name = NULL; // workspace n - char *cmd = argsep(&cmdlist, " "); + char *cmd = argsep(&cmdlist, " ", NULL); if (cmdlist) { - name = argsep(&cmdlist, ",;"); + name = argsep(&cmdlist, ",;", NULL); } // TODO: support "move container to workspace" bindings as well