Re: [PATCH bpf-next v1 2/7] tools: bpftool: fix error message on invalid argument count

[Jakub Kicinski]

On Wed, 20 Mar 2019 18:33:27 +0100, Alban Crequy wrote:
> From: Alban Crequy <alban@xxxxxxxxxx>
> 
> Symptoms:
> $ sudo bpftool map create /sys/fs/bpf/dir/foobar type hash key 8 value 8 entries
> Error: '8' needs at least 2 arguments, 1 found
> 
> After this patch:
> $ sudo bpftool map create /sys/fs/bpf/dir/foobar type hash key 8 value 8 entries
> Error: can't parse max entries: missing argument
> 
> Signed-off-by: Alban Crequy <alban@xxxxxxxxxx>
> ---
>  tools/bpf/bpftool/common.c |  5 +++++
>  tools/bpf/bpftool/main.h   | 11 +++++++----
>  tools/bpf/bpftool/map.c    | 15 ++++++++++++---
>  tools/bpf/bpftool/prog.c   |  2 --
>  4 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index f7261fad45c1..e560cd8f66bc 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -627,6 +627,11 @@ int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what)
>  		return -1;
>  	}
>  
> +	if (*argc == 0) {
> +		p_err("can't parse %s: missing argument", what);
> +		return -1;
> +	}
> 
>  	*val = strtoul(**argv, &endptr, 0);
>  	if (*endptr) {
>  		p_err("can't parse %s as %s", **argv, what);
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index d7dd84d3c660..7fc2973446d0 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -22,14 +22,17 @@
>  #define NEXT_ARGP()	({ (*argc)--; (*argv)++; if (*argc < 0) usage(); })
>  #define BAD_ARG()	({ p_err("what is '%s'?", *argv); -1; })
>  #define GET_ARG()	({ argc--; *argv++; })
> -#define REQ_ARGS(cnt)							\
> +
> +#define REQ_ARGS(cnt)	REQ_ARGS_GENERIC(cnt, argc, argv)
> +#define REQ_ARGSP(cnt)	REQ_ARGS_GENERIC(cnt, *argc, *argv)
> +#define REQ_ARGS_GENERIC(cnt, ARGC, ARGV)				\
>  	({								\
>  		int _cnt = (cnt);					\
>  		bool _res;						\
>  									\
> -		if (argc < _cnt) {					\
> -			p_err("'%s' needs at least %d arguments, %d found", \
> -			      argv[-1], _cnt, argc);			\
> +		if ((ARGC) < _cnt) {					\
> +			p_err("'%s' needs at least %d arguments, %d found",	\
> +			      (ARGV)[-1], _cnt, (ARGC));			\
>  			_res = false;					\
>  		} else {						\
>  			_res = true;					\
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 994a7e0d16fb..18f9bc3aed4f 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -98,6 +98,9 @@ int map_parse_fd(int *argc, char ***argv)
>  		char *endptr;
>  
>  		NEXT_ARGP();
> +		if (!REQ_ARGSP(1)) {
> +			return -1;
> +		}
>  
>  		id = strtoul(**argv, &endptr, 0);
>  		if (*endptr) {
> @@ -114,6 +117,9 @@ int map_parse_fd(int *argc, char ***argv)
>  		char *path;
>  
>  		NEXT_ARGP();
> +		if (!REQ_ARGSP(1)) {
> +			return -1;
> +		}
>  
>  		path = **argv;
>  		NEXT_ARGP();
> @@ -1100,11 +1106,10 @@ static int do_create(int argc, char **argv)
>  	pinfile = GET_ARG();
>  
>  	while (argc) {
> -		if (!REQ_ARGS(2))
> -			return -1;

Seems like it'd be better to make a version of REQ_ARGS() which
uses the next arg in the error message (if it exists) rather than
pushing all the checks out?

I think the format of the parsing loop is quite canonical here, with
all arguments being in the <keyword> <value> form, therefore I think
it's worthwhile improving the REQ_ARGS(2) check.

>  		if (is_prefix(*argv, "type")) {
>  			NEXT_ARG();
> +			if (!REQ_ARGS(1))
> +				return -1;
>  
>  			if (attr.map_type) {
>  				p_err("map type already specified");
> @@ -1119,6 +1124,8 @@ static int do_create(int argc, char **argv)
>  			NEXT_ARG();
>  		} else if (is_prefix(*argv, "name")) {
>  			NEXT_ARG();
> +			if (!REQ_ARGS(1))
> +				return -1;
>  			attr.name = GET_ARG();
>  		} else if (is_prefix(*argv, "key")) {
>  			if (parse_u32_arg(&argc, &argv, &attr.key_size,
> @@ -1138,6 +1145,8 @@ static int do_create(int argc, char **argv)
>  				return -1;
>  		} else if (is_prefix(*argv, "dev")) {
>  			NEXT_ARG();
> +			if (!REQ_ARGS(1))
> +				return -1;
>  
>  			if (attr.map_ifindex) {
>  				p_err("offload device already specified");
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 8ef80d65a474..e0875ef5e444 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -836,8 +836,6 @@ static int parse_attach_detach_args(int argc, char **argv, int *progfd,
>  	}
>  
>  	NEXT_ARG();
> -	if (!REQ_ARGS(2))
> -		return -EINVAL;
>  
>  	*mapfd = map_parse_fd(&argc, &argv);
>  	if (*mapfd < 0)