[lxc-devel] [PATCH v3 - resend] lxc-ls: check for ENOMEM and tweaking
Serge Hallyn
serge.hallyn at ubuntu.com
Mon Jan 18 19:28:56 UTC 2016
Quoting Christian Brauner (christian.brauner at mailbox.org):
> - 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 at mailbox.org>
Thanks
Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
> ---
> First v3 version of patch somehow did not make it to the mailing list so
> resending.
>
> Changelog:
> 2016-01-16T0013:
> - actually reallocate newlen * 2 and not simply newlen to
> avoid calling realloc() unnecessarily often
> 2016-01-16T1752:
> - rework: dynamically allocate lock_path instead of
> lock_path[MAXPATHLEN]:
> Move removing the lock to a separate function
> ls_remove_lock() which we call before and after any
> non-fork()ing recursive calls to ls_get() to remove the lock
> from /run/lxc/lock/.../...
> - ls_get() gains arguments char **lockpath and
> size_t len_lockpath. We share lockpath amongst all
> non-fork()ing recursive calls to ls_get() and use
> len_lockpath to check if realloc()ing makes sense or not.
> Details for the implementation can be found in the commit
> message.
> 2016-01-16T0100:
> - free() grp_tmp when ls_new() fails to prevent memory leak.
> - dynamically allocate lock_path instead of
> lock_path[MAXPATHLEN]:
> I don't know if realloc() will gain us much here. Instead we
> simply call malloc() and free() it once the recursive
> function
> returns.
> - set ls_nesting = 0 to prevent always listing nesting
> containers
> ---
> src/lxc/lxc_ls.c | 108 +++++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 73 insertions(+), 35 deletions(-)
>
> diff --git a/src/lxc/lxc_ls.c b/src/lxc/lxc_ls.c
> index a4ab9b6..bfe37cb 100644
> --- 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);
> @@ -186,13 +188,11 @@ Options :\n\
> -g --groups comma separated list of groups a container must have to be displayed\n",
> .options = my_longopts,
> .parser = my_parser,
> - .ls_nesting = MAX_NESTLVL,
> + .ls_nesting = 0,
> };
>
> 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);
> --
> 2.7.0
>
More information about the lxc-devel
mailing list