From ad085c13325d17a242a813879b8574ba3dd43cc7 Mon Sep 17 00:00:00 2001 From: ael-code Date: Fri, 22 Jun 2018 15:41:44 +0200 Subject: bugfix: avoid access after free if src is NULL due to a previous error we cannot use it in the command result string. Moreover if `src` points to `p.we_wordv[0]` we cannot use it after `wordfree(&p)` in the command result string. Bonus feature: If there was an error accessing the file, the string rapresentation of the error is now included in the command result string. --- sway/commands/output/background.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) (limited to 'sway/commands') diff --git a/sway/commands/output/background.c b/sway/commands/output/background.c index 0c5c164f..82bccf68 100644 --- a/sway/commands/output/background.c +++ b/sway/commands/output/background.c @@ -3,6 +3,7 @@ #include #include #include +#include #include "sway/commands.h" #include "sway/config.h" #include "log.h" @@ -71,21 +72,27 @@ struct cmd_results *output_cmd_background(int argc, char **argv) { if (conf) { char *conf_path = dirname(conf); src = malloc(strlen(conf_path) + strlen(src) + 2); - if (src) { - sprintf(src, "%s/%s", conf_path, p.we_wordv[0]); - } else { + if (!src) { + free(conf); + wordfree(&p); wlr_log(L_ERROR, - "Unable to allocate background source"); + "Unable to allocate resource: Not enough memory"); + return cmd_results_new(CMD_FAILURE, "output", + "Unable to allocate resources"); } + sprintf(src, "%s/%s", conf_path, p.we_wordv[0]); free(conf); } else { wlr_log(L_ERROR, "Unable to allocate background source"); } } - if (!src || access(src, F_OK) == -1) { + + if (access(src, F_OK) == -1) { + struct cmd_results *cmd_res = cmd_results_new(CMD_FAILURE, "output", + "Unable to access background file '%s': %s", src, strerror(errno)); + free(src); wordfree(&p); - return cmd_results_new(CMD_INVALID, "output", - "Background file unreadable (%s).", src); + return cmd_res; } output->background = strdup(src); -- cgit v1.2.3 From 4550cb2b3e7e6b4242cf2a3e126b6f47bc8f2182 Mon Sep 17 00:00:00 2001 From: ael-code Date: Tue, 26 Jun 2018 12:53:47 +0200 Subject: fix memleak on background cmd error - src must be free after join_args() - wordfree must bee used after wordexp --- sway/commands/output/background.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'sway/commands') diff --git a/sway/commands/output/background.c b/sway/commands/output/background.c index 82bccf68..4f422cec 100644 --- a/sway/commands/output/background.c +++ b/sway/commands/output/background.c @@ -62,8 +62,11 @@ struct cmd_results *output_cmd_background(int argc, char **argv) { wordexp_t p; char *src = join_args(argv, j); if (wordexp(src, &p, 0) != 0 || p.we_wordv[0] == NULL) { - return cmd_results_new(CMD_INVALID, "output", - "Invalid syntax (%s).", src); + struct cmd_results *cmd_res = cmd_results_new(CMD_INVALID, "output", + "Invalid syntax (%s)", src); + free(src); + wordfree(&p); + return cmd_res; } free(src); src = p.we_wordv[0]; -- cgit v1.2.3 From a4578815f1fa30a7ebb15ddb6601f1ab2f3a3fb6 Mon Sep 17 00:00:00 2001 From: ael-code Date: Tue, 26 Jun 2018 12:57:22 +0200 Subject: cleanup output-background subcommand handling - fixes a double-free error when access() failed. - refactor code to make memory managment (alloc/free) more straightforward - do not bring the temporary wordexp_t struct around - do not postpone errors handling --- sway/commands/output/background.c | 49 ++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 21 deletions(-) (limited to 'sway/commands') diff --git a/sway/commands/output/background.c b/sway/commands/output/background.c index 4f422cec..55cbdff0 100644 --- a/sway/commands/output/background.c +++ b/sway/commands/output/background.c @@ -69,42 +69,49 @@ struct cmd_results *output_cmd_background(int argc, char **argv) { return cmd_res; } free(src); - src = p.we_wordv[0]; + src = strdup(p.we_wordv[0]); + wordfree(&p); + if (!src) { + wlr_log(L_ERROR, "Failed to duplicate string"); + return cmd_results_new(CMD_FAILURE, "output", + "Unable to allocate resource"); + } + if (config->reading && *src != '/') { + // src file is inside configuration dir + char *conf = strdup(config->current_config); - if (conf) { - char *conf_path = dirname(conf); - src = malloc(strlen(conf_path) + strlen(src) + 2); - if (!src) { - free(conf); - wordfree(&p); - wlr_log(L_ERROR, - "Unable to allocate resource: Not enough memory"); - return cmd_results_new(CMD_FAILURE, "output", + if(!conf) { + wlr_log(L_ERROR, "Failed to duplicate string"); + return cmd_results_new(CMD_FAILURE, "output", "Unable to allocate resources"); - } - sprintf(src, "%s/%s", conf_path, p.we_wordv[0]); + } + + char *conf_path = dirname(conf); + char *rel_path = src; + src = malloc(strlen(conf_path) + strlen(src) + 2); + if (!src) { + free(rel_path); free(conf); - } else { - wlr_log(L_ERROR, "Unable to allocate background source"); + wlr_log(L_ERROR, "Unable to allocate memory"); + return cmd_results_new(CMD_FAILURE, "output", + "Unable to allocate resources"); } + + sprintf(src, "%s/%s", conf_path, rel_path); + free(rel_path); + free(conf); } if (access(src, F_OK) == -1) { struct cmd_results *cmd_res = cmd_results_new(CMD_FAILURE, "output", "Unable to access background file '%s': %s", src, strerror(errno)); free(src); - wordfree(&p); return cmd_res; } - output->background = strdup(src); + output->background = src; output->background_option = strdup(mode); - if (src != p.we_wordv[0]) { - free(src); - } - wordfree(&p); - argc -= j + 1; argv += j + 1; } -- cgit v1.2.3