[PATCH 1/3] cmd: setexpr: add fmt format string operation

[Rasmus Villemoes]

On 28/06/2021 17.17, Roland Gaudig wrote:
> From: Roland Gaudig <roland.gaudig at weidmueller.com>
> 
> Add format string "fmt" operation to the setexpr command
> which converts the input value according the format string
> specification and stores the result into the variable named name.
> 
> Signed-off-by: Roland Gaudig <roland.gaudig at weidmueller.com>
> ---
> 
>  cmd/setexpr.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 101 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/setexpr.c b/cmd/setexpr.c
> index e828be3970..b69cbab3dd 100644
> --- a/cmd/setexpr.c
> +++ b/cmd/setexpr.c
> @@ -11,12 +11,15 @@
>  #include <common.h>
>  #include <config.h>
>  #include <command.h>
> +#include <ctype.h>
>  #include <env.h>
>  #include <log.h>
>  #include <malloc.h>
>  #include <mapmem.h>
>  #include <linux/sizes.h>
>  
> +#define MAX_STR_LEN 128
> +
>  /**
>   * struct expr_arg: Holds an argument to an expression
>   *
> @@ -361,6 +364,95 @@ static int regex_sub_var(const char *name, const char *r, const char *s,
>  }
>  #endif
>  
> +/**
> + * setexpr_fmt_spec_search() - Search for format specifier
> + *
> + * This function searches the intput string for the first occurrence of a
> + * format specifier which starts with a %. Double % are ignored.
> + *
> + * @format: C like format string to search
> + * @return: Pointer to found format specifier, NULL in case none is found
> + */
> +static char *setexpr_fmt_spec_search(char *format)
> +{
> +	while (*format != '\0') {
> +		if (*format == '%') {
> +			switch (*(format + 1)) {
> +			case '%':
> +				/* found '%%', not a format specifier, skip. */
> +				format++;
> +				break;
> +			case '\0':
> +				/* found '%' at end of string,
> +				 * incomplete format specifier.
> +				 */
> +				return NULL;
> +			default:
> +				/* looks like a format specifier */
> +				return format;
> +			}
> +		}
> +		format++;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * setexpr_fmt() - Implements the setexpr <name> fmt command
> + *
> + * This function implements the setexpr <name> fmt <format> <value> command.
> + *
> + * @name: Name of the environment variable to save the evaluated expression in
> + * @format: C like format string
> + * @value: Input value to be converted
> + * @return: 0 if OK, 1 on error
> + */
> +static int setexpr_fmt(char *name, char *format, char *value)
> +{
> +	struct expr_arg aval;
> +	int data_size;
> +	char str[MAX_STR_LEN];
> +	int fmt_len = strlen(format);
> +
> +	if (fmt_len < 2) {
> +		printf("Error: missing format string");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	/* Search format specifier */
> +	char *first = setexpr_fmt_spec_search(format);
> +
> +	/* Exactly one format specifier is required */
> +	if (!first || setexpr_fmt_spec_search(first + 1)) {
> +		printf("Error: exactly one format specifier is required\n");
> +		return CMD_RET_FAILURE;
> +	}

I think this allows a lone % to appear at the end of the string after
the actual %d, i.e. it seems "%d bla %" would be accepted.

> +
> +	/* Search type field of format specifier */
> +	while (*first && !isalpha(*first))
> +		first++;

This will allow stuff like %*d or %30.*x both of which are format
strings expecting two integer arguments, the first specifying field
width or precision, respectively.


> +	/* Test if type is supported */
> +	if (!strchr("diouxX", *first)) {
> +		printf("Error: format type not supported\n");
> +		return CMD_RET_FAILURE;
> +	}

So this insists that the format specifier is one that expects a
corresponding "int" or "unsigned int" argument (no l or ll modifier) [*]

For something like "%123", the loop above would have made first point at
the nul at the end of the string, and strchr(whatever, '\0') is required
to consider the nul at the end of whatever part of the string, i.e. that
is equivalent to whatever+strlen(whatever), so that format string would
falsely be accepted. So you either need to explicitly test *first for
being non-zero, or write it in terms of switch(*first).

> +	data_size = cmd_get_data_size(value, 4);
> +
> +	if (data_size == CMD_DATA_SIZE_STR) {
> +		free(aval.sval);

Is aval or aval.sval even initialized at this point? I'd expect the
compiler to complain about this.

> +		return CMD_RET_FAILURE;
> +	}
> +
> +	if (get_arg(value, data_size, &aval))
> +		return CMD_RET_FAILURE;
> +
> +	snprintf(str, sizeof(str), format, aval.ival);

[*] but you pass an "unsigned long".

Also, I think you should check for overflow and make it fail instead of
silently assigning a truncated string to the target.

I think allowing "arbitrary" format strings, restricted to those
expected exactly one integer argument, is too fragile and error-prone.
If somebody wants the result prepended by "100% max ", it's just as easy
to do

env set foo "100% max $foo"

right after the setexpr.

It's not that the above are impossible to fix; you can insist on the
specifier ending in "l[diouxX]" (i.e., require the l modifier), but that
makes it somewhat less ergonomic. Casting aval.ival to (int) or
(unsigned int) should not be an option; on 64 bit machines that would
make a lot of values impossible to handle.

Rasmus