Merge pull request #2942 from stapelberg/append

Improve append_layout’s invalid JSON handling

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

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().

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);

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);

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);
+        goto out;
-        return;
     }
 
     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);
 }
 
 /*

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;

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) {
+json_content_t json_determine_content(const char *buf, const size_t len) {
-    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);
     // 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);
+    yajl_handle hand = yajl_alloc(&callbacks, NULL, NULL);
-    hand = yajl_alloc(&callbacks, NULL, (void *)g);
     /* 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);
-
+    yajl_free(hand);
-    fclose(f);
 
     return content_result;
 }
 
-void tree_append_json(Con *con, const char *filename, char **errormsg) {
+void tree_append_json(Con *con, const char *buf, const size_t len, 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;
     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);
+    yajl_handle hand = yajl_alloc(&callbacks, NULL, NULL);
-    hand = yajl_alloc(&callbacks, NULL, (void *)g);
     /* 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);
+    if (to_focus) {
-    free(buf);
-    if (to_focus)
         con_focus(to_focus);
+    }
 }

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);
+        goto out;
-        return false;
+    }
+
+    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);
+    tree_append_json(focused, buf, len, NULL);
-    free(globbed);
 
     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);
+    con_free(con);
-    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);
 
     /* in the case of floating windows, we already focused another container
      * when closing the parent, so we can exit now. */

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;
+}

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
 ################################################################################