[lxc-devel] [PATCH] lxc-ls: try to protect stack in recursive function

[Serge Hallyn]

Quoting Christian Brauner (christian.brauner at mailbox.org):
> 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 security we define
> MAX_NESTLVL to be 5. That should be sufficient for most users. The argument lvl
> to ls_get() can be used to keep track of the level of nesting we are at. If lvl
> is greater than the allowed default level return (without error) and unwind the
> stack.
> 
> --nesting gains an optional numeric argument. This allows the user to specify
> the maximum level of nesting she/he wants to see. Fair warning: If your nesting
> level is really deep and/or you have a lot of containers your might run into
> trouble.
> 
> Signed-off-by: Christian Brauner <christian.brauner at mailbox.org>

Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>

> ---
>  src/lxc/arguments.h |  2 +-
>  src/lxc/lxc_ls.c    | 37 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/src/lxc/arguments.h b/src/lxc/arguments.h
> index 898a9ce..a3d2932 100644
> --- a/src/lxc/arguments.h
> +++ b/src/lxc/arguments.h
> @@ -124,11 +124,11 @@ struct lxc_arguments {
>  	char *ls_fancy_format;
>  	char *ls_groups;
>  	char *ls_regex;
> +	unsigned int ls_nesting; /* maximum allowed nesting level */
>  	bool ls_active;
>  	bool ls_fancy;
>  	bool ls_frozen;
>  	bool ls_line;
> -	bool ls_nesting;
>  	bool ls_running;
>  	bool ls_stopped;
>  
> diff --git a/src/lxc/lxc_ls.c b/src/lxc/lxc_ls.c
> index 3a7fa15..a4ab9b6 100644
> --- a/src/lxc/lxc_ls.c
> +++ b/src/lxc/lxc_ls.c
> @@ -40,6 +40,10 @@
>  lxc_log_define(lxc_ls, lxc);
>  
>  #define LINELEN 1024
> +/* Per default we only allow five levels of recursion to protect the stack at
> + * least a little bit. */
> +#define MAX_NESTLVL 5
> +
>  #define LS_FROZEN 1
>  #define LS_STOPPED 2
>  #define LS_ACTIVE 3
> @@ -154,7 +158,7 @@ static const struct option my_longopts[] = {
>  	{"running", no_argument, 0, LS_RUNNING},
>  	{"frozen", no_argument, 0, LS_FROZEN},
>  	{"stopped", no_argument, 0, LS_STOPPED},
> -	{"nesting", no_argument, 0, LS_NESTING},
> +	{"nesting", optional_argument, 0, LS_NESTING},
>  	{"groups", required_argument, 0, 'g'},
>  	{"regex", required_argument, 0, 'r'},
>  	LXC_COMMON_OPTIONS
> @@ -177,11 +181,12 @@ Options :\n\
>    --running          list only running containers\n\
>    --frozen           list only frozen containers\n\
>    --stopped          list only stopped containers\n\
> -  --nesting          list nested containers\n\
> +  --nesting=NUM      list nested containers up to NUM (default is 5) levels of nesting\n\
>    -r --regex         filter container names by regular expression\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,
>  };
>  
>  int main(int argc, char *argv[])
> @@ -300,6 +305,16 @@ 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)
>  {
> +	/* 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
> +	 * security we define MAX_NESTLVL to be 5. That should be sufficient for
> +	 * most users. The argument lvl can be used to keep track of the level
> +	 * of nesting we are at. If lvl is greater than the allowed default
> +	 * level or the level the user specified on the command line we return
> +	 * and unwind the stack. */
> +	if (lvl > args->ls_nesting)
> +		return 0;
> +
>  	int num = 0, ret = -1;
>  	char **containers = NULL;
>  	/* If we, at some level of nesting, encounter a stopped container but
> @@ -855,6 +870,8 @@ static void ls_print_table(struct ls *l, struct lengths *lht,
>  
>  static int my_parser(struct lxc_arguments *args, int c, char *arg)
>  {
> +	char *invalid;
> +	unsigned long int m, n = MAX_NESTLVL;
>  	switch (c) {
>  	case '1':
>  		args->ls_line = true;
> @@ -875,7 +892,21 @@ static int my_parser(struct lxc_arguments *args, int c, char *arg)
>  		args->ls_stopped = true;
>  		break;
>  	case LS_NESTING:
> -		args->ls_nesting = true;
> +		/* In case strtoul() receives a string that represents a
> +		 * negative number it will return ULONG_MAX - the number that
> +		 * string represents if the number the string represents is <
> +		 * ULONG_MAX and ULONG_MAX otherwise. But it will consider this
> +		 * valid input and not set errno. So we check manually if the
> +		 * first character of num_string == '-'. Otherwise the default
> +		 * level remains set. */
> +		if (arg && !(*arg == '-')) {
> +			errno = 0;
> +			m = strtoul(arg, &invalid, 0);
> +			/* ls_nesting has type unsigned int. */
> +			if (!errno && (*invalid == '\0') && (m <= UINT_MAX))
> +				n = m;
> +		}
> +		args->ls_nesting = n;
>  		break;
>  	case 'g':
>  		args->groups = arg;
> -- 
> 2.7.0
>