From 82423991a8512ab97fbc41d1e190e709c58bc346 Mon Sep 17 00:00:00 2001 From: Ryan Dwyer Date: Mon, 8 Oct 2018 19:28:53 +1000 Subject: [PATCH 1/3] Reload config using idle event This patch makes it so when you run reload, the actual reloading is deferred to the next time the event loop becomes idle. This avoids several use-after-frees and removes the workarounds we have to avoid them. When you run reload, we validate the config before creating the idle event. This is so the reload command will still return an error if there are validation errors. To allow this, load_main_config has been adjusted so it doesn't apply the config if validating is true rather than applying it unconditionally. This also fixes a memory leak in the reload command where if the config failed to load, the bar_ids list would not be freed. --- include/sway/config.h | 1 - sway/commands/bind.c | 49 ++---------------------------------------- sway/commands/reload.c | 36 +++++++++++++++++++++---------- sway/config.c | 6 ++++++ sway/input/keyboard.c | 4 +--- 5 files changed, 34 insertions(+), 62 deletions(-) diff --git a/include/sway/config.h b/include/sway/config.h index 02ace979..0e51fbfb 100644 --- a/include/sway/config.h +++ b/include/sway/config.h @@ -35,7 +35,6 @@ enum binding_flags { BINDING_BORDER=4, // mouse only; trigger on container border BINDING_CONTENTS=8, // mouse only; trigger on container contents BINDING_TITLEBAR=16, // mouse only; trigger on container titlebar - BINDING_RELOAD=32, // the binding runs the reload command }; /** diff --git a/sway/commands/bind.c b/sway/commands/bind.c index 820c2a6a..701d9746 100644 --- a/sway/commands/bind.c +++ b/sway/commands/bind.c @@ -30,33 +30,6 @@ void free_sway_binding(struct sway_binding *binding) { free(binding); } -static struct sway_binding *sway_binding_dup(struct sway_binding *sb) { - struct sway_binding *new_sb = calloc(1, sizeof(struct sway_binding)); - if (!new_sb) { - return NULL; - } - - new_sb->type = sb->type; - new_sb->order = sb->order; - new_sb->flags = sb->flags; - new_sb->modifiers = sb->modifiers; - new_sb->command = strdup(sb->command); - - new_sb->keys = create_list(); - int i; - for (i = 0; i < sb->keys->length; ++i) { - xkb_keysym_t *key = malloc(sizeof(xkb_keysym_t)); - if (!key) { - free_sway_binding(new_sb); - return NULL; - } - *key = *(xkb_keysym_t *)sb->keys->items[i]; - list_add(new_sb->keys, key); - } - - return new_sb; -} - /** * Returns true if the bindings have the same key and modifier combinations. * Note that keyboard layout is not considered, so the bindings might actually @@ -214,9 +187,6 @@ static struct cmd_results *cmd_bindsym_or_bindcode(int argc, char **argv, } binding->command = join_args(argv + 1, argc - 1); - if (strcasestr(binding->command, "reload")) { - binding->flags |= BINDING_RELOAD; - } list_t *split = split_string(argv[0], "+"); for (int i = 0; i < split->length; ++i) { @@ -306,31 +276,16 @@ struct cmd_results *cmd_bindcode(int argc, char **argv) { * Execute the command associated to a binding */ void seat_execute_command(struct sway_seat *seat, struct sway_binding *binding) { - wlr_log(WLR_DEBUG, "running command for binding: %s", - binding->command); - - struct sway_binding *binding_copy = binding; - // if this is a reload command we need to make a duplicate of the - // binding since it will be gone after the reload has completed. - if (binding->flags & BINDING_RELOAD) { - binding_copy = sway_binding_dup(binding); - if (!binding_copy) { - wlr_log(WLR_ERROR, "Failed to duplicate binding during reload"); - return; - } - } + wlr_log(WLR_DEBUG, "running command for binding: %s", binding->command); config->handler_context.seat = seat; struct cmd_results *results = execute_command(binding->command, NULL, NULL); if (results->status == CMD_SUCCESS) { - ipc_event_binding(binding_copy); + ipc_event_binding(binding); } else { wlr_log(WLR_DEBUG, "could not run command for binding: %s (%s)", binding->command, results->error); } - if (binding_copy->flags & BINDING_RELOAD) { - free_sway_binding(binding_copy); - } free_cmd_results(results); } diff --git a/sway/commands/reload.c b/sway/commands/reload.c index 36fb9092..9e136d48 100644 --- a/sway/commands/reload.c +++ b/sway/commands/reload.c @@ -3,15 +3,12 @@ #include "sway/commands.h" #include "sway/config.h" #include "sway/ipc-server.h" +#include "sway/server.h" #include "sway/tree/arrange.h" #include "list.h" +#include "log.h" -struct cmd_results *cmd_reload(int argc, char **argv) { - struct cmd_results *error = NULL; - if ((error = checkarg(argc, "reload", EXPECTED_EQUAL_TO, 0))) { - return error; - } - +static void do_reload(void *data) { // store bar ids to check against new bars for barconfig_update events list_t *bar_ids = create_list(); for (int i = 0; i < config->bars->length; ++i) { @@ -20,9 +17,12 @@ struct cmd_results *cmd_reload(int argc, char **argv) { } if (!load_main_config(config->current_config_path, true, false)) { - return cmd_results_new(CMD_FAILURE, "reload", - "Error(s) reloading config."); + wlr_log(WLR_ERROR, "Error(s) reloading config"); + list_foreach(bar_ids, free); + list_free(bar_ids); + return; } + ipc_event_workspace(NULL, NULL, "reload"); load_swaybars(); @@ -37,12 +37,26 @@ struct cmd_results *cmd_reload(int argc, char **argv) { } } - for (int i = 0; i < bar_ids->length; ++i) { - free(bar_ids->items[i]); - } + list_foreach(bar_ids, free); list_free(bar_ids); arrange_root(); +} + +struct cmd_results *cmd_reload(int argc, char **argv) { + struct cmd_results *error = NULL; + if ((error = checkarg(argc, "reload", EXPECTED_EQUAL_TO, 0))) { + return error; + } + + if (!load_main_config(config->current_config_path, true, true)) { + return cmd_results_new(CMD_FAILURE, "reload", + "Error(s) reloading config."); + } + + // The reload command frees a lot of stuff, so to avoid use-after-frees + // we schedule the reload to happen using an idle event. + wl_event_loop_add_idle(server.wl_event_loop, do_reload, NULL); return cmd_results_new(CMD_SUCCESS, NULL, NULL); } diff --git a/sway/config.c b/sway/config.c index b56c4f71..8f8ed438 100644 --- a/sway/config.c +++ b/sway/config.c @@ -457,6 +457,12 @@ bool load_main_config(const char *file, bool is_active, bool validating) { success = success && load_config(path, config, &config->swaynag_config_errors); + if (validating) { + free_config(config); + config = old_config; + return success; + } + if (is_active) { for (int i = 0; i < config->output_configs->length; i++) { apply_output_config_to_outputs(config->output_configs->items[i]); diff --git a/sway/input/keyboard.c b/sway/input/keyboard.c index 5fc8a806..0e14c036 100644 --- a/sway/input/keyboard.c +++ b/sway/input/keyboard.c @@ -278,9 +278,7 @@ static void handle_keyboard_key(struct wl_listener *listener, void *data) { raw_modifiers, false, input_inhibited); if (binding_pressed) { - if ((binding_pressed->flags & BINDING_RELOAD) == 0) { - next_repeat_binding = binding_pressed; - } + next_repeat_binding = binding_pressed; seat_execute_command(seat, binding_pressed); handled = true; } From 88a5e26c6eeb28cf934401d377654236fdf8cdfd Mon Sep 17 00:00:00 2001 From: Ryan Dwyer Date: Mon, 8 Oct 2018 20:40:47 +1000 Subject: [PATCH 2/3] Remove unneeded variable --- sway/input/keyboard.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/sway/input/keyboard.c b/sway/input/keyboard.c index 0e14c036..fb1fe7b5 100644 --- a/sway/input/keyboard.c +++ b/sway/input/keyboard.c @@ -264,29 +264,27 @@ static void handle_keyboard_key(struct wl_listener *listener, void *data) { } // Identify and execute active pressed binding - struct sway_binding *next_repeat_binding = NULL; + struct sway_binding *binding = NULL; if (event->state == WLR_KEY_PRESSED) { - struct sway_binding *binding_pressed = NULL; get_active_binding(&keyboard->state_keycodes, - config->current_mode->keycode_bindings, &binding_pressed, + config->current_mode->keycode_bindings, &binding, code_modifiers, false, input_inhibited); get_active_binding(&keyboard->state_keysyms_translated, - config->current_mode->keysym_bindings, &binding_pressed, + config->current_mode->keysym_bindings, &binding, translated_modifiers, false, input_inhibited); get_active_binding(&keyboard->state_keysyms_raw, - config->current_mode->keysym_bindings, &binding_pressed, + config->current_mode->keysym_bindings, &binding, raw_modifiers, false, input_inhibited); - if (binding_pressed) { - next_repeat_binding = binding_pressed; - seat_execute_command(seat, binding_pressed); + if (binding) { + seat_execute_command(seat, binding); handled = true; } } // Set up (or clear) keyboard repeat for a pressed binding - if (next_repeat_binding && wlr_device->keyboard->repeat_info.delay > 0) { - keyboard->repeat_binding = next_repeat_binding; + if (binding && wlr_device->keyboard->repeat_info.delay > 0) { + keyboard->repeat_binding = binding; if (wl_event_source_timer_update(keyboard->key_repeat_source, wlr_device->keyboard->repeat_info.delay) < 0) { wlr_log(WLR_DEBUG, "failed to set key repeat timer"); From e168e8f0ffbcd3924570aec63655c6ca0ab384fa Mon Sep 17 00:00:00 2001 From: Ryan Dwyer Date: Mon, 8 Oct 2018 21:18:30 +1000 Subject: [PATCH 3/3] Don't apply seat config when validating --- sway/commands/seat/attach.c | 4 +++- sway/commands/seat/fallback.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/sway/commands/seat/attach.c b/sway/commands/seat/attach.c index 3e771c00..6b4bcf1f 100644 --- a/sway/commands/seat/attach.c +++ b/sway/commands/seat/attach.c @@ -23,6 +23,8 @@ struct cmd_results *seat_cmd_attach(int argc, char **argv) { new_attachment->identifier = strdup(argv[0]); list_add(new_config->attachments, new_attachment); - apply_seat_config(new_config); + if (!config->validating) { + apply_seat_config(new_config); + } return cmd_results_new(CMD_SUCCESS, NULL, NULL); } diff --git a/sway/commands/seat/fallback.c b/sway/commands/seat/fallback.c index 56feaab5..11f5a08c 100644 --- a/sway/commands/seat/fallback.c +++ b/sway/commands/seat/fallback.c @@ -27,6 +27,8 @@ struct cmd_results *seat_cmd_fallback(int argc, char **argv) { "Expected 'fallback '"); } - apply_seat_config(new_config); + if (!config->validating) { + apply_seat_config(new_config); + } return cmd_results_new(CMD_SUCCESS, NULL, NULL); }