diff --git a/AnyEvent-I3/lib/AnyEvent/I3.pm b/AnyEvent-I3/lib/AnyEvent/I3.pm index 75845ccd..8598f850 100644 --- a/AnyEvent-I3/lib/AnyEvent/I3.pm +++ b/AnyEvent-I3/lib/AnyEvent/I3.pm @@ -9,6 +9,7 @@ use AnyEvent::Socket; use AnyEvent; use Encode; use Scalar::Util qw(tainted); +use Carp; =head1 NAME @@ -186,7 +187,7 @@ sub new { # We use getpwuid() instead of $ENV{HOME} because the latter is tainted # and thus produces warnings when running tests with perl -T my $home = (getpwuid($<))[7]; - die "Could not get home directory" unless $home and -d $home; + confess "Could not get home directory" unless $home and -d $home; $path =~ s/~/$home/g; } @@ -330,9 +331,9 @@ scalar), if specified. sub message { my ($self, $type, $content) = @_; - die "No message type specified" unless defined($type); + confess "No message type specified" unless defined($type); - die "No connection to i3" unless defined($self->{ipchdl}); + confess "No connection to i3" unless defined($self->{ipchdl}); my $payload = ""; if ($content) { @@ -373,7 +374,7 @@ sub _ensure_connection { return if defined($self->{ipchdl}); - $self->connect->recv or die "Unable to connect to i3 (socket path " . $self->{path} . ")"; + $self->connect->recv or confess "Unable to connect to i3 (socket path " . $self->{path} . ")"; } =head2 get_workspaces 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/include/load_layout.h b/include/load_layout.h index 0dd81318..9205800f 100644 --- a/include/load_layout.h +++ b/include/load_layout.h @@ -31,6 +31,12 @@ typedef enum { * determine whether the file contains workspaces or regular containers, which * is important to know when deciding where (and how) to append the contents. * */ -json_content_t json_determine_content(const char *filename); +json_content_t json_determine_content(const char *buf, const size_t len); -void tree_append_json(Con *con, const char *filename, char **errormsg); +/** + * Returns true if the provided JSON could be parsed by yajl. + * + */ +bool json_validate(const char *buf, const size_t len); + +void tree_append_json(Con *con, const char *buf, const size_t len, char **errormsg); diff --git a/include/util.h b/include/util.h index 6c5fc4c6..de6fa568 100644 --- a/include/util.h +++ b/include/util.h @@ -164,3 +164,11 @@ void kill_nagbar(pid_t *nagbar_pid, bool wait_for_it); * if the number could be parsed. */ bool parse_long(const char *str, long *out, int base); + +/** + * Slurp reads path in its entirety into buf, returning the length of the file + * or -1 if the file could not be read. buf is set to a buffer of appropriate + * size, or NULL if -1 is returned. + * + */ +ssize_t slurp(const char *path, char **buf); diff --git a/src/commands.c b/src/commands.c index faee3916..2697d6e1 100644 --- a/src/commands.c +++ b/src/commands.c @@ -773,13 +773,25 @@ void cmd_append_layout(I3_CMD, const char *cpath) { /* Make sure we allow paths like '~/.i3/layout.json' */ path = resolve_tilde(path); - json_content_t content = json_determine_content(path); + char *buf = NULL; + ssize_t len; + if ((len = slurp(path, &buf)) < 0) { + /* slurp already logged an error. */ + goto out; + } + + if (!json_validate(buf, len)) { + ELOG("Could not parse \"%s\" as JSON, not loading.\n", path); + yerror("Could not parse \"%s\" as JSON.", path); + goto out; + } + + json_content_t content = json_determine_content(buf, len); LOG("JSON content = %d\n", content); if (content == JSON_CONTENT_UNKNOWN) { ELOG("Could not determine the contents of \"%s\", not loading.\n", path); yerror("Could not determine the contents of \"%s\".", path); - free(path); - return; + goto out; } Con *parent = focused; @@ -795,7 +807,7 @@ void cmd_append_layout(I3_CMD, const char *cpath) { } DLOG("Appending to parent=%p instead of focused=%p\n", parent, focused); char *errormsg = NULL; - tree_append_json(parent, path, &errormsg); + tree_append_json(parent, buf, len, &errormsg); if (errormsg != NULL) { yerror(errormsg); free(errormsg); @@ -820,8 +832,10 @@ void cmd_append_layout(I3_CMD, const char *cpath) { if (content == JSON_CONTENT_WORKSPACE) ipc_send_workspace_event("restored", parent, NULL); - free(path); cmd_output->needs_tree_render = true; +out: + free(path); + free(buf); } /* 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..071b3ccd 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) { @@ -532,36 +538,42 @@ static int json_determine_content_string(void *ctx, const unsigned char *val, si return 0; } +/* + * Returns true if the provided JSON could be parsed by yajl. + * + */ +bool json_validate(const char *buf, const size_t len) { + bool valid = true; + yajl_handle hand = yajl_alloc(NULL, NULL, NULL); + /* Allowing comments allows for more user-friendly layout files. */ + yajl_config(hand, yajl_allow_comments, true); + /* Allow multiple values, i.e. multiple nodes to attach */ + yajl_config(hand, yajl_allow_multiple_values, true); + + setlocale(LC_NUMERIC, "C"); + if (yajl_parse(hand, (const unsigned char *)buf, len) != yajl_status_ok) { + unsigned char *str = yajl_get_error(hand, 1, (const unsigned char *)buf, len); + ELOG("JSON parsing error: %s\n", str); + yajl_free_error(hand, str); + valid = false; + } + setlocale(LC_NUMERIC, ""); + + yajl_complete_parse(hand); + yajl_free(hand); + + return valid; +} + /* Parses the given JSON file until it encounters the first “type” property to * determine whether the file contains workspaces or regular containers, which * is important to know when deciding where (and how) to append the contents. * */ -json_content_t json_determine_content(const char *filename) { - FILE *f; - if ((f = fopen(filename, "r")) == NULL) { - ELOG("Cannot open file \"%s\"\n", filename); - return JSON_CONTENT_UNKNOWN; - } - struct stat stbuf; - if (fstat(fileno(f), &stbuf) != 0) { - ELOG("Cannot fstat() \"%s\"\n", filename); - fclose(f); - return JSON_CONTENT_UNKNOWN; - } - char *buf = smalloc(stbuf.st_size); - int n = fread(buf, 1, stbuf.st_size, f); - if (n != stbuf.st_size) { - ELOG("File \"%s\" could not be read entirely, not loading.\n", filename); - fclose(f); - return JSON_CONTENT_UNKNOWN; - } - DLOG("read %d bytes\n", n); +json_content_t json_determine_content(const char *buf, const size_t len) { // We default to JSON_CONTENT_CON because it is legal to not include // “"type": "con"” in the JSON files for better readability. content_result = JSON_CONTENT_CON; content_level = 0; - yajl_gen g; - yajl_handle hand; static yajl_callbacks callbacks = { .yajl_string = json_determine_content_string, .yajl_map_key = json_key, @@ -570,51 +582,27 @@ json_content_t json_determine_content(const char *filename) { .yajl_end_map = json_determine_content_shallower, .yajl_end_array = json_determine_content_shallower, }; - g = yajl_gen_alloc(NULL); - hand = yajl_alloc(&callbacks, NULL, (void *)g); + yajl_handle hand = yajl_alloc(&callbacks, NULL, NULL); /* Allowing comments allows for more user-friendly layout files. */ yajl_config(hand, yajl_allow_comments, true); /* Allow multiple values, i.e. multiple nodes to attach */ yajl_config(hand, yajl_allow_multiple_values, true); - yajl_status stat; setlocale(LC_NUMERIC, "C"); - stat = yajl_parse(hand, (const unsigned char *)buf, n); + const yajl_status stat = yajl_parse(hand, (const unsigned char *)buf, len); if (stat != yajl_status_ok && stat != yajl_status_client_canceled) { - unsigned char *str = yajl_get_error(hand, 1, (const unsigned char *)buf, n); + unsigned char *str = yajl_get_error(hand, 1, (const unsigned char *)buf, len); ELOG("JSON parsing error: %s\n", str); yajl_free_error(hand, str); } setlocale(LC_NUMERIC, ""); yajl_complete_parse(hand); - - fclose(f); + yajl_free(hand); return content_result; } -void tree_append_json(Con *con, const char *filename, char **errormsg) { - FILE *f; - if ((f = fopen(filename, "r")) == NULL) { - ELOG("Cannot open file \"%s\"\n", filename); - return; - } - struct stat stbuf; - if (fstat(fileno(f), &stbuf) != 0) { - ELOG("Cannot fstat() \"%s\"\n", filename); - fclose(f); - return; - } - char *buf = smalloc(stbuf.st_size); - int n = fread(buf, 1, stbuf.st_size, f); - if (n != stbuf.st_size) { - ELOG("File \"%s\" could not be read entirely, not loading.\n", filename); - fclose(f); - return; - } - DLOG("read %d bytes\n", n); - yajl_gen g; - yajl_handle hand; +void tree_append_json(Con *con, const char *buf, const size_t len, char **errormsg) { static yajl_callbacks callbacks = { .yajl_boolean = json_bool, .yajl_integer = json_int, @@ -625,15 +613,14 @@ void tree_append_json(Con *con, const char *filename, char **errormsg) { .yajl_end_map = json_end_map, .yajl_end_array = json_end_array, }; - g = yajl_gen_alloc(NULL); - hand = yajl_alloc(&callbacks, NULL, (void *)g); + yajl_handle hand = yajl_alloc(&callbacks, NULL, NULL); /* Allowing comments allows for more user-friendly layout files. */ yajl_config(hand, yajl_allow_comments, true); /* Allow multiple values, i.e. multiple nodes to attach */ yajl_config(hand, yajl_allow_multiple_values, true); - yajl_status stat; json_node = con; to_focus = NULL; + incomplete = 0; parsing_swallows = false; parsing_rect = false; parsing_deco_rect = false; @@ -642,13 +629,19 @@ void tree_append_json(Con *con, const char *filename, char **errormsg) { parsing_focus = false; parsing_marks = false; setlocale(LC_NUMERIC, "C"); - stat = yajl_parse(hand, (const unsigned char *)buf, n); + const yajl_status stat = yajl_parse(hand, (const unsigned char *)buf, len); if (stat != yajl_status_ok) { - unsigned char *str = yajl_get_error(hand, 1, (const unsigned char *)buf, n); + unsigned char *str = yajl_get_error(hand, 1, (const unsigned char *)buf, len); ELOG("JSON parsing error: %s\n", str); 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 @@ -659,10 +652,8 @@ void tree_append_json(Con *con, const char *filename, char **errormsg) { setlocale(LC_NUMERIC, ""); yajl_complete_parse(hand); yajl_free(hand); - yajl_gen_free(g); - fclose(f); - free(buf); - if (to_focus) + if (to_focus) { con_focus(to_focus); + } } diff --git a/src/tree.c b/src/tree.c index 82a4756c..b3d2ce93 100644 --- a/src/tree.c +++ b/src/tree.c @@ -64,12 +64,19 @@ static Con *_create___i3(void) { * */ bool tree_restore(const char *path, xcb_get_geometry_reply_t *geometry) { + bool result = false; char *globbed = resolve_tilde(path); if (!path_exists(globbed)) { LOG("%s does not exist, not restoring tree\n", globbed); - free(globbed); - return false; + goto out; + } + + char *buf = NULL; + ssize_t len; + if ((len = slurp(globbed, &buf)) < 0) { + /* slurp already logged an error. */ + goto out; } /* TODO: refactor the following */ @@ -81,8 +88,7 @@ bool tree_restore(const char *path, xcb_get_geometry_reply_t *geometry) { geometry->height}; focused = croot; - tree_append_json(focused, globbed, NULL); - free(globbed); + tree_append_json(focused, buf, len, NULL); DLOG("appended tree, using new root\n"); croot = TAILQ_FIRST(&(croot->nodes_head)); @@ -104,8 +110,12 @@ bool tree_restore(const char *path, xcb_get_geometry_reply_t *geometry) { } restore_open_placeholder_windows(croot); + result = true; - return true; +out: + free(globbed); + free(buf); + return result; } /* @@ -320,22 +330,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/src/util.c b/src/util.c index 32c3c57e..cd5ee03e 100644 --- a/src/util.c +++ b/src/util.c @@ -475,3 +475,35 @@ bool parse_long(const char *str, long *out, int base) { *out = result; return true; } + +/* + * Slurp reads path in its entirety into buf, returning the length of the file + * or -1 if the file could not be read. buf is set to a buffer of appropriate + * size, or NULL if -1 is returned. + * + */ +ssize_t slurp(const char *path, char **buf) { + FILE *f; + if ((f = fopen(path, "r")) == NULL) { + ELOG("Cannot open file \"%s\": %s\n", path, strerror(errno)); + return -1; + } + struct stat stbuf; + if (fstat(fileno(f), &stbuf) != 0) { + ELOG("Cannot fstat() \"%s\": %s\n", path, strerror(errno)); + fclose(f); + return -1; + } + /* Allocate one extra NUL byte to make the buffer usable with C string + * functions. yajl doesn’t need this, but this makes slurp safer. */ + *buf = scalloc(stbuf.st_size + 1, 1); + size_t n = fread(*buf, 1, stbuf.st_size, f); + fclose(f); + if ((ssize_t)n != stbuf.st_size) { + ELOG("File \"%s\" could not be read entirely: got %zd, want %zd\n", path, n, stbuf.st_size); + free(buf); + *buf = NULL; + return -1; + } + return (ssize_t)n; +} 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 ################################################################################