lxc-ls: check for ENOMEM and tweaking

- If lxc_container_new() fails we check for ENOMEM and if so goto out. If
  ENOMEM is not set we will simply continue. The same goes for the call to
  regcomp() but instead of checking for ENOMEM we need to check for REG_ESPACE.

- Tweaking: Since lxc-ls might have to gather a lot of containers and I don't
  know if compilers will always optimize this, let's move *some* variable
  declarations outside of the loop when it does not hinder readability.

- Set ls_nesting to 0 initially. Otherwise users will always see nested
  containers printed.

- ls_get() gains an argument char **lockpath which is a string pointing us to
  the lock we put under /run/lxc/lock/.../... so that we can remove the lock
  when we no longer need it. To avoid pointless memory allocation in each new
  recursion level, we share lockpath amongst all non-fork()ing recursive calls
  to ls_get().  As it is not guaranteed that realloc() does not do any memory
  moving when newlen == len_lockpath, we give ls_get() an additional argument
  size_t len_lockpath). Every time we have a non-fork()ing recursive call to
  ls_get() we check if newlen > len_lockpath and only then do we
  realloc(*lockpath, newlen * 2) a reasonable chunk of memory (as the path will
  keep growing) and set len_lockpath = newlen * 2 to pass to the next
  non-fork()ing recursive call to ls_get().
  To avoid keeping a variable char *lockpath in main() which serves no purpose
  whatsoever and might be abused later we use a compound literal
  &(char *){NULL} which gives us an anonymous pointer. This pointer we can use
  for memory allocation in ls_get() for lockpath. We can conveniently free() it
  in ls_get() when the nesting level parameter lvl == 0 after exiting the loop.
  The advantage is that the variable is only accessible within ls_get() and not
  in main() while at the same time giving us an easy way to share lockpath
  amongst all non-fork()ing recursive calls to ls_get().

Signed-off-by: Christian Brauner <christian.brauner@mailbox.org>
Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>

diff --git a/src/lxc/lxc_ls.c b/src/lxc/lxc_ls.c
index 513dbd65ea2451534105ad21102abdb2af60ec14..bfe37cb4ff59bb27ce660c31c5f223733e775a89 100644 (file)
--- a/src/lxc/lxc_ls.c
+++ b/src/lxc/lxc_ls.c
@@ -94,7 +94,7 @@ static void ls_free_arr(char **arr, size_t size);
  */
 static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
                struct lengths *lht, const char *basepath, const char *parent,
-               unsigned int lvl);
+               unsigned int lvl, char **lockpath, size_t len_lockpath);
 static char *ls_get_cgroup_item(struct lxc_container *c, const char *item);
 static char *ls_get_config_item(struct lxc_container *c, const char *item,
                bool running);
@@ -145,6 +145,8 @@ static void ls_print_table(struct ls *l, struct lengths *lht,
 static int ls_read_and_grow_buf(const int rpipefd, char **save_buf,
                const char *id, ssize_t nbytes_id,
                char **read_buf, ssize_t *read_buf_len);
+static int ls_remove_lock(const char *path, const char *name,
+               char **lockpath, size_t *len_lockpath, bool recalc);
 static int ls_serialize(int wpipefd, struct ls *n);
 static int ls_write(const int wpipefd, const char *id, ssize_t nbytes_id,
                const char *s);
@@ -191,8 +193,6 @@ Options :\n\
 
 int main(int argc, char *argv[])
 {
-       struct ls *ls_arr = NULL;
-       size_t ls_size = 0;
        int ret = EXIT_FAILURE;
        /*
         * The lxc parser requires that my_args.name is set. So let's satisfy
@@ -228,7 +228,12 @@ int main(int argc, char *argv[])
                .autostart_length = 9, /* AUTOSTART */
        };
 
-       int status = ls_get(&ls_arr, &ls_size, &my_args, &max_len, "", NULL, 0);
+       struct ls *ls_arr = NULL;
+       size_t ls_size = 0;
+       /* &(char *){NULL} is no magic. It's just a compound literal which
+        * avoids having a pointless variable in main() that serves no purpose
+        * here. */
+       int status = ls_get(&ls_arr, &ls_size, &my_args, &max_len, "", NULL, 0, &(char *){NULL}, 0);
        if (!ls_arr && status == 0)
                /* We did not fail. There was just nothing to do. */
                exit(EXIT_SUCCESS);
@@ -303,7 +308,7 @@ static void ls_free_arr(char **arr, size_t size)
 
 static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
                struct lengths *lht, const char *basepath, const char *parent,
-               unsigned int lvl)
+               unsigned int lvl, char **lockpath, size_t len_lockpath)
 {
        /* As ls_get() is non-tail recursive we face the inherent danger of
         * blowing up the stack at some level of nesting. To have at least some
@@ -341,6 +346,8 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
                goto out;
        }
 
+       char *tmp = NULL;
+       int check;
        struct ls *l = NULL;
        struct lxc_container *c = NULL;
        size_t i;
@@ -350,17 +357,23 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
                /* Filter container names by regex the user gave us. */
                if (args->ls_regex) {
                        regex_t preg;
-                       if (regcomp(&preg, args->ls_regex, REG_NOSUB | REG_EXTENDED) != 0)
+                       check = regcomp(&preg, args->ls_regex, REG_NOSUB | REG_EXTENDED);
+                       if (check == REG_ESPACE) /* we're out of memory */
+                               goto out;
+                       else if (check != 0)
                                continue;
-                       int rc = regexec(&preg, name, 0, NULL, 0);
+                       check = regexec(&preg, name, 0, NULL, 0);
                        regfree(&preg);
-                       if (rc != 0)
+                       if (check != 0)
                                continue;
                }
 
+               errno = 0;
                c = lxc_container_new(name, path);
-               if (!c)
-                       continue;
+               if ((errno == ENOMEM) && !c)
+                       goto out;
+               else if (!c)
+                       continue;
 
                if (!c->is_defined(c))
                        goto put_and_next;
@@ -390,8 +403,10 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
 
                /* Now it makes sense to allocate memory. */
                l = ls_new(m, size);
-               if (!l)
+               if (!l) {
+                       free(grp_tmp);
                        goto put_and_next;
+               }
 
                /* How deeply nested are we? */
                l->nestlvl = lvl;
@@ -422,7 +437,7 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
                        if (!l->state)
                                goto put_and_next;
 
-                       char *tmp = ls_get_config_item(c, "lxc.start.auto", running);
+                       tmp = ls_get_config_item(c, "lxc.start.auto", running);
                        if (tmp)
                                l->autostart = atoi(tmp);
                        free(tmp);
@@ -451,11 +466,9 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
                 * all other information we need. */
                if (args->ls_nesting && running) {
                        struct wrapargs wargs = (struct wrapargs){.args = NULL};
-
-                       /* Open a socket so that the child can communicate with
-                        * us. */
-                       int ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, wargs.pipefd);
-                       if (ret == -1)
+                       /* Open a socket so that the child can communicate with us. */
+                       check = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, wargs.pipefd);
+                       if (check == -1)
                                goto put_and_next;
 
                        /* Set the next nesting level. */
