From f120a9d929fd9ad64dd26813967e7b0a03b1390b Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Wed, 13 Sep 2017 16:39:13 +0200 Subject: [PATCH] Bugfix: free incomplete containers when JSON parsing fails related to #2755 --- include/con.h | 6 ++ src/con.c | 24 +++++++ src/load_layout.c | 13 ++++ src/tree.c | 17 +---- testcases/t/215-layout-restore-crash.t | 93 +++++++++++++++++++++++++- 5 files changed, 135 insertions(+), 18 deletions(-) diff --git a/include/con.h b/include/con.h index 1c7bb932..6a7e31bc 100644 --- a/include/con.h +++ b/include/con.h @@ -25,6 +25,12 @@ Con *con_new_skeleton(Con *parent, i3Window *window); */ Con *con_new(Con *parent, i3Window *window); +/** + * Frees the specified container. + * + */ +void con_free(Con *con); + /** * Sets input focus to the given container. Will be updated in X11 in the next * run of x_push_changes(). diff --git a/src/con.c b/src/con.c index 6bbe692f..9797afa6 100644 --- a/src/con.c +++ b/src/con.c @@ -73,6 +73,30 @@ Con *con_new(Con *parent, i3Window *window) { return new; } +/* + * Frees the specified container. + * + */ +void con_free(Con *con) { + free(con->name); + FREE(con->deco_render_params); + TAILQ_REMOVE(&all_cons, con, all_cons); + while (!TAILQ_EMPTY(&(con->swallow_head))) { + Match *match = TAILQ_FIRST(&(con->swallow_head)); + TAILQ_REMOVE(&(con->swallow_head), match, matches); + match_free(match); + free(match); + } + while (!TAILQ_EMPTY(&(con->marks_head))) { + mark_t *mark = TAILQ_FIRST(&(con->marks_head)); + TAILQ_REMOVE(&(con->marks_head), mark, marks); + FREE(mark->name); + FREE(mark); + } + free(con); + DLOG("con %p freed\n", con); +} + static void _con_attach(Con *con, Con *parent, Con *previous, bool ignore_focus) { con->parent = parent; Con *loop; diff --git a/src/load_layout.c b/src/load_layout.c index 7961e17f..0fa3e85b 100644 --- a/src/load_layout.c +++ b/src/load_layout.c @@ -18,6 +18,7 @@ /* TODO: refactor the whole parsing thing */ static char *last_key; +static int incomplete; static Con *json_node; static Con *to_focus; static bool parsing_swallows; @@ -68,6 +69,9 @@ static int json_start_map(void *ctx) { json_node->name = NULL; json_node->parent = parent; } + /* json_node is incomplete and should be removed if parsing fails */ + incomplete++; + DLOG("incomplete = %d\n", incomplete); } } return 1; @@ -166,6 +170,8 @@ static int json_end_map(void *ctx) { LOG("Creating window\n"); x_con_init(json_node); json_node = json_node->parent; + incomplete--; + DLOG("incomplete = %d\n", incomplete); } if (parsing_swallows && swallow_is_empty) { @@ -634,6 +640,7 @@ void tree_append_json(Con *con, const char *filename, char **errormsg) { yajl_status stat; json_node = con; to_focus = NULL; + incomplete = 0; parsing_swallows = false; parsing_rect = false; parsing_deco_rect = false; @@ -649,6 +656,12 @@ void tree_append_json(Con *con, const char *filename, char **errormsg) { if (errormsg != NULL) *errormsg = sstrdup((const char *)str); yajl_free_error(hand, str); + while (incomplete-- > 0) { + Con *parent = json_node->parent; + DLOG("freeing incomplete container %p\n", json_node); + con_free(json_node); + json_node = parent; + } } /* In case not all containers were restored, we need to fix the diff --git a/src/tree.c b/src/tree.c index 82a4756c..296a6a37 100644 --- a/src/tree.c +++ b/src/tree.c @@ -320,22 +320,7 @@ bool tree_close_internal(Con *con, kill_window_t kill_window, bool dont_kill_par DLOG("parent container killed\n"); } - free(con->name); - FREE(con->deco_render_params); - TAILQ_REMOVE(&all_cons, con, all_cons); - while (!TAILQ_EMPTY(&(con->swallow_head))) { - Match *match = TAILQ_FIRST(&(con->swallow_head)); - TAILQ_REMOVE(&(con->swallow_head), match, matches); - match_free(match); - free(match); - } - while (!TAILQ_EMPTY(&(con->marks_head))) { - mark_t *mark = TAILQ_FIRST(&(con->marks_head)); - TAILQ_REMOVE(&(con->marks_head), mark, marks); - FREE(mark->name); - FREE(mark); - } - free(con); + con_free(con); /* in the case of floating windows, we already focused another container * when closing the parent, so we can exit now. */ diff --git a/testcases/t/215-layout-restore-crash.t b/testcases/t/215-layout-restore-crash.t index 4430dac8..5b34c29c 100644 --- a/testcases/t/215-layout-restore-crash.t +++ b/testcases/t/215-layout-restore-crash.t @@ -131,14 +131,103 @@ print $fh <<'EOT'; EOT $fh->flush; my $reply = cmd "append_layout $filename"; -diag('reply = ' . Dumper($reply)); +ok(!$reply->[0]->{success}, 'IPC reply did not indicate success'); does_i3_live; -ok(!$reply->[0]->{success}, 'IPC reply did not indicate success'); close($fh); +################################################################################ +# another file with a superfluous trailing comma (issue #2755) +################################################################################ + +subtest 'issue 2755' => sub { + plan tests => 4; + $ws = fresh_workspace; + + @content = @{get_ws_content($ws)}; + is(@content, 0, 'no nodes on the new workspace yet'); + + ($fh, $filename) = tempfile(UNLINK => 1); + print $fh <<'EOT'; +// vim:ts=4:sw=4:et +{ + // splith split container with 2 children + "border": "normal", + "floating": "auto_off", + "layout": "splith", + "percent": null, + "type": "con", + "nodes": [ + { + "border": "normal", + "current_border_width": 2, + "floating": "auto_off", + "geometry": { + "height": 860, + "width": 1396, + "x": 1922, + "y": 38 + }, + "name": "Chromium1", + "percent": 0.5, + "swallows": [ + { + "class": "^Chromium$", + // "instance": "^chromium$", + // "title": "^Git\\ Tutorial\\ \\-\\ corp\\ \\-\\ Chromium$", + // "transient_for": "^$", + // "window_role": "^browser$" + } + ], + "type": "con" + }, + { + "border": "normal", + "current_border_width": 2, + "floating": "auto_off", + "geometry": { + "height": 1040, + "width": 956, + "x": 2, + "y": 38 + }, + "name": "Chromium2", + "percent": 0.5, + "swallows": [ + { + "class": "^Chromium$", + // "instance": "^chromium$", + // "title": "^Nutanix\\ \\-\\ Prod\\ \\-\\ Sign\\ In\\ \\-\\ Chromium$", + // "transient_for": "^$", + // "window_role": "^browser$" + } + ], + "type": "con" + } + ] +} + +EOT + $fh->flush; + $reply = cmd "append_layout $filename"; + ok(!$reply->[0]->{success}, 'IPC reply indicated success'); + + does_i3_live; + + # Move to a different workspace rendered the half-attached con’s con->parent + # invalid. + fresh_workspace; + + cmd '[urgent=latest] focus'; + $reply = cmd 'scratchpad show'; + + does_i3_live; + + close($fh); +}; + ################################################################################ # wrong percent key in a child node ################################################################################ -- 2.39.2