From 4d21f4cfc2cfd3166164218f665f89b9ec8a69a5 Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Tue, 11 Sep 2018 19:11:05 +0300 Subject: [PATCH 1/4] init_ws_for_output: use workspace_move_to_output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a crash produced with the following config: # i3 config file (v4) workspace 1 output $screen1 workspace 2 output $screen2 exec --no-startup-id "i3-msg workspace 1, open && i3-msg workspace 2 && xrandr --output $screen2 --off && xrandr --output $screen1 --auto --output $screen2 --auto --right-of $screen1 " Which results in: ERROR: AddressSanitizer: heap-use-after-free on address … READ of size 8 at 0x614000001f48 thread T0 #0 0x5563df6e73a8 in init_ws_for_output i3/src/randr.c:468 #1 0x5563df6ef3b4 in randr_query_outputs i3/src/randr.c:940 #2 0x5563df68dbe1 in handle_screen_change i3/src/handlers.c:450 … is located 264 bytes inside of 448-byte region … freed by thread T0 here: #1 0x5563df634b0a in con_free i3/src/con.c:96 #2 0x5563df7151e6 in tree_close_internal i3/src/tree.c:344 #3 0x5563df7280fe in workspace_show i3/src/workspace.c:499 #4 0x5563df6e7315 in init_ws_for_output i3/src/randr.c:457 #5 0x5563df6ef3b4 in randr_query_outputs i3/src/randr.c:940 #6 0x5563df68dbe1 in handle_screen_change i3/src/handlers.c:450 Which is similar to #3228, #3248. --- src/randr.c | 48 +++++++----------------------------------------- src/workspace.c | 10 +++++++++- 2 files changed, 16 insertions(+), 42 deletions(-) diff --git a/src/randr.c b/src/randr.c index b1a9e8cb..0c1f9704 100644 --- a/src/randr.c +++ b/src/randr.c @@ -442,47 +442,13 @@ void init_ws_for_output(Output *output, Con *content) { continue; } - /* if so, move it over */ - LOG("Moving workspace \"%s\" from output \"%s\" to \"%s\" due to assignment\n", - workspace->name, workspace_out->name, output_primary_name(output)); - - /* if the workspace is currently visible on that output, we need to - * switch to a different workspace - otherwise the output would end up - * with no active workspace */ - bool visible = workspace_is_visible(workspace); - Con *previous = NULL; - if (visible && (previous = TAILQ_NEXT(workspace, focused))) { - LOG("Switching to previously used workspace \"%s\" on output \"%s\"\n", - previous->name, workspace_out->name); - workspace_show(previous); - } - - /* Render the output on which the workspace was to get correct Rects. - * Then, we need to work with the "content" container, since we cannot - * be sure that the workspace itself was rendered at all (in case it’s - * invisible, it won’t be rendered). */ - render_con(workspace_out, false); - Con *ws_out_content = output_get_content(workspace_out); - - Con *floating_con; - TAILQ_FOREACH(floating_con, &(workspace->floating_head), floating_windows) - /* NB: We use output->con here because content is not yet rendered, - * so it has a rect of {0, 0, 0, 0}. */ - floating_fix_coordinates(floating_con, &(ws_out_content->rect), &(output->con->rect)); - - con_detach(workspace); - con_attach(workspace, content, false); - - /* In case the workspace we just moved was visible but there was no - * other workspace to switch to, we need to initialize the source - * output as well */ - if (visible && previous == NULL) { - LOG("There is no workspace left on \"%s\", re-initializing\n", - workspace_out->name); - init_ws_for_output(get_output_by_name(workspace_out->name, true), - output_get_content(workspace_out)); - DLOG("Done re-initializing, continuing with \"%s\"\n", output_primary_name(output)); - } + DLOG("Moving workspace \"%s\" from output \"%s\" to \"%s\" due to assignment\n", + workspace->name, workspace_out->name, output_primary_name(output)); + /* Need to copy output's rect since content is not yet rendered. We + * can't call render_con here because render_output only proceeds if a + * workspace exists. */ + content->rect = output->con->rect; + workspace_move_to_output(workspace, output); } /* if a workspace exists, we are done now */ diff --git a/src/workspace.c b/src/workspace.c index e92aaa5f..2a9f88db 100644 --- a/src/workspace.c +++ b/src/workspace.c @@ -966,7 +966,11 @@ bool workspace_move_to_output(Con *ws, Output *output) { LOG("got output %p with content %p\n", output, content); Con *previously_visible_ws = TAILQ_FIRST(&(content->focus_head)); - LOG("Previously visible workspace = %p / %s\n", previously_visible_ws, previously_visible_ws->name); + if (previously_visible_ws) { + DLOG("Previously visible workspace = %p / %s\n", previously_visible_ws, previously_visible_ws->name); + } else { + DLOG("No previously visible workspace on output.\n"); + } bool workspace_was_visible = workspace_is_visible(ws); if (con_num_children(ws->parent) == 1) { @@ -1024,6 +1028,10 @@ bool workspace_move_to_output(Con *ws, Output *output) { workspace_show(ws); } + if (!previously_visible_ws) { + return true; + } + /* NB: We cannot simply work with previously_visible_ws since it might * have been cleaned up by workspace_show() already, depending on the * focus order/number of other workspaces on the output. From db3b9e41874400958cf85b461a5d1ff04a398fa3 Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Tue, 11 Sep 2018 20:09:16 +0300 Subject: [PATCH 2/4] randr_disable_output: Always restore focus con_detach and con_attach modify the focus stack. This will make sure that the currently focused workspace will remain focused after disabling an output. --- src/randr.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/randr.c b/src/randr.c index 0c1f9704..4123cfcf 100644 --- a/src/randr.c +++ b/src/randr.c @@ -942,13 +942,8 @@ void randr_disable_output(Output *output) { if (output->con != NULL) { /* We need to move the workspaces from the disappearing output to the first output */ - /* 1: Get the con to focus next, if the disappearing ws is focused */ - Con *next = NULL; - if (TAILQ_FIRST(&(croot->focus_head)) == output->con) { - DLOG("This output (%p) was focused! Getting next\n", output->con); - next = focused; - DLOG("next = %p\n", next); - } + /* 1: Get the con to focus next */ + Con *next = focused; /* 2: iterate through workspaces and re-assign them, fixing the coordinates * of floating containers as we go */ @@ -971,15 +966,12 @@ void randr_disable_output(Output *output) { TAILQ_FOREACH(floating_con, &(current->floating_head), floating_windows) { floating_fix_coordinates(floating_con, &(output->con->rect), &(first->con->rect)); } - DLOG("Done, next\n"); } - DLOG("re-attached all workspaces\n"); - if (next) { - DLOG("now focusing next = %p\n", next); - con_activate(next); - workspace_show(con_get_workspace(next)); - } + /* Restore focus after con_detach / con_attach */ + DLOG("now focusing next = %p\n", next); + con_focus(next); + workspace_show(con_get_workspace(next)); /* 3: move the dock clients to the first output */ Con *child; From 5976381012ab1ce382ef6eb8e432ec39362a03c5 Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Tue, 11 Sep 2018 20:39:33 +0300 Subject: [PATCH 3/4] output_init_con: Restore focus if possible Before this, i3 would focus newly created workspaces on output init --- src/randr.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/randr.c b/src/randr.c index 4123cfcf..7c90f1c0 100644 --- a/src/randr.c +++ b/src/randr.c @@ -421,6 +421,8 @@ void output_init_con(Output *output) { * */ void init_ws_for_output(Output *output, Con *content) { + Con *previous_focus = con_get_workspace(focused); + /* go through all assignments and move the existing workspaces to this output */ struct Workspace_Assignment *assignment; TAILQ_FOREACH(assignment, &ws_assignments, ws_assignments) { @@ -451,6 +453,9 @@ void init_ws_for_output(Output *output, Con *content) { workspace_move_to_output(workspace, output); } + /* Temporarily set the focused container, might not be initialized yet. */ + focused = content; + /* if a workspace exists, we are done now */ if (!TAILQ_EMPTY(&(content->nodes_head))) { /* ensure that one of the workspaces is actually visible (in fullscreen @@ -459,10 +464,9 @@ void init_ws_for_output(Output *output, Con *content) { GREP_FIRST(visible, content, child->fullscreen_mode == CF_OUTPUT); if (!visible) { visible = TAILQ_FIRST(&(content->nodes_head)); - focused = content; workspace_show(visible); } - return; + goto restore_focus; } /* otherwise, we create the first assigned ws for this output */ @@ -473,17 +477,18 @@ void init_ws_for_output(Output *output, Con *content) { LOG("Initializing first assigned workspace \"%s\" for output \"%s\"\n", assignment->name, assignment->output); - focused = content; workspace_show_by_name(assignment->name); - return; + goto restore_focus; } /* if there is still no workspace, we create the first free workspace */ DLOG("Now adding a workspace\n"); - Con *ws = create_workspace_on_output(output, content); + workspace_show(create_workspace_on_output(output, content)); - /* TODO: Set focus in main.c */ - con_focus(ws); +restore_focus: + if (previous_focus) { + workspace_show(previous_focus); + } } /* From f6bb1e22bb62d483f883e53c8749d4ec288ab7c0 Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Wed, 12 Sep 2018 16:53:20 +0300 Subject: [PATCH 4/4] init_ws_for_output: Remove content argument --- include/randr.h | 2 +- src/fake_outputs.c | 2 +- src/randr.c | 7 ++++--- src/xinerama.c | 4 ++-- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/include/randr.h b/include/randr.h index 39182c54..5c0c8b3d 100644 --- a/include/randr.h +++ b/include/randr.h @@ -48,7 +48,7 @@ void output_init_con(Output *output); * • Create the first unused workspace. * */ -void init_ws_for_output(Output *output, Con *content); +void init_ws_for_output(Output *output); /** * Initializes the specified output, assigning the specified workspace to it. diff --git a/src/fake_outputs.c b/src/fake_outputs.c index 39cbd7bb..5f3622d4 100644 --- a/src/fake_outputs.c +++ b/src/fake_outputs.c @@ -74,7 +74,7 @@ void fake_outputs_init(const char *output_spec) { else TAILQ_INSERT_TAIL(&outputs, new_output, outputs); output_init_con(new_output); - init_ws_for_output(new_output, output_get_content(new_output->con)); + init_ws_for_output(new_output); num_screens++; } new_output->primary = primary; diff --git a/src/randr.c b/src/randr.c index 7c90f1c0..d22726d2 100644 --- a/src/randr.c +++ b/src/randr.c @@ -420,7 +420,8 @@ void output_init_con(Output *output) { * • Create the first unused workspace. * */ -void init_ws_for_output(Output *output, Con *content) { +void init_ws_for_output(Output *output) { + Con *content = output_get_content(output->con); Con *previous_focus = con_get_workspace(focused); /* go through all assignments and move the existing workspaces to this output */ @@ -908,7 +909,7 @@ void randr_query_outputs(void) { if (!TAILQ_EMPTY(&(content->nodes_head))) continue; DLOG("Should add ws for output %s\n", output_primary_name(output)); - init_ws_for_output(output, content); + init_ws_for_output(output); } /* Focus the primary screen, if possible */ @@ -1013,7 +1014,7 @@ void randr_disable_output(Output *output) { static void fallback_to_root_output(void) { root_output->active = true; output_init_con(root_output); - init_ws_for_output(root_output, output_get_content(root_output->con)); + init_ws_for_output(root_output); } /* diff --git a/src/xinerama.c b/src/xinerama.c index d0651a85..4acfd3cb 100644 --- a/src/xinerama.c +++ b/src/xinerama.c @@ -71,7 +71,7 @@ static void query_screens(xcb_connection_t *conn) { else TAILQ_INSERT_TAIL(&outputs, s, outputs); output_init_con(s); - init_ws_for_output(s, output_get_content(s->con)); + init_ws_for_output(s); num_screens++; } @@ -98,7 +98,7 @@ static void use_root_output(xcb_connection_t *conn) { s->active = true; TAILQ_INSERT_TAIL(&outputs, s, outputs); output_init_con(s); - init_ws_for_output(s, output_get_content(s->con)); + init_ws_for_output(s); } /*