@@ -470,14 +483,14 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
                        lxc_attach_options_t aopt = LXC_ATTACH_OPTIONS_DEFAULT;
                        aopt.env_policy = LXC_ATTACH_CLEAR_ENV;
 
-                       /* fork(): Attach to the namespace of the child and run
-                        * ls_get() in it which is called in ls_get_wrapper(). */
-                       int status = c->attach(c, ls_get_wrapper, &wargs, &aopt, &out);
+                       /* fork(): Attach to the namespace of the container and
+                        * run ls_get() in it which is called in * ls_get_wrapper(). */
+                       check = c->attach(c, ls_get_wrapper, &wargs, &aopt, &out);
                        /* close the socket */
                        close(wargs.pipefd[1]);
 
                        /* Retrieve all information we want from the child. */
-                       if (status == 0)
+                       if (check == 0)
                                if (ls_deserialize(wargs.pipefd[0], m, size) == -1)
                                        goto put_and_next;
 
@@ -509,23 +522,17 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
                        if (!newpath)
                                goto put_and_next;
 
-                       /* We want to remove all locks we created under
+                       /* We want to remove all locks we create under
                         * /run/lxc/lock so we create a string pointing us to
                         * the lock path for the current container. */
-                       char lock_path[MAXPATHLEN];
-                       int ret = snprintf(lock_path, MAXPATHLEN, "%s/lxc/lock/%s/%s", RUNTIME_PATH, path, name);
-                       if (ret < 0 || ret >= MAXPATHLEN)
+                       if (ls_remove_lock(path, name, lockpath, &len_lockpath, true) == -1)
                                goto put_and_next;
 
-                       /* Remove the lock. */
-                       lxc_rmdir_onedev(lock_path, NULL);
-
-                       ls_get(m, size, args, lht, newpath, l->name, lvl + 1);
-
-                       /* Remove the lock. */
-                       lxc_rmdir_onedev(lock_path, NULL);
-
+                       ls_get(m, size, args, lht, newpath, l->name, lvl + 1, lockpath, len_lockpath);
                        free(newpath);
+
+                       /* Remove the lock. No need to check for failure here. */
+                       ls_remove_lock(path, name, lockpath, &len_lockpath, false)
                }
 
 put_and_next:
@@ -536,6 +543,10 @@ put_and_next:
 out:
        ls_free_arr(containers, num);
        free(path);
+       /* lockpath is shared amongst all non-fork()ing recursive calls to
+        * ls_get() so only free it on the uppermost level. */
+       if (lvl == 0)
+               free(*lockpath);
 
        return ret;
 }
@@ -932,7 +943,10 @@ static int ls_get_wrapper(void *wrap)
        /* close pipe */
        close(wargs->pipefd[0]);
 
-       ls_get(&m, &len, wargs->args, wargs->lht, "", wargs->parent, wargs->nestlvl);
+       /* &(char *){NULL} is no magic. It's just a compound literal which
+        * avoids having a pointless variable in main() that serves no purpose
+        * here. */
+       ls_get(&m, &len, wargs->args, wargs->lht, "", wargs->parent, wargs->nestlvl, &(char *){NULL}, 0);
        if (!m)
                goto out;
 
@@ -955,6 +969,30 @@ out:
        return ret;
 }
 
+static int ls_remove_lock(const char *path, const char *name,
+               char **lockpath, size_t *len_lockpath, bool recalc)
+{
+       /* Avoid doing unnecessary work if we can. */
+       if (recalc) {
+               size_t newlen = strlen(path) + strlen(name) + strlen(RUNTIME_PATH) + /* /+lxc+/+lock+/+/= */ 11 + 1;
+               if (newlen > *len_lockpath) {
+                       char *tmp = realloc(*lockpath, newlen * 2);
+                       if (!tmp)
+                               return -1;
+                       *lockpath = tmp;
+                       *len_lockpath = newlen * 2;
+               }
+       }
+
+       int check = snprintf(*lockpath, *len_lockpath, "%s/lxc/lock/%s/%s", RUNTIME_PATH, path, name);
+       if (check < 0 || check >= *len_lockpath)
+               return -1;
+
+       lxc_rmdir_onedev(*lockpath, NULL);
+
+       return 0;
+}
+
 static int ls_serialize(int wpipefd, struct ls *n)
 {
        ssize_t nbytes = sizeof(n->ram);