From 1b7f554474552bd6c463b417305562d6d7dfd3d3 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 16:44:36 +0900 Subject: [PATCH 01/22] log_kernel: s/fclose/pclose/ (for popen'd FILE) With recent glibc the functions are strictly identical, but this might not be true for all libc implementations Found through static analysis. --- sway/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sway/main.c b/sway/main.c index a325dc3a..124f9fbb 100644 --- a/sway/main.c +++ b/sway/main.c @@ -175,7 +175,7 @@ static void log_kernel() { } free(line); } - fclose(f); + pclose(f); } static void security_sanity_check() { From 9c8fb7d025920eacf264e290010e235452235c83 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 16:49:13 +0900 Subject: [PATCH 02/22] invoke_swaybar: fix message length header size size_t/ssize_t are 8 bytes on 64bit systems, so use the proper size to transmit that information. This could lead to ridiculously large alloc as len is not initialized to zero Found through static analysis --- sway/config/bar.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sway/config/bar.c b/sway/config/bar.c index 5a97c3cc..e790c911 100644 --- a/sway/config/bar.c +++ b/sway/config/bar.c @@ -174,7 +174,7 @@ void invoke_swaybar(struct bar_config *bar) { if (!command) { const char msg[] = "Unable to allocate swaybar command string"; size_t msg_len = sizeof(msg); - if (write(filedes[1], &msg_len, sizeof(int))) {}; + if (write(filedes[1], &msg_len, sizeof(size_t))) {}; if (write(filedes[1], msg, msg_len)) {}; close(filedes[1]); exit(1); @@ -189,8 +189,8 @@ void invoke_swaybar(struct bar_config *bar) { } wlr_log(L_DEBUG, "Spawned swaybar %d", bar->pid); close(filedes[0]); - ssize_t len; - if (read(filedes[1], &len, sizeof(int)) == sizeof(int)) { + size_t len; + if (read(filedes[1], &len, sizeof(size_t)) == sizeof(size_t)) { char *buf = malloc(len); if(!buf) { wlr_log(L_ERROR, "Cannot allocate error string"); From 546ddbcd5bd76def3bb51114d4e1e6eb93eb16e7 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 17:02:23 +0900 Subject: [PATCH 03/22] ipc-server: fix double-free on send error in ipc_send_event ipc_send_reply already does client disconnect on error, so we shouldn't do it again. We also need to process current index again as disconnect removes client from the list we currently are processing (this is an indexed "list") Found through static analysis. --- sway/ipc-server.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sway/ipc-server.c b/sway/ipc-server.c index 241fe742..ec933ec3 100644 --- a/sway/ipc-server.c +++ b/sway/ipc-server.c @@ -263,7 +263,10 @@ static void ipc_send_event(const char *json_string, enum ipc_command_type event) client->current_command = event; if (!ipc_send_reply(client, json_string, (uint32_t) strlen(json_string))) { wlr_log_errno(L_INFO, "Unable to send reply to IPC client"); - ipc_client_disconnect(client); + /* ipc_send_reply destroys client on error, which also + * removes it from the list, so we need to process + * current index again */ + i--; } } } From 0ab04b7434306c5faceda2b4d2922e9d78de0184 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 17:03:45 +0900 Subject: [PATCH 04/22] ipc-server: minor code cleanup No logic change here, this one is mostly to please static analyzer: - client->fd can never be -1 (and if it could, close() a few lines below would have needed the same check) - we never send permission denied error (dead code) --- sway/ipc-server.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/sway/ipc-server.c b/sway/ipc-server.c index ec933ec3..2dfe2d03 100644 --- a/sway/ipc-server.c +++ b/sway/ipc-server.c @@ -386,9 +386,7 @@ void ipc_client_disconnect(struct ipc_client *client) { return; } - if (client->fd != -1) { - shutdown(client->fd, SHUT_RDWR); - } + shutdown(client->fd, SHUT_RDWR); wlr_log(L_INFO, "IPC Client %d disconnected", client->fd); wl_event_source_remove(client->event_source); @@ -468,8 +466,6 @@ void ipc_client_handle_command(struct ipc_client *client) { } buf[client->payload_length] = '\0'; - const char *error_denied = "{ \"success\": false, \"error\": \"Permission denied\" }"; - switch (client->current_command) { case IPC_COMMAND: { @@ -650,9 +646,6 @@ void ipc_client_handle_command(struct ipc_client *client) { goto exit_cleanup; } - ipc_send_reply(client, error_denied, (uint32_t)strlen(error_denied)); - wlr_log(L_DEBUG, "Denied IPC client access to %i", client->current_command); - exit_cleanup: client->payload_length = 0; free(buf); From ebe69583c7304fe50247b5929106e3e5ce95b53a Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 17:18:47 +0900 Subject: [PATCH 05/22] ipc-server: fix more use-after-frees on ipc_send_reply error Since ipc_send_reply frees the client on error, we need to check the return value properly as we access client later on Found through static analysis. --- sway/ipc-server.c | 46 +++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/sway/ipc-server.c b/sway/ipc-server.c index 2dfe2d03..3e510c2e 100644 --- a/sway/ipc-server.c +++ b/sway/ipc-server.c @@ -466,6 +466,7 @@ void ipc_client_handle_command(struct ipc_client *client) { } buf[client->payload_length] = '\0'; + bool client_valid = true; switch (client->current_command) { case IPC_COMMAND: { @@ -473,7 +474,7 @@ void ipc_client_handle_command(struct ipc_client *client) { const char *json = cmd_results_to_json(results); char reply[256]; int length = snprintf(reply, sizeof(reply), "%s", json); - ipc_send_reply(client, reply, (uint32_t) length); + client_valid = ipc_send_reply(client, reply, (uint32_t)length); free_cmd_results(results); goto exit_cleanup; } @@ -496,7 +497,8 @@ void ipc_client_handle_command(struct ipc_client *client) { } } const char *json_string = json_object_to_json_string(outputs); - ipc_send_reply(client, json_string, (uint32_t) strlen(json_string)); + client_valid = + ipc_send_reply(client, json_string, (uint32_t)strlen(json_string)); json_object_put(outputs); // free goto exit_cleanup; } @@ -507,7 +509,8 @@ void ipc_client_handle_command(struct ipc_client *client) { container_for_each_descendant_dfs(&root_container, ipc_get_workspaces_callback, workspaces); const char *json_string = json_object_to_json_string(workspaces); - ipc_send_reply(client, json_string, (uint32_t) strlen(json_string)); + client_valid = + ipc_send_reply(client, json_string, (uint32_t)strlen(json_string)); json_object_put(workspaces); // free goto exit_cleanup; } @@ -517,7 +520,7 @@ void ipc_client_handle_command(struct ipc_client *client) { // TODO: Check if they're permitted to use these events struct json_object *request = json_tokener_parse(buf); if (request == NULL) { - ipc_send_reply(client, "{\"success\": false}", 18); + client_valid = ipc_send_reply(client, "{\"success\": false}", 18); wlr_log_errno(L_INFO, "Failed to read request"); goto exit_cleanup; } @@ -538,7 +541,8 @@ void ipc_client_handle_command(struct ipc_client *client) { } else if (strcmp(event_type, "binding") == 0) { client->subscribed_events |= event_mask(IPC_EVENT_BINDING); } else { - ipc_send_reply(client, "{\"success\": false}", 18); + client_valid = + ipc_send_reply(client, "{\"success\": false}", 18); json_object_put(request); wlr_log_errno(L_INFO, "Failed to parse request"); goto exit_cleanup; @@ -546,7 +550,7 @@ void ipc_client_handle_command(struct ipc_client *client) { } json_object_put(request); - ipc_send_reply(client, "{\"success\": true}", 17); + client_valid = ipc_send_reply(client, "{\"success\": true}", 17); goto exit_cleanup; } @@ -558,7 +562,8 @@ void ipc_client_handle_command(struct ipc_client *client) { json_object_array_add(inputs, ipc_json_describe_input(device)); } const char *json_string = json_object_to_json_string(inputs); - ipc_send_reply(client, json_string, (uint32_t)strlen(json_string)); + client_valid = + ipc_send_reply(client, json_string, (uint32_t)strlen(json_string)); json_object_put(inputs); // free goto exit_cleanup; } @@ -571,7 +576,8 @@ void ipc_client_handle_command(struct ipc_client *client) { json_object_array_add(seats, ipc_json_describe_seat(seat)); } const char *json_string = json_object_to_json_string(seats); - ipc_send_reply(client, json_string, (uint32_t)strlen(json_string)); + client_valid = + ipc_send_reply(client, json_string, (uint32_t)strlen(json_string)); json_object_put(seats); // free goto exit_cleanup; } @@ -581,7 +587,8 @@ void ipc_client_handle_command(struct ipc_client *client) { json_object *tree = ipc_json_describe_container_recursive(&root_container); const char *json_string = json_object_to_json_string(tree); - ipc_send_reply(client, json_string, (uint32_t) strlen(json_string)); + client_valid = + ipc_send_reply(client, json_string, (uint32_t) strlen(json_string)); json_object_put(tree); goto exit_cleanup; } @@ -592,7 +599,8 @@ void ipc_client_handle_command(struct ipc_client *client) { container_descendants(&root_container, C_VIEW, ipc_get_marks_callback, marks); const char *json_string = json_object_to_json_string(marks); - ipc_send_reply(client, json_string, (uint32_t)strlen(json_string)); + client_valid = + ipc_send_reply(client, json_string, (uint32_t)strlen(json_string)); json_object_put(marks); goto exit_cleanup; } @@ -601,7 +609,8 @@ void ipc_client_handle_command(struct ipc_client *client) { { json_object *version = ipc_json_get_version(); const char *json_string = json_object_to_json_string(version); - ipc_send_reply(client, json_string, (uint32_t)strlen(json_string)); + client_valid = + ipc_send_reply(client, json_string, (uint32_t)strlen(json_string)); json_object_put(version); // free goto exit_cleanup; } @@ -616,7 +625,9 @@ void ipc_client_handle_command(struct ipc_client *client) { json_object_array_add(bars, json_object_new_string(bar->id)); } const char *json_string = json_object_to_json_string(bars); - ipc_send_reply(client, json_string, (uint32_t)strlen(json_string)); + client_valid = + ipc_send_reply(client, json_string, + (uint32_t)strlen(json_string)); json_object_put(bars); // free } else { // Send particular bar's details @@ -630,12 +641,15 @@ void ipc_client_handle_command(struct ipc_client *client) { } if (!bar) { const char *error = "{ \"success\": false, \"error\": \"No bar with that ID\" }"; - ipc_send_reply(client, error, (uint32_t)strlen(error)); + client_valid = + ipc_send_reply(client, error, (uint32_t)strlen(error)); goto exit_cleanup; } json_object *json = ipc_json_describe_bar_config(bar); const char *json_string = json_object_to_json_string(json); - ipc_send_reply(client, json_string, (uint32_t)strlen(json_string)); + client_valid = + ipc_send_reply(client, json_string, + (uint32_t)strlen(json_string)); json_object_put(json); // free } goto exit_cleanup; @@ -647,7 +661,9 @@ void ipc_client_handle_command(struct ipc_client *client) { } exit_cleanup: - client->payload_length = 0; + if (client_valid) { + client->payload_length = 0; + } free(buf); return; } From 5690bea22745789ada70ba8b4814f2e15ee23bd2 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 22:05:27 +0900 Subject: [PATCH 06/22] input_config: free new_input_config on error Found through static analysis. --- sway/commands/input/accel_profile.c | 1 + sway/commands/input/click_method.c | 1 + sway/commands/input/drag_lock.c | 1 + sway/commands/input/dwt.c | 1 + sway/commands/input/events.c | 1 + sway/commands/input/left_handed.c | 1 + sway/commands/input/map_from_region.c | 8 ++++++++ sway/commands/input/middle_emulation.c | 1 + sway/commands/input/natural_scroll.c | 1 + sway/commands/input/pointer_accel.c | 1 + sway/commands/input/repeat_delay.c | 1 + sway/commands/input/repeat_rate.c | 1 + sway/commands/input/scroll_method.c | 1 + sway/commands/input/tap.c | 1 + 14 files changed, 21 insertions(+) diff --git a/sway/commands/input/accel_profile.c b/sway/commands/input/accel_profile.c index 37d6e133..a4108ec3 100644 --- a/sway/commands/input/accel_profile.c +++ b/sway/commands/input/accel_profile.c @@ -23,6 +23,7 @@ struct cmd_results *input_cmd_accel_profile(int argc, char **argv) { } else if (strcasecmp(argv[0], "flat") == 0) { new_config->accel_profile = LIBINPUT_CONFIG_ACCEL_PROFILE_FLAT; } else { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "accel_profile", "Expected 'accel_profile '"); } diff --git a/sway/commands/input/click_method.c b/sway/commands/input/click_method.c index 8f1f0aa7..5d0d8cc2 100644 --- a/sway/commands/input/click_method.c +++ b/sway/commands/input/click_method.c @@ -26,6 +26,7 @@ struct cmd_results *input_cmd_click_method(int argc, char **argv) { } else if (strcasecmp(argv[0], "clickfinger") == 0) { new_config->click_method = LIBINPUT_CONFIG_CLICK_METHOD_CLICKFINGER; } else { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "click_method", "Expected 'click_method drag_lock = LIBINPUT_CONFIG_DRAG_LOCK_DISABLED; } else { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "drag_lock", "Expected 'drag_lock '"); } diff --git a/sway/commands/input/dwt.c b/sway/commands/input/dwt.c index 995a2f47..73937507 100644 --- a/sway/commands/input/dwt.c +++ b/sway/commands/input/dwt.c @@ -22,6 +22,7 @@ struct cmd_results *input_cmd_dwt(int argc, char **argv) { } else if (strcasecmp(argv[0], "disabled") == 0) { new_config->dwt = LIBINPUT_CONFIG_DWT_DISABLED; } else { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "dwt", "Expected 'dwt '"); } diff --git a/sway/commands/input/events.c b/sway/commands/input/events.c index 2217f5ce..e2ccdc94 100644 --- a/sway/commands/input/events.c +++ b/sway/commands/input/events.c @@ -29,6 +29,7 @@ struct cmd_results *input_cmd_events(int argc, char **argv) { new_config->send_events = LIBINPUT_CONFIG_SEND_EVENTS_DISABLED_ON_EXTERNAL_MOUSE; } else { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "events", "Expected 'events '"); } diff --git a/sway/commands/input/left_handed.c b/sway/commands/input/left_handed.c index 94b8e03e..769ce98c 100644 --- a/sway/commands/input/left_handed.c +++ b/sway/commands/input/left_handed.c @@ -23,6 +23,7 @@ struct cmd_results *input_cmd_left_handed(int argc, char **argv) { } else if (strcasecmp(argv[0], "disabled") == 0) { new_config->left_handed = 0; } else { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "left_handed", "Expected 'left_handed '"); } diff --git a/sway/commands/input/map_from_region.c b/sway/commands/input/map_from_region.c index 80bb856d..40f04214 100644 --- a/sway/commands/input/map_from_region.c +++ b/sway/commands/input/map_from_region.c @@ -54,20 +54,28 @@ struct cmd_results *input_cmd_map_from_region(int argc, char **argv) { bool mm1, mm2; if (!parse_coords(argv[0], &new_config->mapped_from_region->x1, &new_config->mapped_from_region->y1, &mm1)) { + free(new_config->mapped_from_region); + free_input_config(new_config); return cmd_results_new(CMD_FAILURE, "map_from_region", "Invalid top-left coordinates"); } if (!parse_coords(argv[1], &new_config->mapped_from_region->x2, &new_config->mapped_from_region->y2, &mm2)) { + free(new_config->mapped_from_region); + free_input_config(new_config); return cmd_results_new(CMD_FAILURE, "map_from_region", "Invalid bottom-right coordinates"); } if (new_config->mapped_from_region->x1 > new_config->mapped_from_region->x2 || new_config->mapped_from_region->y1 > new_config->mapped_from_region->y2) { + free(new_config->mapped_from_region); + free_input_config(new_config); return cmd_results_new(CMD_FAILURE, "map_from_region", "Invalid rectangle"); } if (mm1 != mm2) { + free(new_config->mapped_from_region); + free_input_config(new_config); return cmd_results_new(CMD_FAILURE, "map_from_region", "Both coordinates must be in the same unit"); } diff --git a/sway/commands/input/middle_emulation.c b/sway/commands/input/middle_emulation.c index a551fd51..7ca01629 100644 --- a/sway/commands/input/middle_emulation.c +++ b/sway/commands/input/middle_emulation.c @@ -24,6 +24,7 @@ struct cmd_results *input_cmd_middle_emulation(int argc, char **argv) { new_config->middle_emulation = LIBINPUT_CONFIG_MIDDLE_EMULATION_DISABLED; } else { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "middle_emulation", "Expected 'middle_emulation '"); } diff --git a/sway/commands/input/natural_scroll.c b/sway/commands/input/natural_scroll.c index c4e19b78..55236790 100644 --- a/sway/commands/input/natural_scroll.c +++ b/sway/commands/input/natural_scroll.c @@ -23,6 +23,7 @@ struct cmd_results *input_cmd_natural_scroll(int argc, char **argv) { } else if (strcasecmp(argv[0], "disabled") == 0) { new_config->natural_scroll = 0; } else { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "natural_scroll", "Expected 'natural_scroll '"); } diff --git a/sway/commands/input/pointer_accel.c b/sway/commands/input/pointer_accel.c index 171063aa..8bbd0724 100644 --- a/sway/commands/input/pointer_accel.c +++ b/sway/commands/input/pointer_accel.c @@ -20,6 +20,7 @@ struct cmd_results *input_cmd_pointer_accel(int argc, char **argv) { float pointer_accel = atof(argv[0]); if (pointer_accel < -1 || pointer_accel > 1) { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "pointer_accel", "Input out of range [-1, 1]"); } diff --git a/sway/commands/input/repeat_delay.c b/sway/commands/input/repeat_delay.c index ce265841..c9ddbf0e 100644 --- a/sway/commands/input/repeat_delay.c +++ b/sway/commands/input/repeat_delay.c @@ -20,6 +20,7 @@ struct cmd_results *input_cmd_repeat_delay(int argc, char **argv) { int repeat_delay = atoi(argv[0]); if (repeat_delay < 0) { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "repeat_delay", "Repeat delay cannot be negative"); } diff --git a/sway/commands/input/repeat_rate.c b/sway/commands/input/repeat_rate.c index f2ea2e69..56878176 100644 --- a/sway/commands/input/repeat_rate.c +++ b/sway/commands/input/repeat_rate.c @@ -20,6 +20,7 @@ struct cmd_results *input_cmd_repeat_rate(int argc, char **argv) { int repeat_rate = atoi(argv[0]); if (repeat_rate < 0) { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "repeat_rate", "Repeat rate cannot be negative"); } diff --git a/sway/commands/input/scroll_method.c b/sway/commands/input/scroll_method.c index 0a1c57ac..4c6ac6b6 100644 --- a/sway/commands/input/scroll_method.c +++ b/sway/commands/input/scroll_method.c @@ -27,6 +27,7 @@ struct cmd_results *input_cmd_scroll_method(int argc, char **argv) { } else if (strcasecmp(argv[0], "on_button_down") == 0) { new_config->scroll_method = LIBINPUT_CONFIG_SCROLL_ON_BUTTON_DOWN; } else { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "scroll_method", "Expected 'scroll_method '"); } diff --git a/sway/commands/input/tap.c b/sway/commands/input/tap.c index e7f03058..7d027d5d 100644 --- a/sway/commands/input/tap.c +++ b/sway/commands/input/tap.c @@ -23,6 +23,7 @@ struct cmd_results *input_cmd_tap(int argc, char **argv) { } else if (strcasecmp(argv[0], "disabled") == 0) { new_config->tap = LIBINPUT_CONFIG_TAP_DISABLED; } else { + free_input_config(new_config); return cmd_results_new(CMD_INVALID, "tap", "Expected 'tap '"); } From 557a14a6fe79002427008a0cf831808202609ae4 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 22:12:10 +0900 Subject: [PATCH 07/22] config_commands_command: make alloc failure check more permanent policy is accessed again later Found through static analysis --- sway/commands.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sway/commands.c b/sway/commands.c index 5b20857a..5b67e1ec 100644 --- a/sway/commands.c +++ b/sway/commands.c @@ -428,8 +428,7 @@ struct cmd_results *config_commands_command(char *exec) { struct cmd_handler *handler = find_handler(cmd, NULL, 0); if (!handler && strcmp(cmd, "*") != 0) { - char *input = cmd ? cmd : "(empty)"; - results = cmd_results_new(CMD_INVALID, input, "Unknown/invalid command"); + results = cmd_results_new(CMD_INVALID, cmd, "Unknown/invalid command"); goto cleanup; } @@ -471,10 +470,12 @@ struct cmd_results *config_commands_command(char *exec) { } if (!policy) { policy = alloc_command_policy(cmd); - sway_assert(policy, "Unable to allocate security policy"); - if (policy) { - list_add(config->command_policies, policy); + if (!sway_assert(policy, "Unable to allocate security policy")) { + results = cmd_results_new(CMD_INVALID, cmd, + "Unable to allocate memory"); + goto cleanup; } + list_add(config->command_policies, policy); } policy->context = context; From ab187405297b77f85e0d5ed9630ae43b2db61324 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 22:16:54 +0900 Subject: [PATCH 08/22] output commands: move !argc checks after argc gets decremented Found through static analysis. --- sway/commands/output/mode.c | 2 +- sway/commands/output/position.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sway/commands/output/mode.c b/sway/commands/output/mode.c index daec6d44..ef56ae9e 100644 --- a/sway/commands/output/mode.c +++ b/sway/commands/output/mode.c @@ -36,11 +36,11 @@ struct cmd_results *output_cmd_mode(int argc, char **argv) { } } else { // Format is 1234 4321 + argc--; argv++; if (!argc) { return cmd_results_new(CMD_INVALID, "output", "Missing mode argument (height)."); } - argc--; argv++; output->height = strtol(*argv, &end, 10); if (*end) { return cmd_results_new(CMD_INVALID, "output", diff --git a/sway/commands/output/position.c b/sway/commands/output/position.c index c2aeb281..449767b1 100644 --- a/sway/commands/output/position.c +++ b/sway/commands/output/position.c @@ -27,11 +27,11 @@ struct cmd_results *output_cmd_position(int argc, char **argv) { } } else { // Format is 1234 4321 (legacy) + argc--; argv++; if (!argc) { return cmd_results_new(CMD_INVALID, "output", "Missing position argument (y)."); } - argc--; argv++; config->handler_context.output_config->y = strtol(*argv, &end, 10); if (*end) { return cmd_results_new(CMD_INVALID, "output", From 9c9ee3e4ef9d4e57ec801a79e0b2da1bd2a6d46e Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 22:39:37 +0900 Subject: [PATCH 09/22] find prev/next output/workspace: add NULL check These could be called with NULL if there is no focus Found through static analysis. --- sway/tree/workspace.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sway/tree/workspace.c b/sway/tree/workspace.c index 5eb4be0f..3a311cd1 100644 --- a/sway/tree/workspace.c +++ b/sway/tree/workspace.c @@ -271,6 +271,9 @@ struct sway_container *workspace_by_name(const char *name) { */ struct sway_container *workspace_output_prev_next_impl( struct sway_container *output, bool next) { + if (!output) { + return NULL; + } if (!sway_assert(output->type == C_OUTPUT, "Argument must be an output, is %d", output->type)) { return NULL; @@ -303,6 +306,9 @@ struct sway_container *workspace_output_prev_next_impl( */ struct sway_container *workspace_prev_next_impl( struct sway_container *workspace, bool next) { + if (!workspace) { + return NULL; + } if (!sway_assert(workspace->type == C_WORKSPACE, "Argument must be a workspace, is %d", workspace->type)) { return NULL; From c78ab67877e15e4bbbdfd4e8bb7f94309980489b Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 22:46:48 +0900 Subject: [PATCH 10/22] workspace_next_name: fix string length for ws_num >= 100 The check didn't include && ws_num < 100 so l would always be 1 or 2 Instead of fixing logic it's simpler to just call snprintf twice to get length and use that. Also change malloc failure check to sway_assert because both callers of this function do not do null check and would segfault... Found through static analysis. --- sway/tree/workspace.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/sway/tree/workspace.c b/sway/tree/workspace.c index 3a311cd1..2db06a31 100644 --- a/sway/tree/workspace.c +++ b/sway/tree/workspace.c @@ -109,7 +109,6 @@ static bool workspace_valid_on_output(const char *output_name, char *workspace_next_name(const char *output_name) { wlr_log(L_DEBUG, "Workspace: Generating new workspace name for output %s", output_name); - int l = 1; // Scan all workspace bindings to find the next available workspace name, // if none are found/available then default to a number struct sway_mode *mode = config->current_mode; @@ -202,14 +201,9 @@ char *workspace_next_name(const char *output_name) { // As a fall back, get the current number of active workspaces // and return that + 1 for the next workspace's name int ws_num = root_container.children->length; - if (ws_num >= 10) { - l = 2; - } else if (ws_num >= 100) { - l = 3; - } + int l = snprintf(NULL, 0, "%d", ws_num); char *name = malloc(l + 1); - if (!name) { - wlr_log(L_ERROR, "Could not allocate workspace name"); + if (!sway_assert(name, "Cloud not allocate workspace name")) { return NULL; } sprintf(name, "%d", ws_num++); From df494a7e517281f21fde265bc31badae3245bb26 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 22:51:21 +0900 Subject: [PATCH 11/22] transaction_apply: use float for quotient Pre-dividing 1000/60 would lose 2/3 due to round-up Found through static analysis --- sway/desktop/transaction.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sway/desktop/transaction.c b/sway/desktop/transaction.c index d2932c87..cb524999 100644 --- a/sway/desktop/transaction.c +++ b/sway/desktop/transaction.c @@ -186,8 +186,8 @@ static void transaction_apply(struct sway_transaction *transaction) { (now.tv_nsec - commit->tv_nsec) / 1000000.0; float ms_total = ms_arranging + ms_waiting; wlr_log(L_DEBUG, "Transaction %p: %.1fms arranging, %.1fms waiting, " - "%.1fms total (%.1f frames if 60Hz)", transaction, - ms_arranging, ms_waiting, ms_total, ms_total / (1000 / 60)); + "%.1fms total (%.1f frames if 60Hz)", transaction, + ms_arranging, ms_waiting, ms_total, ms_total / (1000.0f / 60)); } // Apply the instruction state to the container's current state From a2354d599254941da46e3e17b929c1a40e816cc4 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 22:54:41 +0900 Subject: [PATCH 12/22] cmd_background: fix leak on error Found through static analysis. --- sway/commands/output/background.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sway/commands/output/background.c b/sway/commands/output/background.c index 55cbdff0..65b5f902 100644 --- a/sway/commands/output/background.c +++ b/sway/commands/output/background.c @@ -81,8 +81,9 @@ struct cmd_results *output_cmd_background(int argc, char **argv) { // src file is inside configuration dir char *conf = strdup(config->current_config); - if(!conf) { + if (!conf) { wlr_log(L_ERROR, "Failed to duplicate string"); + free(src); return cmd_results_new(CMD_FAILURE, "output", "Unable to allocate resources"); } From 0c6149171b3cbbf2b85a58743555db6fd866fa00 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 22:57:09 +0900 Subject: [PATCH 13/22] read_config: fix leak on error Found through static analysis. --- sway/config.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sway/config.c b/sway/config.c index 12a02163..d3040a67 100644 --- a/sway/config.c +++ b/sway/config.c @@ -583,6 +583,8 @@ bool read_config(FILE *file, struct sway_config *config) { } char *expanded = expand_line(block, line, brace_detected > 0); if (!expanded) { + list_foreach(stack, free); + list_free(stack); return false; } wlr_log(L_DEBUG, "Expanded line: %s", expanded); From f0d1d263204287b4e0f22368c046cefbbb92cb25 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 23:06:40 +0900 Subject: [PATCH 14/22] get_parent_pid: fix memory leak Found through static analysis. --- common/util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/util.c b/common/util.c index fb7f9454..678926ed 100644 --- a/common/util.c +++ b/common/util.c @@ -95,7 +95,7 @@ pid_t get_parent_pid(pid_t child) { token = strtok(NULL, sep); // parent pid parent = strtol(token, NULL, 10); } - + free(buffer); fclose(stat); } From 6d2b82253a5f2fb0ab8e63ded2b62e5d4088e63b Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 23:09:17 +0900 Subject: [PATCH 15/22] bar_cmd_font: fix leak of font join_args is a freshly allocated string and can be used as is. Found through static analysis. --- sway/commands/bar/font.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sway/commands/bar/font.c b/sway/commands/bar/font.c index 80b7a593..f036cbc3 100644 --- a/sway/commands/bar/font.c +++ b/sway/commands/bar/font.c @@ -14,7 +14,7 @@ struct cmd_results *bar_cmd_font(int argc, char **argv) { } char *font = join_args(argv, argc); free(config->current_bar->font); - config->current_bar->font = strdup(font); + config->current_bar->font = font; wlr_log(L_DEBUG, "Settings font '%s' for bar: %s", config->current_bar->font, config->current_bar->id); return cmd_results_new(CMD_SUCCESS, NULL, NULL); From c73c552cae435dd61ebbe0c76aa66570095375a9 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 23:12:17 +0900 Subject: [PATCH 16/22] bar_cmd_modifier: fix use-after-free on error Found through static analysis. --- sway/commands/bar/modifier.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sway/commands/bar/modifier.c b/sway/commands/bar/modifier.c index 7ba4b125..02f845e6 100644 --- a/sway/commands/bar/modifier.c +++ b/sway/commands/bar/modifier.c @@ -22,9 +22,10 @@ struct cmd_results *bar_cmd_modifier(int argc, char **argv) { mod |= tmp_mod; continue; } else { + error = cmd_results_new(CMD_INVALID, "modifier", + "Unknown modifier '%s'", split->items[i]); free_flat_list(split); - return cmd_results_new(CMD_INVALID, "modifier", - "Unknown modifier '%s'", split->items[i]); + return error; } } free_flat_list(split); From 971b2f11f9336c220870fa7504999e0fa5fe266a Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 23:14:43 +0900 Subject: [PATCH 17/22] utf8_size: fix loop boundary Found through static analysis --- common/unicode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/unicode.c b/common/unicode.c index 38a9b48e..5070e083 100644 --- a/common/unicode.c +++ b/common/unicode.c @@ -92,7 +92,7 @@ static const struct { int utf8_size(const char *s) { uint8_t c = (uint8_t)*s; - for (size_t i = 0; i < sizeof(sizes) / 2; ++i) { + for (size_t i = 0; i < sizeof(sizes) / sizeof(*sizes); ++i) { if ((c & sizes[i].mask) == sizes[i].result) { return sizes[i].octets; } From e67c8cf1cb6c09f96bc091612b4eb75aea857452 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 23:17:28 +0900 Subject: [PATCH 18/22] cmd_assign: fix leak on error Found through static analysis. --- sway/commands/assign.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sway/commands/assign.c b/sway/commands/assign.c index 9d15e166..a90498ce 100644 --- a/sway/commands/assign.c +++ b/sway/commands/assign.c @@ -27,6 +27,7 @@ struct cmd_results *cmd_assign(int argc, char **argv) { if (strncmp(*argv, "→", strlen("→")) == 0) { if (argc < 3) { + free(criteria); return cmd_results_new(CMD_INVALID, "assign", "Missing workspace"); } ++argv; From 248ea93c1af7eae5a57c354b0e2e50898f57b17d Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 23:22:21 +0900 Subject: [PATCH 19/22] bar config: fix uninitialized accesses on init error If init fails halfway through it will call the destroy function, which needs some coherent stuff filled. Allocate with calloc and fill in what cannot fail first Found through static analysis. --- sway/config/bar.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/sway/config/bar.c b/sway/config/bar.c index e790c911..b97076a0 100644 --- a/sway/config/bar.c +++ b/sway/config/bar.c @@ -70,16 +70,12 @@ void free_bar_config(struct bar_config *bar) { struct bar_config *default_bar_config(void) { struct bar_config *bar = NULL; - bar = malloc(sizeof(struct bar_config)); + bar = calloc(1, sizeof(struct bar_config)); if (!bar) { return NULL; } - if (!(bar->mode = strdup("dock"))) goto cleanup; - if (!(bar->hidden_state = strdup("hide"))) goto cleanup; bar->outputs = NULL; bar->position = strdup("bottom"); - if (!(bar->bindings = create_list())) goto cleanup; - if (!(bar->status_command = strdup("while :; do date +'%Y-%m-%d %l:%M:%S %p'; sleep 1; done"))) goto cleanup; bar->pango_markup = false; bar->swaybar_command = NULL; bar->font = NULL; @@ -91,6 +87,19 @@ struct bar_config *default_bar_config(void) { bar->binding_mode_indicator = true; bar->verbose = false; bar->pid = 0; + if (!(bar->mode = strdup("dock"))) { + goto cleanup; + } + if (!(bar->hidden_state = strdup("hide"))) { + goto cleanup; + } + if (!(bar->bindings = create_list())) { + goto cleanup; + } + if (!(bar->status_command = + strdup("while date +'%Y-%m-%d %l:%M:%S %p'; do sleep 1; done"))) { + goto cleanup; + } // set default colors if (!(bar->colors.background = strndup("#000000ff", 9))) { goto cleanup; From 8c526bbb03a5171422cf71b0217d271feeb072b6 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 23:34:07 +0900 Subject: [PATCH 20/22] config include: fix leak on relative include path Found through static analysis --- sway/config.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sway/config.c b/sway/config.c index d3040a67..8b6f7b6f 100644 --- a/sway/config.c +++ b/sway/config.c @@ -425,7 +425,7 @@ static bool load_include_config(const char *path, const char *parent_dir, // save parent config const char *parent_config = config->current_config; - char *full_path = strdup(path); + char *full_path; int len = strlen(path); if (len >= 1 && path[0] != '/') { len = len + strlen(parent_dir) + 2; @@ -436,6 +436,8 @@ static bool load_include_config(const char *path, const char *parent_dir, return false; } snprintf(full_path, len, "%s/%s", parent_dir, path); + } else { + full_path = strdup(path); } char *real_path = realpath(full_path, NULL); From 4eeca10a8a1bdef73c0ad7dc8e4d74bb31507676 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 23:36:44 +0900 Subject: [PATCH 21/22] load_config: move NULL path check before first use Found through static analysis --- sway/config.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sway/config.c b/sway/config.c index 8b6f7b6f..0aae1696 100644 --- a/sway/config.c +++ b/sway/config.c @@ -302,6 +302,11 @@ static char *get_config_path(void) { const char *current_config_path; static bool load_config(const char *path, struct sway_config *config) { + if (path == NULL) { + wlr_log(L_ERROR, "Unable to find a config file!"); + return false; + } + wlr_log(L_INFO, "Loading config from %s", path); current_config_path = path; @@ -310,11 +315,6 @@ static bool load_config(const char *path, struct sway_config *config) { return false; } - if (path == NULL) { - wlr_log(L_ERROR, "Unable to find a config file!"); - return false; - } - FILE *f = fopen(path, "r"); if (!f) { wlr_log(L_ERROR, "Unable to open %s for reading", path); From 2725185aeba3ae5dd4ab129158cb4677a8a63042 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 1 Jul 2018 23:40:20 +0900 Subject: [PATCH 22/22] swaylock daemonize: fix leak of devnull fd --- swaylock/main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/swaylock/main.c b/swaylock/main.c index 591df7b4..f31ed679 100644 --- a/swaylock/main.c +++ b/swaylock/main.c @@ -42,6 +42,7 @@ static void daemonize() { int devnull = open("/dev/null", O_RDWR); dup2(STDOUT_FILENO, devnull); dup2(STDERR_FILENO, devnull); + close(devnull); uint8_t success = 0; if (chdir("/") != 0) { write(fds[1], &success, 1);