[lxc-devel] [PATCH v2] lxc-ls: check for ENOMEM and tweaking

Serge Hallyn serge.hallyn at ubuntu.com
Sat Jan 16 00:50:01 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.
> 
> Signed-off-by: Christian Brauner <christian.brauner at mailbox.org>
> ---
> Changelog:
>         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 | 55 ++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/src/lxc/lxc_ls.c b/src/lxc/lxc_ls.c
> index a4ab9b6..31279fb 100644
> --- a/src/lxc/lxc_ls.c
> +++ b/src/lxc/lxc_ls.c
> @@ -186,7 +186,7 @@ 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[])
> @@ -315,7 +315,7 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
>  	if (lvl > args->ls_nesting)
>  		return 0;
>  
> -	int num = 0, ret = -1;
> +	int ret = -1;

>  	char **containers = NULL;
>  	/* If we, at some level of nesting, encounter a stopped container but
>  	 * want to retrieve nested containers we need to build an absolute path
> @@ -331,6 +331,7 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
>  	if (!dir_exists(path))
>  		goto out;
>  
> +	int num = 0;

....  :(

>  	/* Do not do more work than is necessary right from the start. */
>  	if (args->ls_active || (args->ls_active && args->ls_frozen))
>  		num = list_active_containers(path, &containers, NULL);
> @@ -341,6 +342,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 +353,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 +399,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 +433,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 +462,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. */
> @@ -472,12 +481,12 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
>  
>  			/* 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);
> +			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;
>  
> @@ -512,20 +521,24 @@ static int ls_get(struct ls **m, size_t *size, const struct lxc_arguments *args,
>  			/* We want to remove all locks we created 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)
> +			size_t len = strlen(path) + strlen(name) + strlen(RUNTIME_PATH) + /* /+lxc+/+lock+/+/= */ 11 + 1;
> +			char *lock_path = malloc(len);

Actually I meant keep lock_path for the duration of the function, use
realloc here, and free at the very end.

Is there an advantage to doing it as here?  If so (apart from too
much churn from variables moving around)

> +			if (!lock_path)
> +				goto put_and_next;
> +
> +			check = snprintf(lock_path, len, "%s/lxc/lock/%s/%s", RUNTIME_PATH, path, name);
> +			if (check < 0 || (size_t)check >= len)

but if you do use per-iteration malloc then you need to free here.

>  				goto put_and_next;
>  
>  			/* Remove the lock. */
>  			lxc_rmdir_onedev(lock_path, NULL);
>  
>  			ls_get(m, size, args, lht, newpath, l->name, lvl + 1);
> +			free(newpath);
>  
>  			/* Remove the lock. */
>  			lxc_rmdir_onedev(lock_path, NULL);
> -
> -			free(newpath);
> +			free(lock_path);
>  		}
>  
>  put_and_next:
> -- 
> 2.7.0
> 
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel


More information about the lxc-devel mailing list